-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator][GroupSequences] GroupSequences still execute next group if first fail #9939
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Issue confirmed and reproduced (https://github.com/jakzal/symfony-standard/compare/symfony:master...jakzal:issue9939). |
@lemoinem I see one problem with the If you change the implementation to check if public function isThree()
{
// ...
return $this->bar && 3 == $this->getBar()->myBar();
} You'll notice that validation is actually not run on The reason why your code fails is because the value needs to be passed to visitors in order to make a decision (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Mapping/ClassMetadata.php#L114), not because the validation is run. |
That said I think it might still be an issue, since I can imagine a scenario where you would like to avoid fetching the value (for example when it's expensive to calculate). |
Or in the case the method has a complex side-effect. But anyway, if it's not deemed as a bug or is deemed too complex to be fixed. I think documenting this would be a good thing. Because the current state of the documentation implies otherwise. |
Could you check whether #10287 fixes this issue? |
…mozart) This PR was merged into the 2.5-dev branch. Discussion ---------- [WIP][Validator] New NodeTraverser implementation | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | TODO | License | MIT | Doc PR | TODO This PR is ready for review. #### Todo - [x] Test extensively to avoid regressions - [x] Test more extensively - [x] Finish inline documentation - [x] Provide a layer to choose the desired API through ValidatorBuilder - [x] Provide a layer to choose the desired API through FrameworkBundle - [x] Update UPGRADE file - [x] Update CHANGELOG - [ ] Update user documentation #### Goal The goal of this PR is to be able to fix the following tickets: - [x] #6138 Simplify adding of constraint violations - [x] #7146 Support group sequences in Validator API - [x] #7432 Poorly implemented Visitor Pattern - [x] #8617 Control traversal on class level - [x] #9888 Improve support for collection validation (PR: #9988) The following tickets are probably fixed, but require testing first: - [ ] #8376 Using validation_group causes error message to display multiple times - [ ] #9939 GroupSequences still execute next group if first fail Of course, full backwards compatibility **must** be guaranteed. Other tickets I want to fix in follow-up PRs: * #3622 Constraints\Valid does not respect "groups" option * #4453 walk constraints by groups * #7700 Propagate implicit group names in constraints * #9051 Always ask value event if field isn't in the validating group * #10163 poor collection validation test coverage * #10221 TypeValidator does not enforce desired type when value is NULL * #10495 Class Valid Constraint can't be used on a Form Type #### In a nutshell The implementation removes the Visitor pattern, which was implemented badly. I tried fixing it via a NodeTraverser/NodeVisitor implementation, but performance degraded too much so I decided to remove the pattern altogether. A couple of new features and bug fixes are possible thanks to the new implementation. See below for details. #### PHP Versions PHP 5.3.8 and older does not allow to implement two different interfaces which both contain a method with the same name. This is used in the compatibility layer that supports both the old and the new API. For this reason, the compatibility layer is disabled on PHP < 5.3.9. Older PHP versions need to decide on the old API or the new API (without compatibility layer). #### Choosing the API Version The API version can be specified by one of `Validation::API_VERSION_2_4`, `Validation::API_VERSION_2_5` and `Validation::API_VERSION_2_5_BC` to `setApiVersion()` when building the validator: ```php // Old implementation $validator = Validation::createValidatorBuilder() ->setApiVersion(Validation::API_VERSION_2_4) ->getValidator(); // New implementation with BC API // Does not work on PHP < 5.3.9 $validator = Validation::createValidatorBuilder() ->setApiVersion(Validation::API_VERSION_2_5) ->getValidator(); // New implementation without BC API $validator = Validation::createValidatorBuilder() ->setApiVersion(Validation::API_VERSION_2_5_BC) ->getValidator(); ``` #### Features ##### Constraint validation as first-class citizen The new API merges `validateValue()` and `validate()`. The idea is that the validation of values against constraints should be as simple as possible. Object validation is a special case where an object is tested against the `Valid` constraint. A backwards compatibility layer is provided to use both `validate()` and `validateValue()` with the old signature. ```php // Validate against explicit constraints $violations = $validator->validate($firstName, array( new NotNull(), new Length(array('min' => 3)), )); // Validate against metadata $violations = $validator->validate($object); // Same, more expressive notation $violations = $validator->validate($object, new Valid()); // Validate each entry against its metadata $violations = $validator->validate($array); ``` ##### Aggregate violations It is now possible to call the methods of the validator multiple times and aggregate the violations to a common violation list. To do so, call `startContext()`, execute the calls and call `getViolations()` in the end to retrieve the violations: ```php $violations = $validator->startContext() ->validate($title, new NotNull()) ->validate($text, new NotNull()) ->validate($author->getName(), new NotNull()) ->getViolations() ``` Most of the time, you will want to specify a property path for each validation. Use the method `atPath()` for that: ```php $violations = $validator->startContext() ->atPath('title')->validate($title, new NotNull()) ->atPath('text')->validate($text, new NotNull()) ->atPath('author.name')->validate($author->getName(), new NotNull()) ->getViolations() ``` ##### Control the context of nested validations In Symfony <= 2.4, you can validate objects or constraints from within a constraint validator like this: ```php $this->context->validate($object); $this->context->validateValue($value, new Length(array('min' => 3))); ``` The validations will run and all violations will be added to the current context. This way, it is impossible though to validate something, inspect the result and then decide what kind of violations to add to the context. This is needed, for example, for the `Some` constraint (proposed in #9888), which should succeed if any of the validated values did *not* generate violations. For this reason, the new context API features a method `getValidator()`. This method returns the validator instance, you can use it to validate anything in a new context (as the validator always does): ```php $validator = $this->context->getValidator(); $violations = $validator->validate($object); if (count($violations) > 0) { $this->context->addViolation('The validation did not pass'); } ``` You can also explicitly start a new context: ```php $validator = $this->context->getValidator(); $violations = $validator->startContext() ->atPath('title')->validate($title, new NotNull()) ->atPath('text')->validate($text, new NotNull()) ->getViolations() ``` If you want to execute the validation in the current context, use the `inContext()` method of the validator instead: ```php // violations are added to $this->context $validator->inContext($this->context) ->atPath('title')->validate($title, new NotNull()) ->atPath('text')->validate($text, new NotNull()) ; ``` With this feature, #9888 (especially the PR for it: #9988) can be finished. ##### Custom group sequences (#7146) It is now possible to pass `GroupSequence` instances whenever you can pass a group to the validator. For example: ```php $violations = $validator->validate($object, new Valid(), new GroupSequence('Basic', 'Strict')); ``` Or in the context of the Form component: ```php $form = $this->createForm(new BlogType(), new Blog(), array( 'validation_groups' => new GroupSequence('Basic', 'Strict'), )); ``` ##### Constraint violation builders (#6138) The API for adding constraint violations was simplified: ```php $this->context->addViolation('message', array('param' => 'value')); // or $this->context->buildViolation('message') ->atPath('property') ->setParameter('param', 'value') ->setTranslationDomain('validation_strict') ->addViolation(); ``` ##### Control traversal at class level (#8617) Currently, it is possible whether to traverse a `Traversable` object or not in the `Valid` constraint: ```php /** * @Assert\Valid(traverse=true) */ private $tags = new TagList(); ``` (actually, `true` is the default) In this way, the validator will iterate the `TagList` instance and validate each of the contained objects. You can also set "traverse" to `false` to disable iteration. What if you want to specify, that `TagList` instances should always (or never) be traversed? That's currently not possible. With this PR, you can do the following: ```php /** * @Assert\Traverse(false) */ class TagList implements \IteratorAggregate { // ... } ``` #### Follow-up features Features of the follow-up PRs will be described directly there. #### Backwards compatibility I implemented a new `AbstractValidatorTest` which tests both the old and the new implementation for compatibility. I still want to extend this test to make sure we don't introduce any regressions. Almost none of the existing classes were modified (or only slightly). If users depend on the current (now "legacy") implementation, they will have the choice to continue using it until 3.0 (of course, without the new features). #### Your task Congrats, you made it till here :) If you have time, please skim over the code and give me feedback on the overall implementation and the class/method names. Again, no feedback on details yet, there are quite a few areas in the code that are still work in progress. Thanks, Bernhard [1] That means that only the nodes from the root of the graph until the currently validated node are held in memory. Commits ------- ca6a722 [Validator] Converted `@deprecate` doc comment into regular doc comment 68d8018 [Validator] Documented changes in the UPGRADE files b1badea [Validator] Fixed failing CsrfFormLoginTest 0bfde4a [Validator] Fixed misnamed method calls in FrameworkExtension 3dc2b4d [Validator] Made "symfony/property-access" an optional dependency c5629bb [Validator] Added getObject() to ExecutionContextInterface 9b204c9 [FrameworkBundle] Implemented configuration to select the desired Validator API 0946dbe [Validator] Adapted CHANGELOG 1b111d0 [Validator] Fixed typos pointed out by @cordoval 7bc952d [Validator] Improved inline documentation of RecursiveContextualValidator 166d71a [Validator] Removed unused property 90c27bb [Validator] Removed traverser implementation 3183aed [Validator] Improved performance of cache key generation 029a716 [Validator] Moved logic of replaceDefaultGroup() to validateNode() 2f23d97 [Validator] Reduced number of method calls on the execution context 73c9cc5 [Validator] Optimized performance by calling spl_object_hash() only once per object 94ef21e [Validator] Optimized use statements 1622eb3 [Validator] Fixed reference to removed class in ValidatorBuilder be508e0 [Validator] Merged DefaultGroupReplacingVisitor and ContextUpdateVisitor into NodeValidationVisitor 50bb84d [Validator] Optimized RecursiveContextualValidator eed29d8 [Validator] Improved performance of *ContextualValidator::validate() 5c479d8 [Validator] Simplified validateNodeForGroup eeed509 [Validator] Improved phpdoc of RecursiveValidator 274d4e6 [Validator] Changed ValidatorBuilder to always use LegacyExecutionContext 38e26fb [Validator] Decoupled RecursiveContextualValidator from Node 23534ca [Validator] Added a recursive clone of the new implementation for speed comparison f61d31e [Validator] Fixed grammar 886e05e [Validator] Removed unused use statement 93fdff7 [Validator] The supported API versions can now be passed to the ValidatorBuilder 987313d [Validator] Improved inline documentation of the violation builder 79387a7 [Validator] Improved inline documentation of the metadata classes 01ceeda [Validator] Improved test coverage of the Traverse constraint 9ca61df [Validator] Improved inline documentation of CascadingStrategy and TraversalStrategy 524a953 [Validator] Improved inline documentation of the validators 9986f03 [Validator] Added inline documentation for the PropertyPath utility class be7f055 [Validator] Visitors may now abort the traversal by returning false from beforeTraversal() 299c2dc [Validator] Improved test coverage and prevented duplicate validation of constraints 186c115 [Validator] Improved test coverage of NonRecursiveNodeTraverser 822fe47 [Validator] Completed inline documentation of the Node classes and the NodeTraverser dbce5a2 [Validator] Updated outdated doc blocks 8558377 [Validator] Added deprecation notes e8fa15b [Validator] Fixed the new validator API under PHP < 5.3.9 2936d10 [Validator] Removed unused use statement 6fc6ecd [Validator] Fixed tests under PHP<5.3.9 778ec24 [Validator] Removed helper class Traversal 76d8c9a [Validator] Fixed typos 4161371 [Validator] Removed unused use statements aeb6822 [Validator] Improved visitor names 08172bf [Validator] Merged validate(), validateObject() and validateObjects() to simplify usage 51197f6 [Validator] Made traversal of Traversables consistent 117b1b9 [Validator] Wrapped collections into CollectionNode instances 94583a9 [Validator] Changed NodeTraverser to traverse nodes iteratively, not recursively cf1281f [Validator] Added "Visitor" suffix to all node visitors 230f2a7 [Validator] Fixed exception message e057b19 [Validator] Decoupled ContextRefresher from ExecutionContext e440690 [Validator] Renamed validateCollection() to validateObjects() df41974 [Validator] Changed context manager to context factory 26eafa4 [Validator] Removed unused use statements bc29591 [Validator] Clearly separated classes supporting the API <2.5/2.5+ a3555fb [Validator] Fixed: Objects are not traversed unless they are instances of Traversable 2c65a28 [Validator] Completed test coverage and documentation of the Node classes 9c9e715 [Validator] Completed documentation of GroupManagerInterface 1e81f3b [Validator] Finished test coverage and documentation of ExecutionContextManager feb3d6f [Validator] Tested the validation in a separate context 718601c [Validator] Changed validateValue() to validate() in the new API ee1adad [Validator] Implemented handling of arrays and Traversables in LegacyExecutionContext::validate() 09f744b [Validator] Implemented BC traversal of traversables through validate() 297ba4f [Validator] Added a note why scalars are passed to cascadeObject() in NodeTraverser 9b07b0c [Validator] Implemented BC validation of arrays through validate() 405a03b [Validator] Updated deprecation notes in GroupSequence 499b2bb [Validator] Completed test coverage of ExecutionContext adc1437 [Validator] Fixed failing tests 4ea3ff6 [Validator] Finished inline documentation of ExecutionContext[Interface] f6b7288 [Validator] Removed unused use statement 8318286 [Validator] Completed GroupSequence implementation 5fbf848 [Validator] Added note about Callback constraint to CHANGELOG c1b1e03 [Validator] Added TODO reminder 8ae68c9 [Validator] Made tests green (yay!) 680f1ee [Validator] Renamed $params to $parameters 321d5bb [Validator] Throw exception if ObjectInitializer is constructed without visitors 1156bde [Validator] Extracted code for group sequence resolving into GroupSequenceResolver b1a9477 [Validator] Added ObjectInitializer visitor 7e3a41d [Validator] Moved visitors to NodeVisitor namespace a40189c [Validator] Decoupled the new classes a bit a6ed4ca [Validator] Prototype of the traverser implementation 25cdc68 [Validator] Refactored ValidatorTest and ValidationVisitorTest into an abstract validator test class
@lemoinem asked me to look at it for him. No, the issue is still present. I wasn’t able to properly write a test as I had trouble understanding how everything was interconnected, but I made a very simplified version of his demo: https://github.com/lavoiesl/GroupSequenceDemoBundle . Perhaps someone can write a proper test. The problem is that all validators are called and the success of each validator is not checked before testing the next one. This can be desirable if you want all the error messages, but I remember that when @lemoinem stumbled upon this case, it was because he was using the Perhaps there could an option ? I believe the exact for loop is here:
Great job on that PR though ! |
I reproduced the issue now. The problem is not that the return value of
The group check, that is currently only done in step 4, needs to be moved to step 3 as well in order to prevent the accessors from actually being called. |
I just tried fixing this, it's at least not trivial. I see two different solutions:
|
But like I said, the behaviour of checking all groups (and not breaking on the first failure) might be desirable in some cases. If this were to be changed, it should be an option. |
Anybody still working on this bug? |
I encountered this today and it would make sense to separate this from group sequences, since sequence implies that the order matters. Maybe a new interface GroupsProviderInterface could be introduced with GroupsProviderInterface::getGroups() : array, that would just return groups and all of them would be validated. It may (or may not) be easier to implement. Thoughts? |
I looked at the code and the implementation would be pretty straightforward. Will try to put a PR together when i'll have some time (somewhere around July). |
Adding voice to this issue as it appears to still be present. It seems like a pretty important feature for advanced form validation cases. Where we ran into this bug was on a form that has multiple dates that first need to be checked to ensure they are a valid DateTime format and then if they pass that validation we run a custom validation that does a comparison on the dates. We defined two levels of validation with a GroupSequence; however, validation continues to the next group even when a violation in the first group fails. |
I've set up a demo of the issue, the issue doesn't seem to be present when used with annotations, but it does when all constraints are in the form type. |
After hours of searching, I was unable to find a way to get GroupSequence to work. I tried annotations, form options and various combinations and order of groups. Since it was causing a bug in production, my solution was to entirely circumvent the GroupSequence class and do something like this: if ($form->isValid() && $this->validateCreditCard($form)) {
// save and redirect
}
private function validateCreditCard(Form $form)
{
$violations = $this->validator->validate($form->getData(), [new CreditCardConstraint()]);
if (count($violations) === 1) {
$form->addError(new FormError($violations->get(0)->getMessage()));
return false;
}
return true;
} It may not directly solve the bug at hand, but at least gives you an option, since it doesn't look like this will be fixed any time soon. |
Just passing by to confirm the issue is still present is Symfony 3.4.3. |
I've been able to reproduce the bug too on the last 3.4. In other words, IIUC constraints on "getter" methods are interpreted as property constraints with a method call. This design leads to this bug when using constraints on "regular" methods. if (property_exists($this->getClassName(), $property)) {
$this->addPropertyMetadata($this->getters[$property]);
} I'm not sure whether we should fix it like this in 3.4, or document the limitation and consider this a feature targeting master. ping @xabbuh @ogizanagi |
This is all related, I guess that the property is somehow needed to get a property path to attach any error to when calling a method. Though it may not be a problem with the Nevertheless, from my experience it's quite common to use the |
Ok after many tries to get a proper failing test case, I see that I was wrong :). The fix should be done as said in #9939 (comment). Though I don't think it degrades performances that much to use a proxy (creating objects and closure is almost negligible). See #36245 for an attempt to finally fix this 6 years later. |
…(HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [Validator] Fixed calling getters before resolving groups | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #9939 | License | MIT | Doc PR | ~ Commits ------- edcfd60 [Validator] Fixed calling getters before resolving groups
Issue is still present in last 4.x and 5.x, unless it's a different issue? The following still triggers both groups instead of just the first if both fields are invalid: <?php
namespace App\Form;
use App\Model\Contact1;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\SubmitType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Validator\Constraints\GroupSequence;
use Symfony\Component\Validator\Constraints\NotBlank;
class Contact1Type extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options): void
{
$builder->add('fieldOne', TextType::class, [
'constraints' => [
new NotBlank([
'groups' => ['First'],
]),
],
]);
$builder->add('fieldTwo', TextType::class, [
'constraints' => [
new NotBlank([
'groups' => ['Second'],
]),
],
]);
$builder->add('submit', SubmitType::class);
}
public function configureOptions(OptionsResolver $resolver): void
{
$resolver->setDefaults([
'data_class' => Contact1::class,
'validation_groups' => new GroupSequence(['First', 'Second']),
]);
}
} |
@xabbuh hmm, hadn't seen that one - I'll try to test it later, thanks! |
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | FIx #9939 (comment), Fix #35556 | License | MIT | Doc PR | ~ This is not the same as the original issue fixed by #36245, that was reported in #9939 (comment). The form also fails to cascade sequence validation properly because each nested field is validated against the sequence, and one can fail at a step independently from another which could failed in another step. I've added a lot of tests to ensure this is working properly and tested in a website skeleton too. This PR aims to close #35556 which tries to fix the same issue but afterwards in its implementation as said in #35556 (comment). Commits ------- dfb61c2 [Form] Fixed handling groups sequence validation
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | FIx symfony/symfony#9939 (comment), Fix #35556 | License | MIT | Doc PR | ~ This is not the same as the original issue fixed by #36245, that was reported in symfony/symfony#9939 (comment). The form also fails to cascade sequence validation properly because each nested field is validated against the sequence, and one can fail at a step independently from another which could failed in another step. I've added a lot of tests to ensure this is working properly and tested in a website skeleton too. This PR aims to close #35556 which tries to fix the same issue but afterwards in its implementation as said in symfony/symfony#35556 (comment). Commits ------- dfb61c204c [Form] Fixed handling groups sequence validation
Hello,
Using the example below (smallest I could think of).
If you try the given controller, you will have the following results:
I would expect test3 to give the same result as test4 (Meaning: If the first group of the GroupSequence is violated, the others are not executed).
This is explicitly specified in the doc here : http://symfony.com/doc/current/book/validation.html#group-sequence ""In this example, it will first validate all constraints in the group User (which is the same as the Default group). Only if all constraints in that group are valid, the second group, Strict, will be validated."" (emphasis mine)
and here: #2947 (comment)
cc @bschussek
The text was updated successfully, but these errors were encountered: