Validation Constraint Inheritance Broken #21567

Closed
scheb opened this Issue Feb 8, 2017 · 8 comments

Projects

None yet

6 participants

@scheb
scheb commented Feb 8, 2017 edited
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version >=3.1.9

Symfony validator does no longer inherit all constraints from the parent class. It seems like all the constraints from super-classes are lost when there are additional constraints on the same attribute in the sub-class. I was able to track it down to this commit, if I roll-back the changes it's working again. But couldn't wrap my head around yes, why this is happening.

Test script to reproduce the bug:

<?xml version="1.0" encoding="UTF-8"?>
<constraint-mapping xmlns="http://symfony.com/schema/dic/constraint-mapping" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/constraint-mapping http://symfony.com/schema/dic/constraint-mapping/constraint-mapping-1.0.xsd">
    <class name="Foo">
        <property name="attribute">
            <constraint name="NotNull">
                <option name="message">NOT_NULL</option>
            </constraint>
        </property>
    </class>
    <class name="Bar">
        <property name="attribute">
            <constraint name="Type">
                <option name="type">string</option>
                <option name="message">NOSTRING</option>
            </constraint>
        </property>
    </class>
</constraint-mapping>
<?php
include(__DIR__ . '/vendor/autoload.php');

use Symfony\Component\Validator\Validation;

abstract class Foo {
    public $attribute;
}

class Bar extends Foo {
}

$validator = Validation::createValidatorBuilder()->addXmlMapping(__DIR__ . '/validation.xml')->getValidator();

$test1 = new Bar();
$test1->attribute = null; // NotNull violation
var_dump($validator->validate($test1)->count()); // Should be 1, but is 0 in version >= 3.1.9

$test2 = new Bar();
$test2->attribute = 42; // Type violation
var_dump($validator->validate($test2)->count()); // Should be 1
@nicolas-grekas
Member

ping @uwej711

@scheb
scheb commented Feb 8, 2017

Just noticed, moving this piece of code

// Include constraints from the parent class
$this->mergeConstraints($metadata);

from line 123 to line 115 right after the $metadata = new ClassMetadata($class); fixes it. That's the place where the logic was before the commit that I've mentioned.

@xabbuh
Member
xabbuh commented Feb 8, 2017

I think this is caused by the changes made in #21053 (see also #21538). Can you confirm that?

@scheb
scheb commented Feb 8, 2017 edited

Rolling back the changes from #21053 fixes it for me.

@xabbuh
Member
xabbuh commented Feb 8, 2017

So it seems we have conflicting requirements here and in #15950 which are more or less mutually exclusive. I am actually not sure how to solve this sufficiently.

Maybe we can mitigate #15950 by modifying the patch from #21053 partially by skipping the merge only when constraints being used are the same (regardless their options)?

What do you think @symfony/deciders?

@ro0NL
Contributor
ro0NL commented Feb 9, 2017 edited

Imo. a sub class can only add additional constraints, at least by default that is. You're extending by definition (class Sub extends Base).

That said.. what about @Assert\Override()? Or make a per-constraint decision, but im not sure it's sufficient enough.

@scheb
scheb commented Feb 12, 2017

Agree with @ro0NL that by default constraints should extend constraints from super-classes instead of overriding them.

In my opinion, it should only override property constraints, when the related property has been overwritten in the sub-class. So the decision, which constraints should be applied, can only be made if we take into account the class hierarchy and how the properties of these classes are defined in addition to the constraint metadata.

To make clear what I mean, let's say we have constraints defined on both Foo::$attribute and Bar::$attribute.

"Extends" case:

class Foo {
    public $attribute;
}

class Bar extends Foo {
}
  • When validating a Bar object, $attribute constraints from both Foo and Bar should be applied.
  • When validating a Foo object, $attribute constraints only from Foo should be applied.

"Override" case:

class Foo {
    public $attribute;
}

class Bar extends Foo {
    public $attribute;
}
  • When validating a Bar object, $attribute constraints only from Bar should be applied, because Bar overrides the attribute and therefore also the constraints from Bar should completely override the ones from Foo.
  • When validating a Foo object, $attribute constraints only from Foo would be applied.

Overall it looks to me that the MetadataFactory needs to become aware of how the classes look like, it cannot just merge constraint metadata the one or the other way.

@ro0NL
Contributor
ro0NL commented Feb 13, 2017

@scheb that's a way of looking at it :) but im not sure it's convenient to mix&match the PHP with the XML definition or so. Ie. adding the attribute in your "override" example changes the behavior of the XML definition.. that's weird.

For annotations it could work, as you need to define the property anyway to annotate it with constraints.. but imo. it's highly implicit behavior (ie. magic).

Basically we need a fix for #15950 only.

@fabpot fabpot added a commit that referenced this issue Feb 13, 2017
@fabpot fabpot bug #21592 [Validator] property constraints can be added in child cla…
…sses (angelk, xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[Validator] property constraints can be added in child classes

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

This reverts the changes done in #21053 (and applies the test written by @angelk in #21538). I think trying to "fix" #15950 with the changes from #21053 was a mistake. Child classes should be able to refine validation constraints (I basically agree with @ro0NL's reasoning in #21567 (comment).

Commits
-------

9513a8a property constraints can be added in child classes
a266ff7 added test for staticClassLoader in LazyLoadingMetadatafactory
5c07ffa
@fabpot fabpot closed this Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment