[RFC][Validator][DoctrineBridge] Allow UniqueEntity to be aware of the class it is declared on #16969
Comments
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Dec 11, 2015
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
This was referenced Dec 11, 2015
|
As I said in #15002, I find your solution elegant and well-thought. But I'm still not convinced by the application for other constraints than |
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Dec 12, 2015
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Dec 16, 2015
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Feb 2, 2016
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Feb 2, 2016
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Feb 2, 2016
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Feb 2, 2016
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
lemoinem
added a commit
to lemoinem/symfony
that referenced
this issue
Feb 3, 2016
Fixes symfony#16969 Fixes symfony#4087 Fixes symfony#12573 Incompatible with symfony#15002 Incompatible with symfony#12977
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is an alternative fix to #4087 (and therefore impacts #15002, #12573 and #12977 too).
I first posted it as a comment on #15002 . But I think it would be easier to think about it.
Plus, since the design will probably requires several PRs, I was thinking of using this issue to centralize them.
The basic idea is to add a new interface (and its implementation as a Trait) to allow class Constraints to be aware of the class on which they have been declared. This would then be used to make
UniqueEntityaware of the class it's been declared on and use this in its Validator to pull the right Repository.Following here, I provided a detailed analysis of how the design and the fix would impact the current behavior.
Target aware constraints
I thought we could create a new
TargetAwareInterface(inSymfony\Component\Validator). The interface would actually be empty since Constraints options are handled through public properties, and a Trait to implement it easily.The setter would be called automatically by the Constraint Loaders (
if ($constraint instanceof TAI) { $constraint->target = $className; }). This means minor adjustments to theXmlLoader,YamlLoaderandAnnotationLoader. Developers wouldn't need to specify it themselves.Discussion
I think this would provide a completely generic approach (any constraint implementing the Interface could get access to its target). This is moreover seamless for the user.
Backward Compatibiliy
As a transitory method, in case the loader isn't patched yet (or if the
StaticMethodLoaderis used and the target is not provided). The Validator could fallback to the current behavior and trigger a deprecation notice.Flexibility
Discussion WRT #15002
This design is a bit less flexible than the design of #15002. But, IMHO the additional flexibility provided by the first PR get out of the semantic scope of
UniqueEntity:root,current,realwith the first solution, is fixed in mine (equivalent tocurrent).Awhile using a Repository for a classB.currentvsrealBehaviorsIf there are no class inheritance, there is no difference between
currentandreal.If there is any kind of class inheritance,
realdoesn’t actually work (which is the source of the original bug).Separation between Validated Entities and Target Repository
This covers both an explicit
repositoryor therootbehavior, which is just a special case of it.Given the
Person,Student,Teacherclass hierarchy example,I see three possibilities when the constraint is applied on
Awhile usingB's repository:Bis outside the inheritance line (e.g. Sister/Cousin class) ofA(e.g., Constraint is on classStudent, butrepositoryisTeacher)Person, butrepositoryisStudent)Student, butrepositoryisPerson, includes therootbehavior)IMO, these are not deep issues, If you feel like a long(er) read, here is a detailed analysis of each case, otherwise, you might want to skip the next couple of paragraphs...
(TL;DR: it falls outside the scope of
UniqueEntity’s semantic and should be addressed with a separate custom Constraint)In the first case (B is outside the inheritance line of A), it would mean that Students cannot have names used by Teachers, but the other way around wouldn’t be an issue. Additional care is required to ensure Students already existing do not conflict with a new Teacher name. This is a blacklisting semantic. While useful, we could argue this semantic isn’t covered by
UniqueEntityand should be implemented using a custom Constraint.In the second case (B is a child class of A), it would mean that Persons cannot have names already used by Students, but names used by Teachers would be OK. This is very similar to the previous case and seems to carry the same Blacklisting semantic. Therefore, it falls outside of
UniqueEntity’s semantic and requires a custom Constraint.In the third and last case (B is a child class of A), it would mean that Teachers can have duplicated names, but Students can’t have duplicated names, nor the same name as Teachers. Although the first part of Students’ restrictions seems to be matching the
UniqueEntity’s semantic, the second part matches the Blacklisting semantic. So, IMO, it should be a combination of both.Multiple Table inheritance
In the case of Multiple Table inheritance, there are a few cases where the current implementation may not crash, since the UNIQUE Index may prove impossible to generate (For example, if the field is represented differently in some subclasses).
The current implementation would simply fail to enforce the constraint. It's therefore another bug. Plus the semantic is ambiguous.
If what we need is a per-table
UniqueEntitybehavior (similar to what separate UNIQUE Indices on separate tables would enforce), this should be enforced by using separateUniqueEntityConstraints on each class representing each table.If what we need is an actual cross-table
UniqueEntityConstraint, using the Repository of the Root Entity should solve this problem. There might be some restrictions in the Repository and Doctrine themselves that makes it impractical. However, if that is the case, it's a different issue which should be solved by a different PR with a stricter scope and a more complex implementation. It may require a rather extensive MetaData analysis.Conclusion
Thanks for taking the time to read all that if you did. It’s rather long and a bit tedious if I may say so myself.
The design is implemented through two separated PRs:
TargetAwareInterfacefor class constraints ([Validator] Target aware class constraints #16978)UniqueEntityconstraint ([WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance #16981) with the fixed behaviorThe text was updated successfully, but these errors were encountered: