8000 Validation Constraint Inheritance Broken · Issue #21567 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Validation Constraint Inheritance Broken #21567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
scheb opened this issue Feb 8, 2017 · 9 comments
Closed

Validation Constraint Inheritance Broken #21567

scheb opened this issue Feb 8, 2017 · 9 comments
Labels
Bug Status: Needs Review Validator

Comments

@scheb
Copy link
Contributor
scheb commented Feb 8, 2017 •
edited
Loading
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
Copy link
Member

ping @uwej711

@scheb
Copy link
Contributor Author
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
Copy link
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
Copy link
Contributor Author
scheb commented Feb 8, 2017 •
edited
Loading

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

@xabbuh
Copy link
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
Copy link
Contributor
ro0NL commented Feb 9, 2017 •
edited
Loading

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
Copy link
Contributor Author
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.

@xabbuh xabbuh mentioned this issue Feb 12, 2017
Merged
@ro0NL
Copy link
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 added a commit that referenced this issue Feb 13, 2017
@fabpot
bug #21592 [Validator] property constraints can be added in child cla…
5c07ffa 
…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
@fabpot fabpot closed this as completed Feb 13, 2017
@xabbuh xabbuh mentioned this issue Feb 14, 2017
Closed
@przemyslaw-bogusz
Copy link
Contributor

I have proposed @Assert\EnableOverridingPropertyConstraints() in #36155, which allows you to choose, if the constraints should be merged or overriden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @nicolas-grekas @ro0NL @scheb @xabbuh @carsonbot @przemyslaw-bogusz
0