[Validator][GroupSequences] GroupSequences still execute next group if first fail

[lemoinem]

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: #2947 (comment)

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));
    }
}

[jakzal]

Issue confirmed and reproduced (https://github.com/jakzal/symfony-standard/compare/symfony:master...jakzal:issue9939).

[jakzal]

@lemoinem I see one problem with the Foo::isThree() implementation. Calling methods shouldn't throw exceptions, just because an object is put in an invalid state.

If you change the implementation to check if $this->bar exists before using it:

    public function isThree()
    {
        // ...

        return $this->bar && 3 == $this->getBar()->myBar();
    }

You'll notice that validation is actually not run on isThree().

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.

[jakzal]

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).

[lemoinem]

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.

[webmozart]

Could you check whether #10287 fixes this issue?

[lavoiesl]

@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 @Assert\NotNull as a safeguard, so he wouldn’t need to copy-paste a if (null !== $var) in each validation group.

Perhaps there could an option ? @Assert\GroupSequence({"FilledIn", "Foo"}, break=true)

I believe the exact for loop is here:

symfony/src/Symfony/Component/Validator/Mapping/ClassMetadata.php

Line 162 in 58bed5d

foreach ($this->getConstrainedProperties() as $property) {

Great job on that PR though !

[webmozart]

I reproduced the issue now. The problem is not that the return value of isTwo() is validated, but that isTwo() is called before the validator checks whether that property ("two") actually has any constraints in the requested group ("FilledIn"). So basically, what the validator does is:

1. Validate entity
2. Check first group "FilledIn"
3. Get the values of all properties that have constraints in any group (source)
4. Validate those values against the constraints in the given group ("FilledIn")

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.

[webmozart]

I just tried fixing this, it's at least not trivial. I see two different solutions:

• Call PropertyMetadataInterface::getPropertyValue() lazily by wrapping it in a proxy class. This degrades performance for the average case though, so I discarded this solution.
• Add the argument $groups = null to ClassMetadata::getConstrainedProperties(). This requires a change in the book-keeping in order to be efficient. This solution might work, but I couldn't finish it for now.

[lavoiesl]

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.

[data219]

Anybody still working on this bug?

[DusanKasan]

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?

[DusanKasan]

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).

[PeterA]

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.

[veloxy]

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.

[afilina]

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.

[ralitsavladimirova]

Just passing by to confirm the issue is still present is Symfony 3.4.3.

[HeahDude]

I've been able to reproduce the bug too on the last 3.4.
I think I understand the problem, it does not come from a validator implementation issue, but from the way methods are registered in the class metadata. What I never understand is the limitation when using such methods that must start with get, has or is.

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.
I've been able to fix the issue by changing this line to:

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

[HeahDude]

What I never understand is the limitation when using such methods that must start with get, has or is.

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 Callback constraint since it has low level access and can call atPath on the current context.

Nevertheless, from my experience it's quite common to use the IsTrue constraint with some isSomethingValid method without having a property somethingValid, the error being attached to the validated object by default (kind of bubbling up).

[HeahDude]

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.

[veloxy]

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']),
        ]);
    }
}

See https://github.com/veloxy/symfony-group-sequence-issue

[xabbuh]

@veloxy Would #35556 solve your issue?

[veloxy]

@xabbuh hmm, hadn't seen that one - I'll try to test it later, thanks!

[HeahDude]

@veloxy thanks for the report, this is not related to the original issue here, see #36343.