Fix #19943 Make sure to process each interface metadata only once #20078
Conversation
|
The label "BC break" should be removed. With some tests it would be 👍 , thanks! |
c23bc3b to
a696f42
Compare
|
@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 |
| @@ -19,7 +19,7 @@ | |||
| * @Assert\GroupSequence({"Foo", "Entity"}) | |||
| * @Assert\Callback({"Symfony\Component\Validator\Tests\Fixtures\CallbackClass", "callback"}) | |||
| */ | |||
| class Entity extends EntityParent | |||
| class Entity extends EntityParent implements EntityInterface, EntityParentInterface | |||
There was a problem hiding this comment.
I think you should add another interface like OtherEntityInterface (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.
Or just add the EntityParentInterface to the EntityParent instead :)
There was a problem hiding this comment.
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 the EntityParent 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.
If I added EntityParentInterface to EntityParent would not have solved the issue because I had added the EntityInterface to it too. I think the new version is better.
a696f42 to
422ca4e
Compare
|
@HeahDude I updated the tests to take into account your comments. Thanks for the feedback. The interfaces are now named |
| @@ -20,15 +20,17 @@ class LazyLoadingMetadataFactoryTest extends \PHPUnit_Framework_TestCase | |||
| { | |||
| const CLASSNAME = 'Symfony\Component\Validator\Tests\Fixtures\Entity'; | |||
| const PARENTCLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityParent'; | |||
| const INTERFACECLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityInterface'; | |||
| const INTERFACEACLASS = 'Symfony\Component\Validator\Tests\Fixtures\EntityInterfaceA'; | |||
There was a problem hiding this comment.
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.
Yup, good point. Here you go.
422ca4e to
a59f2a5
Compare
a59f2a5 to
2f9b65a
Compare
|
LGTM 👍, thanks! Status: Reviewed Failures unrelated and should now be fixed by #20079. |
|
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/A Here 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 loaded 2. `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 reappear Commits ------- 2f9b65a Fix #19943 Make sure to process each interface metadata only once
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: