NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

vudaltsov wants to merge7 commits intosymfony:2.8fromvudaltsov:2.8

Closed

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

vudaltsov wants to merge7 commits intosymfony:2.8fromvudaltsov:2.8

Conversation

Copy link

Contributor

vudaltsov commentedJun 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 thatEmbeddables appeared only in doctrine/orm 2.5. I added class_exists checks for that.

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

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

		return;
		}

		$expectedTypes = [newType(

Copy link

Member

nicolas-grekasJun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

please use array() instead of []

Copy link

ContributorAuthor

vudaltsovJun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

nicolas-grekas added this to the2.8 milestone

Jun 1, 2017

Fixed array declaration

e1deefd

Copy link

ContributorAuthor

vudaltsov commentedJun 1, 2017•
edited
Loading

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',];

FixedDoctrineExtractor::getProperties() in the next commit.

Fixed getProperties() for embeddables

2420190

stloyd suggested changes

Jun 2, 2017

View reviewed changes

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

		publicfunctiontestGetEmbeddableProperties()
		{
		if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
		return;

Copy link

Contributor

stloydJun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

vudaltsovJun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you, fixed this!

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

		publicfunctiontestExtractEmbeddable()
		{
		if (!class_exists('Doctrine\ORM\Mapping\Embedded')) {
		return;

Copy link

Contributor

stloydJun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same here.

Corrected test methods names and added markTestSkipped()

d572d53

stloyd approved these changes

Jun 2, 2017

View reviewed changes

Copy link

ContributorAuthor

vudaltsov commentedJun 11, 2017

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

Copy link

Member

dunglas commentedJun 14, 2017

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

dunglas requested changes

Jun 14, 2017

View reviewed changes

Copy link

Member

dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice improvement! I just left some minor comments.

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

		$properties =array_merge($metadata->getFieldNames(),$metadata->getAssociationNames());

		if (class_exists('Doctrine\ORM\Mapping\Embedded')) {
		if ($metadatainstanceof ClassMetadataInfo &&count($metadata->embeddedClasses) >0) {

Copy link

Member

dunglasJun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

&& $metadata->embeddedClasses will be faster.

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

		returnarray_merge($metadata->getFieldNames(),$metadata->getAssociationNames());
		$properties =array_merge($metadata->getFieldNames(),$metadata->getAssociationNames());

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

Copy link

Member

dunglasJun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		));
		}

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

Copy link

Member

dunglasJun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

same here, same in tests.

Copy link

ContributorAuthor

vudaltsov commentedJun 14, 2017•
edited
Loading

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

Copy link

Member

dunglas commentedJun 29, 2017

Ok to consider it a bug fix then. 👍

ping @symfony/deciders

dunglas approved these changes

Jun 29, 2017

View reviewed changes

Improved ClassMetadataInfo::$embeddedClasses nonempty array check

fff8330

Copy link

ContributorAuthor

vudaltsov commentedJul 13, 2017•
edited
Loading

anything else needed here?

xabbuh approved these changes

Jul 13, 2017

View reviewed changes

fabpot reviewed

Jul 13, 2017

View reviewed changes

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

		$properties =array_merge($metadata->getFieldNames(),$metadata->getAssociationNames());

		if (class_exists('Doctrine\ORM\Mapping\Embedded')) {
		if ($metadatainstanceof ClassMetadataInfo &&$metadata->embeddedClasses) {

Copy link

Member

fabpotJul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

could be merge with the previousif condition

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

		}

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

Copy link

Member

fabpotJul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same here, you can merge the 2 conditions.

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

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

Copy link

Member

fabpotJul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

could be on one line

Codestyle fix and if optimizations

42ffb14

dunglas reviewed

Jul 22, 2017

View reviewed changes

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

		returnarray_merge($metadata->getFieldNames(),$metadata->getAssociationNames());
		$properties =array_merge($metadata->getFieldNames(),$metadata->getAssociationNames());

		if (class_exists('Doctrine\ORM\Mapping\Embedded') &&$metadatainstanceof ClassMetadataInfo &&$metadata->embeddedClasses) {

Copy link

Member

dunglasJul 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

vudaltsovJul 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

dunglasJul 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

vudaltsovJul 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@dunglas, done!

Performance improvements

268f545

Copy link

Member

dunglas commentedJul 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 …

7428aba

…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 closed this

Jul 22, 2017