New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into
from

Conversation

@vudaltsov
Contributor

vudaltsov commented Jun 1, 2017

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.

Added support for Doctrine Embeddables
"doctrine/orm": "^2.5" only
Show outdated Hide outdated src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php
return;
}
$expectedTypes = [new Type(

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 1, 2017

Member

please use array() instead of []

@nicolas-grekas

nicolas-grekas Jun 1, 2017

Member

please use array() instead of []

This comment has been minimized.

@vudaltsov

vudaltsov Jun 1, 2017

Contributor

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

@vudaltsov

vudaltsov Jun 1, 2017

Contributor

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

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

@vudaltsov

This comment has been minimized.

Show comment
Hide comment
@vudaltsov

vudaltsov Jun 1, 2017

Contributor

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.

Contributor

vudaltsov commented Jun 1, 2017

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.

Show outdated Hide outdated src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php
public function testGetEmbeddableProperties()
{
if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
return;

This comment has been minimized.

@stloyd

stloyd Jun 2, 2017

Contributor

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

@stloyd

stloyd Jun 2, 2017

Contributor

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

This comment has been minimized.

@vudaltsov

vudaltsov Jun 2, 2017

Contributor

Thank you, fixed this!

@vudaltsov

vudaltsov Jun 2, 2017

Contributor

Thank you, fixed this!

Show outdated Hide outdated src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php
public function testExtractEmbeddable()
{
if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
return;

This comment has been minimized.

@stloyd

stloyd Jun 2, 2017

Contributor

Same here.

@stloyd

stloyd Jun 2, 2017

Contributor

Same here.

@stloyd

stloyd approved these changes Jun 2, 2017

@vudaltsov

This comment has been minimized.

Show comment
Hide comment
@vudaltsov

vudaltsov Jun 11, 2017

Contributor

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

Contributor

vudaltsov commented Jun 11, 2017

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

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Jun 14, 2017

Member

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

Member

dunglas commented Jun 14, 2017

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.

Show outdated Hide outdated 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) {

This comment has been minimized.

@dunglas

dunglas Jun 14, 2017

Member

&& $metadata->embeddedClasses will be faster.

@dunglas

dunglas Jun 14, 2017

Member

&& $metadata->embeddedClasses will be faster.

Show outdated Hide outdated 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')) {

This comment has been minimized.

@dunglas

dunglas Jun 14, 2017

Member

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

@dunglas

dunglas Jun 14, 2017

Member

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

Show outdated Hide outdated 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')) {

This comment has been minimized.

@dunglas

dunglas Jun 14, 2017

Member

same here, same in tests.

@dunglas

dunglas Jun 14, 2017

Member

same here, same in tests.

@vudaltsov

This comment has been minimized.

Show comment
Hide comment
@vudaltsov

vudaltsov Jun 14, 2017

Contributor

@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?

Contributor

vudaltsov commented Jun 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Jun 29, 2017

Member

Ok to consider it a bug fix then. 👍

ping @symfony/deciders

Member

dunglas commented Jun 29, 2017

Ok to consider it a bug fix then. 👍

ping @symfony/deciders

@vudaltsov

This comment has been minimized.

Show comment
Hide comment
@vudaltsov

vudaltsov Jul 13, 2017

Contributor

anything else needed here?

Contributor

vudaltsov commented Jul 13, 2017

anything else needed here?

@xabbuh

xabbuh approved these changes Jul 13, 2017

Show outdated Hide outdated 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) {

This comment has been minimized.

@fabpot

fabpot Jul 13, 2017

Member

could be merge with the previous if condition

@fabpot

fabpot Jul 13, 2017

Member

could be merge with the previous if condition

Show outdated Hide outdated 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])) {

This comment has been minimized.

@fabpot

fabpot Jul 13, 2017

Member

Same here, you can merge the 2 conditions.

@fabpot

fabpot Jul 13, 2017

Member

Same here, you can merge the 2 conditions.

Show outdated Hide outdated src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php
Type::BUILTIN_TYPE_OBJECT,
false,
$metadata->embeddedClasses[$property]['class']
));

This comment has been minimized.

@fabpot

fabpot Jul 13, 2017

Member

could be on one line

@fabpot

fabpot Jul 13, 2017

Member

could be on one line

Show outdated Hide outdated 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) {

This comment has been minimized.

@dunglas

dunglas Jul 22, 2017

Member

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

@dunglas

dunglas Jul 22, 2017

Member

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

This comment has been minimized.

@vudaltsov

vudaltsov Jul 22, 2017

Contributor

@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.

@vudaltsov

vudaltsov Jul 22, 2017

Contributor

@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.

This comment has been minimized.

@dunglas

dunglas Jul 22, 2017

Member

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

@dunglas

dunglas Jul 22, 2017

Member

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

This comment has been minimized.

@vudaltsov

vudaltsov Jul 22, 2017

Contributor

@dunglas, done!

@vudaltsov

vudaltsov Jul 22, 2017

Contributor

@dunglas, done!

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Jul 22, 2017

Member

Thank you @vudaltsov.

Member

dunglas commented Jul 22, 2017

Thank you @vudaltsov.

dunglas added a commit that referenced this pull request Jul 22, 2017

bug #23023 [DoctrineBridge][PropertyInfo] Added support for Doctrine …
…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

@dunglas dunglas closed this Jul 22, 2017

@vudaltsov vudaltsov deleted the vudaltsov:2.8 branch Jul 31, 2017

This was referenced Aug 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment