[WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance #16981

lemoinem · 2015-12-11T23:51:42Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes, only if `StaticMethodLoader` or a custom Constraint Loader is used to load `UniqueEntity` constraints
Tests pass?	yes
Fixed tickets	#16969, #4087, #12573
Dependens on	#16978
License	MIT
Doc PR	N/A

This is the second half of #16969 with the actual fix.

This version of the code is ready for code review. However, since I'm dependent on #16978, I'm leaving the PR as a WIP until the dependence is merged and I rebase the current one on master.

Since I introduce deprecations, I updated UPGRADE-4.0.md.

~~Manage the case where @UniqueEntity is attached to a non-Entity class~~

ogizanagi · 2015-12-12T00:12:44Z

UPGRADE-4.0.md

Isn't is possible to iterate over $metadata->getConstraints() in the StaticMethodLoader in order to avoid this ?

foreach ($metadata->getConstraints() as $constraint) { if ($constraint instanceof TargetAwareConstraintInterface) { $constraint->target = $metadata->getClassName(); } }

It could be, however the current behavior of StaticMethodLoader is to do the bare minimum and to let the implementation of the loader method do all the heavy lifting work.

That's why I didn't implement the support for TA constraints in there. But, yeah, it could be done, I guess... I will add a note in the other PR.

PS: The use case would still be possible (and the deprecation still required) for custom constraint loaders, just thought about that...

ogizanagi · 2015-12-12T00:16:38Z

Won't it be a BC break as this behavior is the current one described in other PRs, whereas the existing behavior until now matches the real one ?

lemoinem · 2015-12-12T00:28:34Z

Well, the way I see it, it would be a BC break if the previously correct behavior was changed. But since the current behavior is a bug, it is erroneous, so fixing it wouldn't be a BC break.

ogizanagi · 2015-12-15T21:21:47Z

@lemoinem : I've thought about edges cases with this proposal: What if the UniqueEntityConstraint is declared on a parent class, which is not an entity, and thus, does not have a repository ?
This is the case of the FOSUserBundle for instance.

ogizanagi · 2015-12-15T21:22:34Z

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

- @trigger_error('Not providing a target for a UniqueEntity constraint is deprecated since version 3.1 and will be removed in 3.0.', E_USER_DEPRECATED); + @trigger_error('Not providing a target for a UniqueEntity constraint is deprecated since version 3.1 and will be removed in 4.0.', E_USER_DEPRECATED);

(4.0, not 3.0)

Good Catch. Thank you!

lemoinem · 2015-12-16T22:38:08Z

Indeed, that's a very good point... I didn't consider it because having a @UniqueEntity on a non-Entity class seems to be a non-sense to me... BTW, what you describe seems to be FOSUB 1 behavior only, FOSUB 2 defines FOS\UserBundle\Model\User as its entity and it's also the class the entity mapping is defined for.

However, that would definitely be a BC Break...

I'm thinking the best way would be to grab the highest parent of the current entity, among the descendants of the target that IS an Entity and use ITS Repository.

By this, I mean, given these classes:

Root (@Entity)
| - Base (@UniqueEntity)
    | - BaseEntity (@Entity)
    |   | - ChildEntity (@Entity)
    |       | - MyEntity (@Entity)
    | - EntityB (@Entity)
    | - EntityC (@Entity)

calling the validator on an instance of MyEntity would use the Repository of BaseEntity. This is consistent with the strategy of using the highest possible Repository with respect to the target.

Using the Repository of Root could enforce the constraint against values that aren't supposed to be included (i.e. matching against Entities that aren't instances of BaseEntity).

Trying to compute the exact match (i.e. trying to lookup in the Repositories of BaseEntity, EntityB and EntityC), while being semantically correct (we match against ALL the entities instance of Base) seems to be as being highly unreliable since it will depend on which classes have a mapping loaded at the time the Constraint is validated. This could be far from complete and volatile in the case of Annotations Mappings.

ogizanagi · 2015-12-16T23:22:03Z

FOSUB 2 defines FOS\UserBundle\Model\User as its entity and it's also the class the entity mapping is defined for.

I don't see what you mean. The file I linked above is from the master branch and so is FOSUser 2. :/
The mapping is, indeed, on the FOS\UserBundle\Model\User, as well as the validation constraints, but this class is still abstract and you'll need to extend it. So when calling the validator on your own user, the target will be FOS\UserBundle\Model\User, but this class isn't an entity (just a mapped superclass) and doesn't have a repository.

Your new suggestion seems good to me however.

lemoinem · 2015-12-17T14:22:04Z

I was referring to FriendsOfSymfony/FOSUserBundle@047b0e6 which deleted the Entity classes.

Since this commit, in master, the Doctrine mapping is now applied on FOS\UserBundle\Model\User instead of FOS\UserBundle\Entity\User previously. However I just saw it's not an Entity mapping but a Mapped Superclass mapping. So you were right, my bad.

