Skip to content
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

Valid groupSequence seems not to work properly #26205

Closed
DonCallisto opened this issue Feb 17, 2018 · 10 comments
Closed

Valid groupSequence seems not to work properly #26205

DonCallisto opened this issue Feb 17, 2018 · 10 comments

Comments

@DonCallisto
Copy link
Contributor

DonCallisto commented Feb 17, 2018

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4

Since Symfony 3.4 it is possible to specify groups in Valid constraint.
Here I provided a working example to prove that groupSequence seems not to worworking with Valid groups.

Is this a bug or am I missing something here?

To test it, clone the repository and run the command bin/console app:valid-group-test

@DonCallisto
Copy link
Contributor Author

DonCallisto commented Feb 17, 2018

Don't know if it's clear what I'm trying to point here, let's be more precise:

  • if I validate Foo without groups --> no Bar validation
  • If I validate Bar, I would like: in first place, to apply all validationsGroup (like Default), otherwise to apply all validationsGroup passed through Valid.

At the moment, this is not be possible for several reason: the biggest problem is that we don't have the chance to specify something like outerGroup and innerGroup where outer is the one of the entity "holding" the sub-one (so Foo in my example) whereas innerGroup is the group for the other (so Bar in my example).

Still don't know if this is a bug or a missing feature 🤔

@eschultz-magix
Copy link
Contributor

eschultz-magix commented Mar 8, 2018

If I understand everything correct here, I can conform this bug. This is my non-working example:

validation.yaml:

App\Model\Outer:
    properties:
        inner:
            - Valid:
                groups: [Inner, Complex]

App\Model\Inner:
    constraints:
        - App\Validator\Constraints\Complex:
            groups: [Complex]

I want to validate the Outer model, which has some more properties, inner is just an example. The Inner model also has some more properties with simple validation constraints and Complex should only get executed if none of the other constraints were failing!

Call:

$validator->validate($outer, null, new GroupSequence(['Inner', 'Complex']));

Result: The Complex validation for the Valid constraint is skipped.

The bug is in RecursiveContextualValidator in method validateInGroup which will check for $context->isConstraintValidated($cacheKey, spl_object_hash($constraint)) and that is true for the second group Complex.

Solution (more or less a workaround):

validation.yaml:

App\Model\Outer:
    properties:
        inner:
            - Valid:
                groups: [Inner]
            - Valid:
                groups: [Complex]

This is working, because both Valid constraints will have a different hash.

@xabbuh
Copy link
Member

xabbuh commented Mar 9, 2018

This is related to #17675 (i.e. both issues are caused by the same cache check in the RecursiveContextualValidator).

@xabbuh
Copy link
Member

xabbuh commented Feb 6, 2019

@DonCallisto Can you confirm that #29499 fixes this issue too?

@DonCallisto
Copy link
Contributor Author

DonCallisto commented Feb 7, 2019

@xabbuh I suppose this is not a valid issue.
Months later, looking at the code, I'm pretty sure that calling that validator with Bar and not with Default produce the "right" behavior as GroupSequence now refers to Default (is clearly stated here.
WDYT?

@xabbuh
Copy link
Member

xabbuh commented Feb 7, 2019

I think I don't follow here. Can you explain a bit more what you mean?

@DonCallisto
Copy link
Contributor Author

@xabbuh Take a look at the code provided. I mean: my expectations were wrong since the moment I've open this issue.
The example is very autoesplicative and there are also some comments that explain what I would have expected.
If something is not understendable for you, let me know.

@xabbuh
Copy link
Member

xabbuh commented Feb 7, 2019

So there is no bug and we can close here?

@DonCallisto
Copy link
Contributor Author

I suppose that we can close

@xabbuh
Copy link
Member

xabbuh commented Feb 7, 2019

Thanks for the confirmation.

@xabbuh xabbuh closed this as completed Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants