Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix #19943 Make sure to process each interface metadata only once#20078
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix #19943 Make sure to process each interface metadata only once#20078
Uh oh!
There was an error while loading.Please reload this page.
Conversation
HeahDude commentedSep 28, 2016
The label "BC break" should be removed. With some tests it would be 👍 , thanks! |
c23bc3b toa696f42Comparelemoinem commentedSep 28, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@HeahDude Yes, the label has been added by mistake, I made an error with the PR template... And I can't seem to be able to remove it ;). Here are the tests, I had to add an Interface so the use case would be adequately represented. I double checked, the tests fail without the patch. I think I'm good for the full review. The failed tests on appveyor and travis concern the Twig Bridge and Bundle or the SecurityBundle. It's unrelated to this PR. Status: Needs Review |
| * @Assert\Callback({"Symfony\Component\Validator\Tests\Fixtures\CallbackClass", "callback"}) | ||
| */ | ||
| class Entityextends EntityParent | ||
| class Entityextends EntityParentimplements EntityInterface, EntityParentInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you should add another interface likeOtherEntityInterface (I don't really like the name but you got the point), because implementing both interfaces when one extends another does not make sense, this PR is not trying to fix that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or just add theEntityParentInterface to theEntityParent instead :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes... I guess I see your point. Although the fix is going to take care of this case too.
I also added the interface on theEntityParent this case (interface implemented through class inheritance) was already taken care of in the code before my patch, but not in the test. I'll add (yet) another interface to take care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I addedEntityParentInterface toEntityParent would not have solved the issue because I had added theEntityInterface to it too. I think the new version is better.
a696f42 to422ca4eComparelemoinem commentedSep 28, 2016
@HeahDude I updated the tests to take into account your comments. Thanks for the feedback. The interfaces are now named |
| constCLASSNAME ='Symfony\Component\Validator\Tests\Fixtures\Entity'; | ||
| constPARENTCLASS ='Symfony\Component\Validator\Tests\Fixtures\EntityParent'; | ||
| constINTERFACECLASS ='Symfony\Component\Validator\Tests\Fixtures\EntityInterface'; | ||
| constINTERFACEACLASS ='Symfony\Component\Validator\Tests\Fixtures\EntityInterfaceA'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you please add some underscores to constants names to ease readability, I know you did it for consistency because there weren't any but since you add some more, and longer ones with single-lettered meanings... I think maybe we could start using them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yup, good point. Here you go.
422ca4e toa59f2a5Comparea59f2a5 to2f9b65aCompareHeahDude commentedSep 29, 2016 • edited by javiereguiluz
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by javiereguiluz
Uh oh!
There was an error while loading.Please reload this page.
LGTM 👍, thanks! Status: Reviewed Failures unrelated and should now be fixed by#20079. |
fabpot commentedSep 30, 2016
Thank you@lemoinem. |
…ly once (lemoinem)This PR was merged into the 2.7 branch.Discussion----------Fix#19943 Make sure to process each interface metadata only once| Q | A| ------------- | ---| Branch? | 2.7+| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19943| License | MIT| Doc PR | N/AHere is the fix for#19943, If you have `InterfaceA <- InterfaceB <- Class` with a constraint on a method defined on `InterfaceA`, the constraint and its eventual violations are currently validated and reported twice.Copy from#19943 (comment):As far as I can see, the problem seems to arise in [`LazyLoadingMetadataFactory`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php#L117-L123):[`ReflectionClass::getInterfaces()`](http://php.net/manual/en/reflectionclass.getinterfaces.php) returns both interfaces implemented directly and through inheritance (either through another interface or through a parent class). In the end, the following process occurs:1. `PriceInterface` is parsed and its `NotBlank` constraint on `value` is loaded2. `VariablePriceInterface` is parsed and inherits `PriceInterface`'s constraints (which is OK).3. `ProductPrice` is parsed and inherits both `PriceInterface` and `VariablePriceInterface`'s constraints, which leads to a duplicated `NotBlank` constraint, one from each Interface.The Best Way ™️ would be to be able to extract the list of interfaces implemented by a class directly only. However, the process seems a bit intricate... I will start working on it and prepare a PR to that effect. However, if any of you has a better idea, I'm all hears...TODO:- [x] Regression tests to make sure the bug doesn't reappearCommits-------2f9b65aFix#19943 Make sure to process each interface metadata only once
Uh oh!
There was an error while loading.Please reload this page.
Here is the fix for#19943, If you have
InterfaceA <- InterfaceB <- Classwith a constraint on a method defined onInterfaceA, the constraint and its eventual violations are currently validated and reported twice.Copy from#19943 (comment):
As far as I can see, the problem seems to arise in
LazyLoadingMetadataFactory:ReflectionClass::getInterfaces()returns both interfaces implemented directly and through inheritance (either through another interface or through a parent class). In the end, the following process occurs:PriceInterfaceis parsed and itsNotBlankconstraint onvalueis loadedVariablePriceInterfaceis parsed and inheritsPriceInterface's constraints (which is OK).ProductPriceis parsed and inherits bothPriceInterfaceandVariablePriceInterface's constraints, which leads to a duplicatedNotBlankconstraint, one from each Interface.The Best Way ™️ would be to be able to extract the list of interfaces implemented by a class directly only. However, the process seems a bit intricate... I will start working on it and prepare a PR to that effect. However, if any of you has a better idea, I'm all hears...
TODO: