[Validator] Don't add multiple MemberMetadata for the same property and the same class when merging constraints.

[fancyweb]

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22538
License	MIT
Doc PR	-

Hello, before reading my comments, you should look at the fixed ticket for more context.

So, given the following classes (taken from the example) :

interface PriceInterface {}
interface VariantPriceInterface extends PriceInterface {}
abstract class AbstractPrice implements PriceInterface {}
class VariantSamplePrice extends AbstractPrice implements VariantPriceInterface {}

and validation file :

PriceInterface:
    getters:
        value:
            - NotBlank: ~

To begin, I tried to improve the work done by @lemoinem to fix the previous issue of double validation I reported (cf #19943). I tried to make it work with the new case I have found but I couldn't. IMO, it is impossible without changing some public methods signatures. I will describe the problem with the example classes.

The problem is that both class AbstractPrice and interface VariantPriceInterface implements the PriceInterface interface but when you get their metadata, you get them independently. I mean there is no "context" that indicates that in reality, we want the metadata for the VariantSamplePrice model. In other words, both classes don't know about each other, they cannot know that they both implement the same interface, and have no way of knowing if the constraints for the PriceInterface have already been added or no.

So I had a different approach, and thought that I could "clean" the duplicated constraints after they were all added. There are several ways to do this, I chose to make the check as soon as possible, when the constraints are merged, so the metadata are in an "invalid" state the shortest time. The check is simple : don't merge MemberMetadata constraints if there are already defined for the same property and the same class.

To illustrate with my example : at a given time, the VariantSamplePrice metadata will already have a GetterMetadata for the property value of the class PriceInterface either coming from the AbstractPrice or the VariantPriceInterface metadata. Then, the PriceInterface metadata will be merged a second time in the VariantSamplePrice metadata (coming from the other class metadata). But with the check, the constraints won't be merged another time. Because a MemberMetadata for the same property and of the same class exists so it means that the exact same constraints have already been added.

My main concern is the choice of which MemberMetadata should be kept as there are not exactly the same : depending of of its class, the groups in the constraints are not the same. I honestly don't know if this is a problem or not.

Also, I originally deleted @lemoinem work because it was not really needed anymore and it covers less cases. But actually, as long as there are only interfaces concerned, it works better because the metadata are never "invalid". But I don't know if it's logical to keep both checks in different files, I need advice on this.

[dmaicher]

But the two constraints are different? So this should be 2 here imho.

[ro0NL]

it's just a test :) the behavior is actually correct imo. Skip constraints from the same class/property (i.e. the ones we already visited). in real life these are the same ;-)

[fancyweb]

Indeed, the constraint doesn't matter. What matters in this test is that it's the same metadata class (self::INTERFACE_A_CLASS) and the same getter "property" (data). However, I agree that the constraint should maybe be the same so it's easier to understand.

[ro0NL]

i actually liked you used different constraints :P it shows we're testing behavior

[dmaicher]

Not sure I would agree that they are always "the same" in "real-life" 😉 Seems like a BC break to me.

Imho we would have to check if the constraints are actually 100% the same, so same class & same options. Only then we have real duplicates.

[ro0NL]

maybe you're right.. we cannot foresee the possible things people are doing. I dont like the idea of a deep comparison nightmare; see #15950 which we need to consider as well.

I think we actually need to prevent the wrong call to mergeConstraint based on the above inheritance.

[dmaicher]

One different approach that I'm using in one of my apps is that I simply only render "unique" error messages as this simply makes the most sense to the end-user.

Can we maybe go into that direction? So even if constraints are duplicated we move the de- duplication to the validation phase?

Just an idea 😋

[ro0NL]

clever.. :) but still imo. this needs a fix low-level. Like done in #20078; filtering out duplicated interfaces. That needs to cover above usecase.

[ro0NL]

@fancyweb i played with it yesterday, and you're absolutely right. The approach is hard... (i re-read you issue once more) and you encountered everything already :P

However, i think i got something (forgot to push, but i can show you later). My idea was to track visited classes, and simply skip ones we already visited. It seems to work, in terms of i get the same no. of constraints. However some constraints have different groups (BC break), which probably means we need to add implicit groups for the classes we skipped (not sure, here's where i stopped :))

The failing test would be

diff --git a/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php b/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php
index 4674f8b35a..8aaf21b719 100644
--- a/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php
+++ b/src/Symfony/Component/Validator/Tests/Fixtures/EntityParent.php
@@ -13,7 +13,7 @@ namespace Symfony\Component\Validator\Tests\Fixtures;
 
 use Symfony\Component\Validator\Constraints\NotNull;
 
-class EntityParent implements EntityInterfaceA
+class EntityParent implements EntityInterfaceA, EntityParentInterface
 {
     protected $firstName;
     private $internal;

[nicolas-grekas]

ping @fancyweb

[fancyweb]

We can close this as my solution is more a hack than a fix and it breaks BC. I'm working on a new solution atm and will open a new PR...