-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Description
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 UniqueEntity
aware 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
(in Symfony\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 the XmlLoader
, YamlLoader
and AnnotationLoader
. 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 StaticMethodLoader
is 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
:
- The behavior, which could be one of
root
,current
,real
with the first solution, is fixed in mine (equivalent tocurrent
). - My design doesn't allow to apply the constraint on a Class
A
while using a Repository for a classB
.
current
vs real
Behaviors
If there are no class inheritance, there is no difference between current
and real
.
If there is any kind of class inheritance, real
doesn’t actually work (which is the source of the original bug).
Separation between Validated Entities and Target Repository
This covers both an explicit repository
or the root
behavior, which is just a special case of it.
Given the Person
, Student
, Teacher
class hierarchy example,
I see three possibilities when the constraint is applied on A
while using B
's repository:
B
is outside the inheritance line (e.g. Sister/Cousin class) ofA
(e.g., Constraint is on classStudent
, butrepository
isTeacher
)- B is a Child class of A (e.g., Constraint is on class
Person
, butrepository
isStudent
) - B is a Parent class of A (e.g., Constraint is on class
Student
, butrepository
isPerson
, includes theroot
behavior)
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 UniqueEntity
and 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 UniqueEntity
behavior (similar to what separate UNIQUE Indices on separate tables would enforce), this should be enforced by using separate UniqueEntity
Constraints on each class representing each table.
If what we need is an actual cross-table UniqueEntity
Constraint, 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:
- The
TargetAwareInterface
for class constraints ([Validator] Target aware class constraints #16978) - The Target Aware
UniqueEntity
constraint ([WCM][DoctrineBridge] Fix UniqueEntity interaction with inheritance #16981) with the fixed behavior