However, having a UniqueEntity constraint on a Mapped Superclass makes more sense than on a class without any kind of mapping. I'm guessing we could deprecate applying UniqueEntity on a class without mapping. I'm thinking it would be a separate issue, and I'm not sure either whether that could help with the process of discovering the right Repository to use. I'll check during the implementation if it does.

Thank you for the feedback!

lemoinem · 2015-12-17T19:29:52Z

After testing (https://github.com/lemoinem/symfony-standard/tree/test/doctrine/mapped-superclass-repository, composer install, Setup DB, bin/console doc:sc:up --force, Insert various Entities (discr may be either "root" or "entity", bin/console test), calling a MappedSuperclass' Repository is perfectly fine, as long as there is a parent entity and the fields the query is based on are defined and mapped and such parent entity. Doctrine documentation does specify that MSC aren't supposed to be query-able.

This doesn't change much to the implementation I intended to put in place.

lemoinem · 2016-02-02T22:21:43Z

OK, I finally had time to write a relevant test case for the repository search.
I used the "highest repository" strategy we discussed. If you have any other feedback, I'd be happy to hear it!

PS: I also took the chance to rebase both PRs. The PHP 5.6 Travis build currently fails because of FrameworkBundle errors which aren't related to this PR.

ogizanagi · 2016-02-03T07:07:53Z

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

typo

- findHighestEntityMetada + findHighestEntityMetadata

Yup, indeed!
Thanks!

Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977

carsonbot added the Status: Needs Review label Dec 11, 2015

This was referenced Dec 11, 2015

[RFC][Validator][DoctrineBridge] Allow UniqueEntity to be aware of the class it is declared on #16969

Closed

[DoctrineBridge] Add a way to select the repository used by the UniqueEntity validator #15002

Merged

ogizanagi reviewed Dec 12, 2015
View reviewed changes

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from a610e22 to e0ff5d5 Compare December 12, 2015 00:26

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from e0ff5d5 to 2d9946c Compare December 12, 2015 00:36

lemoinem mentioned this pull request Dec 12, 2015

[Validator] Target aware class constraints #16978

Closed

ogizanagi reviewed Dec 15, 2015
View reviewed changes

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from 2d9946c to 36f10c9 Compare December 16, 2015 21:49

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from 36f10c9 to 7937237 Compare February 2, 2016 21:52

lemoinem added 6 commits February 2, 2016 16:55

[Validator] Target aware constraint's interface and trait

ef2bad6

[Validator] Target aware constraint test fixture

9427089

[Validator] Target aware constraint support for YamlFileLoader

42dc2d2

[Validator] Target aware constraint support for XmlFileLoader

04d3bdc

[Validator] Target aware constraint support for AnnotationLoader

c02329d

Update CHANGELOG

697833b

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from 7937237 to 54fc801 Compare February 2, 2016 21:59

Add Mock MetadataFactory in UniqueEntityValidatorTest

dba386e

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch 2 times, most recently from 35d86d9 to 2bb4810 Compare February 2, 2016 22:14

lemoinem changed the title ~~[WIP][WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance~~ [WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance Feb 2, 2016

ogizanagi reviewed Feb 3, 2016
View reviewed changes

lemoinem added 2 commits February 3, 2016 10:14

[DoctrineBridge] Fix UniqueEntity interaction with inheritance

6c4b3ae

Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977

[DoctrineBridge][Validator] CHANGELOG & UPGRADE

3868657

lemoinem force-pushed the fix/doctrine-bridge/unique-entity-inheritance branch from 2bb4810 to 3868657 Compare February 3, 2016 15:14

lemoinem changed the title ~~[WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance~~ [WIP][WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance Feb 3, 2016

lemoinem changed the title ~~[WIP][WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance~~ [WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance Feb 3, 2016

ogizanagi mentioned this pull request Mar 6, 2016

UniqueEntityValidator in SingleTableInheritance #17962

Closed

javiereguiluz added Bug Deprecation DoctrineBridge labels Jun 20, 2016

fabpot closed this Oct 14, 2016

Uh oh!

[WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance #16981

[WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance #16981

Uh oh!

Conversation

lemoinem commented Dec 11, 2015

Uh oh!

ogizanagi Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

lemoinem Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

lemoinem Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

ogizanagi commented Dec 12, 2015

Uh oh!

lemoinem commented Dec 12, 2015

Uh oh!

ogizanagi commented Dec 15, 2015

Uh oh!

ogizanagi Dec 15, 2015

Choose a reason for hiding this comment

Uh oh!

lemoinem Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

lemoinem commented Dec 16, 2015

Uh oh!

ogizanagi commented Dec 16, 2015

Uh oh!

lemoinem commented Dec 17, 2015

Uh oh!

lemoinem commented Dec 17, 2015

Uh oh!

lemoinem commented Feb 2, 2016

Uh oh!

ogizanagi Feb 3, 2016

Choose a reason for hiding this comment

Uh oh!

lemoinem Feb 3, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants