symfony: [Validator][GroupSequences] GroupSequences still execute next group if first fail
Hello,
Using the example below (smallest I could think of).
If you try the given controller, you will have the following results:
- test1 : all OK [as expected]
- test2 : “is not three” [as expected]
- test3 : _CRASH_ "“Error: Call to a member function myBar() on a non-object in /myProject/src/Me/MeBundle/Doc/Foo.php line 31"” [_NOT_ as expected]
- test4 : “bar should not be null” [as expected]
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: https://github.com/symfony/symfony/issues/2947#issuecomment-3509046
cc @bschussek
<?php
namespace ME\MEBundle\Doc;
use Symfony\Component\Validator\Constraints as Assert;
/**
* @Assert\GroupSequence({"FilledIn", "Foo"})
*/
class Foo
{
protected $bar;
protected $checkBar;
public function __construct($bar, $checkBar = false)
{
$this->bar = $bar;
$this->checkBar = $checkBar;
}
/**
* @Assert\True(groups={"Foo"}, message="is not three")
*/
public function isThree()
{
if ($this->checkBar && null === $this->bar) {
return new Bar(null);
}
return 3 == $this->getBar()->myBar();
}
/**
* @Assert\NotNull(groups={"FilledIn"}, message="bar should not be null")
*/
public function getBar()
{
return $this->bar;
}
}
<?php
namespace Me\MeBundle\Doc;
class Bar
{
protected $bar;
public function __construct($bar)
{
$this->bar = $bar;
}
public function myBar()
{
return $this->bar;
}
}
<?php
namespace Me\MeBundle\Controller;
use Me\MeBundle\Doc;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Template;
use Symfony\Component\HttpFoundation\JsonResponse;
class DefaultController extends Controller
{
protected function getFooData(Foo $foo)
{
$json = [['bar' => null === $foo->getBar() ? null : (array)$foo->getBar()]];
$violations = $this->get('validator')->validate($foo);
$errors = [];
foreach($violations as $violation) {
$errors[] = $violation->getMessage();
}
$json[] = $errors;
return $json;
}
/**
* @Route("/test1", defaults={"_format"="json"})
*/
public function test1Action()
{
new Foo(null);
$foo = new Foo(new Bar(3));
return new JsonResponse($this->getFooData($foo));
}
/**
* @Route("/test2", defaults={"_format"="json"})
*/
public function test2Action()
{
$foo = new Foo(new Bar(4));
return new JsonResponse($this->getFooData($foo));
}
/**
* @Route("/test3", defaults={"_format"="json"})
*/
public function test3Action()
{
$foo = new Foo(null);
return new JsonResponse($this->getFooData($foo));
}
/**
* @Route("/test4", defaults={"_format"="json"})
*/
public function test4Action()
{
$foo = new Foo(null, true);
return new JsonResponse($this->getFooData($foo));
}
}
About this issue
- Original URL
- State: closed
- Created 10 years ago
- Comments: 23 (17 by maintainers)
Commits related to this issue
- feature #10287 [WIP][Validator] New NodeTraverser implementation (webmozart) This PR was merged into the 2.5-dev branch. Discussion ---------- [WIP][Validator] New NodeTraverser implementation | Q... — committed to symfony/symfony by fabpot 10 years ago
- bug #36245 [Validator] Fixed calling getters before resolving groups (HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [Validator] Fixed calling getters before resolving grou... — committed to symfony/symfony by nicolas-grekas 4 years ago
- bug #36343 [Form] Fixed handling groups sequence validation (HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q ... — committed to symfony/symfony by nicolas-grekas 4 years ago
- bug #36343 [Form] Fixed handling groups sequence validation (HeahDude) This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q ... — committed to symfony/form by nicolas-grekas 4 years ago
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.
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 https://github.com/symfony/symfony/issues/9939#issuecomment-39191810. 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.
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:
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.
@xabbuh hmm, hadn’t seen that one - I’ll try to test it later, thanks!
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).
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?