[DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables #23023

vudaltsov · 2017-06-01T20:52:21Z

Q	A
Branch?	2.8 and higher
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Note that Embeddables appeared only in doctrine/orm 2.5. I added class_exists checks for that.

"doctrine/orm": "^2.5" only

nicolas-grekas · 2017-06-01T21:01:35Z

src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php

+            return;
+        }
+
+        $expectedTypes = [new Type(


please use array() instead of []

thank you, changed PHP version to 5.3 in PhpStorm for this project and fixed declaration

vudaltsov · 2017-06-01T21:40:30Z

Suddenly I faced another problem with embeddables.

When an entity has properties with embeddables, Doctrine\Common\Persistence\Mapping\ClassMetadata::getFieldNames() returns property paths for them. Example:

/**
 * @ORM\Entity
 */
class DoctrineWithEmbedded
{
    /**
     * @Id
     * @Column(type="smallint")
     */
    public $id;

    /**
     * @Embedded(class="DoctrineEmbeddable")
     */
    protected $embedded;
}

/**
 * @ORM\Embeddable
 */
class DoctrineEmbeddable
{
    /**
     * @Column(type="string")
     */
    protected $field;
}

$container->get('doctrine.orm.default_entity_manager.metadata_factory')
    ->getMetadataFor('DoctrineWithEmbedded')
    ->getFieldNames();

// returns
[
    'id',
    'embedded.field',
];

Fixed DoctrineExtractor::getProperties() in the next commit.

stloyd · 2017-06-02T07:46:11Z

src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php

+    public function testGetEmbeddableProperties()
+    {
+        if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
+            return;


You should put $this->markTestSkipped() with message that embedded is not available.

Thank you, fixed this!

stloyd · 2017-06-02T07:46:19Z

src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php

+    public function testExtractEmbeddable()
+    {
+        if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
+            return;


vudaltsov · 2017-06-11T14:38:45Z

@nicolas-grekas, anything else needed here? could it be merged?

dunglas · 2017-06-14T18:30:56Z

This should be merged in 3.4 (it's a new feature, not a bug fix).

dunglas

Nice improvement! I just left some minor comments.

dunglas · 2017-06-14T18:31:36Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

+        $properties = array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+
+        if (class_exists('Doctrine\ORM\Mapping\Embedded')) {
+            if ($metadata instanceof ClassMetadataInfo && count($metadata->embeddedClasses) > 0) {


&& $metadata->embeddedClasses will be faster.

dunglas · 2017-06-14T18:32:30Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

-        return array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+        $properties = array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+
+        if (class_exists('Doctrine\ORM\Mapping\Embedded')) {


As we'll merge this PR in 3.4, you can import this class and do class_exists(Embedded::class).

dunglas · 2017-06-14T18:34:10Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

@@ -105,6 +117,16 @@ public function getTypes($class, $property, array $context = array())
            ));
        }

+        if (class_exists('Doctrine\ORM\Mapping\Embedded')) {


same here, same in tests.

vudaltsov · 2017-06-14T18:55:29Z

@dunglas, the problem is that now when you do

$container
    ->get('doctrine.orm.default_entity_manager.property_info_extractor')
    ->getProperties(DoctrineWithEmbedded::class); // the class from my example above

you receive

[
    'id',
    'embedded.field', // but not `embedded`
];

which is not what developer might expect.

So it appears that currently 2.8 doesn't fully support ORM, which might lead to unexpected behaviour. Isn't it a bug?

dunglas · 2017-06-29T12:17:50Z

Ok to consider it a bug fix then. 👍

ping @symfony/deciders

vudaltsov · 2017-07-13T12:42:48Z

anything else needed here?

fabpot · 2017-07-04T10:47:45Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

+        $properties = array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+
+        if (class_exists('Doctrine\ORM\Mapping\Embedded')) {
+            if ($metadata instanceof ClassMetadataInfo && $metadata->embeddedClasses) {


could be merge with the previous if condition

fabpot · 2017-07-04T10:48:09Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

@@ -105,6 +117,16 @@ public function getTypes($class, $property, array $context = array())
            ));
        }

+        if (class_exists('Doctrine\ORM\Mapping\Embedded')) {
+            if ($metadata instanceof ClassMetadataInfo && isset($metadata->embeddedClasses[$property])) {


Same here, you can merge the 2 conditions.

fabpot · 2017-07-04T10:48:17Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

+                    Type::BUILTIN_TYPE_OBJECT,
+                    false,
+                    $metadata->embeddedClasses[$property]['class']
+                ));


could be on one line

dunglas · 2017-07-22T13:54:07Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

-        return array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+        $properties = array_merge($metadata->getFieldNames(), $metadata->getAssociationNames());
+
+        if (class_exists('Doctrine\ORM\Mapping\Embedded') && $metadata instanceof ClassMetadataInfo && $metadata->embeddedClasses) {


Can you call class_exists after the other tests (for perf, function calls cost more than other checks)?

@dunglas, when class Doctrine\ORM\Mapping\Embedded doesn't exist, $metadata->embeddedClasses is not defined. I can only move instanceof check to the first place.

Ok can you do that? Then I'll merge :)

@dunglas, done!

dunglas · 2017-07-22T16:46:48Z

Thank you @vudaltsov.

…Embeddables (vudaltsov) This PR was squashed before being merged into the 2.8 branch (closes #23023). Discussion ---------- [DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables | Q | A | ------------- | --- | Branch? | 2.8 and higher | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Note that [Embeddables appeared only in doctrine/orm 2.5](http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/changelog/migration_2_5.html). I added class_exists checks for that. Commits ------- 7816f3b [DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables

Added support for Doctrine Embeddables

b1c1b2f

"doctrine/orm": "^2.5" only

carsonbot added Status: Needs Review DoctrineBridge PropertyInfo Bug labels Jun 1, 2017

nicolas-grekas reviewed Jun 1, 2017

View reviewed changes

nicolas-grekas added this to the 2.8 milestone Jun 1, 2017

Fixed array declaration

e1deefd

Fixed getProperties() for embeddables

2420190

stloyd suggested changes Jun 2, 2017

View reviewed changes

Corrected test methods names and added markTestSkipped()

d572d53

stloyd approved these changes Jun 2, 2017

View reviewed changes

dunglas requested changes Jun 14, 2017

View reviewed changes

dunglas approved these changes Jun 29, 2017

View reviewed changes

Improved ClassMetadataInfo::$embeddedClasses nonempty array check

fff8330

xabbuh approved these changes Jul 13, 2017

View reviewed changes

fabpot reviewed Jul 13, 2017

View reviewed changes

Codestyle fix and if optimizations

42ffb14

dunglas reviewed Jul 22, 2017

View reviewed changes

Performance improvements

268f545

dunglas closed this Jul 22, 2017

vudaltsov deleted the 2.8 branch July 31, 2017 19:51

This was referenced Aug 1, 2017

Release v2.8.26 #23735

Merged

Release v3.2.13 #23736

Merged

Release v3.3.6 #23738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables #23023

[DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables #23023

vudaltsov commented Jun 1, 2017

nicolas-grekas Jun 1, 2017

vudaltsov Jun 1, 2017

vudaltsov commented Jun 1, 2017 •

edited

Loading

stloyd Jun 2, 2017

vudaltsov Jun 2, 2017

stloyd Jun 2, 2017

vudaltsov commented Jun 11, 2017

dunglas commented Jun 14, 2017

dunglas left a comment

dunglas Jun 14, 2017

dunglas Jun 14, 2017

dunglas Jun 14, 2017

vudaltsov commented Jun 14, 2017 •

edited

Loading

dunglas commented Jun 29, 2017

vudaltsov commented Jul 13, 2017 •

edited

Loading

fabpot Jul 4, 2017

fabpot Jul 4, 2017

fabpot Jul 4, 2017

dunglas Jul 22, 2017

vudaltsov Jul 22, 2017

dunglas Jul 22, 2017

vudaltsov Jul 22, 2017

dunglas commented Jul 22, 2017

[DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables #23023

[DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables #23023

Conversation

vudaltsov commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vudaltsov commented Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vudaltsov commented Jun 11, 2017

dunglas commented Jun 14, 2017

dunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vudaltsov commented Jun 14, 2017 • edited Loading

dunglas commented Jun 29, 2017

vudaltsov commented Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Jul 22, 2017

vudaltsov commented Jun 1, 2017 •

edited

Loading

vudaltsov commented Jun 14, 2017 •

edited

Loading

vudaltsov commented Jul 13, 2017 •

edited

Loading