Skip to content

[Validator] Allow validating multiple groups in one GroupSequence step #19982

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

Merged
merged 3 commits into from
Oct 19, 2016

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Sep 19, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

To see what I've changed just look at the last commit. The other two are just refactoring without any effect.

GroupSequenceProviderInterface seems to be the recommended solution for conditional validators (like when some properties should be required only if other property has a certain value). And it's a good solution honestly. Except for the fact that it's completely useless when I want to get validation violations for all groups at once and not just violations for the first group that caused any violations.

// If the User validation group causes any violations the Premium group will not be
// validated at all even if the $this->isPremium() is true.
class User implements GroupSequenceProviderInterface {
    public function getGroupSequence() {
        $groups = array('User');
        if ($this->isPremium()) {
            $groups[] = 'Premium';
        }
        return $groups;
    }
}

To be honest I never found a use case for this step-by-step behavior. When user fills a form I want to show him all errors at once not just some subset.

It's surprisingly easy to fix this. With just one changed line in RecursiveContextualValidator it's perfectly solveable:

// With this PR it is possible to do this and get violations for both groups like this.
class User implements GroupSequenceProviderInterface {
    public function getGroupSequence() {
        $groups = array('User');
        if ($this->isPremium()) {
            $groups[] = 'Premium';
        }
        return [$groups]; // this line has changed
    }
}

@enumag
Copy link
Contributor Author

enumag commented Oct 18, 2016

@fabpot Just found out that conditional validations on subforms are a real pain at the moment since the validation groups can only be defined on root forms. This PR provides a far better solution because then I can override the Default validation group by implementing GroupSequenceProvider.

@fabpot
Copy link
Member

fabpot commented Oct 19, 2016

ping @HeahDude

@HeahDude
Copy link
Contributor

Simple and efficient 👍 I really like this parentheses' move feature :)

@fabpot
Copy link
Member

fabpot commented Oct 19, 2016

Thank you @enumag.

@fabpot fabpot merged commit 31b609e into symfony:master Oct 19, 2016
fabpot added a commit that referenced this pull request Oct 19, 2016
…oupSequence step (enumag)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Validator] Allow validating multiple groups in one GroupSequence step

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

To see what I've changed just look at the last commit. The other two are just refactoring without any effect.

GroupSequenceProviderInterface seems to be the recommended solution for conditional validators (like when some properties should be required only if other property has a certain value). And it's a good solution honestly. Except for the fact that it's completely useless when I want to get validation violations for all groups at once and not just violations for the first group that caused any violations.

```php
// If the User validation group causes any violations the Premium group will not be
// validated at all even if the $this->isPremium() is true.
class User implements GroupSequenceProviderInterface {
    public function getGroupSequence() {
        $groups = array('User');
        if ($this->isPremium()) {
            $groups[] = 'Premium';
        }
        return $groups;
    }
}
```

To be honest I never found a use case for this step-by-step behavior. When user fills a form I want to show him all errors at once not just some subset.

It's surprisingly easy to fix this. With just one changed line in RecursiveContextualValidator it's perfectly solveable:

```php
// With this PR it is possible to do this and get violations for both groups like this.
class User implements GroupSequenceProviderInterface {
    public function getGroupSequence() {
        $groups = array('User');
        if ($this->isPremium()) {
            $groups[] = 'Premium';
        }
        return [$groups]; // this line has changed
    }
}
```

Commits
-------

31b609e [Validator] Allow validating multiple groups in one GroupSequence step
3847bad [Validator] Refactor tests
38b643a [Validator] Fix annotation
@enumag
Copy link
Contributor Author

enumag commented Oct 20, 2016

@fabpot Thank you! I was going through the code again and found one thing that doesn't seem right. I couldn't find any usage of GroupSequence::$cascadedGroup. If I understand the code correctly it should be used in RecursiveContextualValidator::validateClassNode() when calling the stepThroughGroupSequence method. If I'm not mistaken $defaultOverridden ? Constraint::DEFAULT_GROUP : null, should actually be $defaultOverridden ? ($group->cascadedGroup ?: Constraint::DEFAULT_GROUP) : null,.

@webmozart You were the one who originally added the cascadedGroup. Am I right about what I've written in the previous paragraph?

If you can confirm I'm right about this I'll send another PR. Also cascadedGroup should be allowed to be an array as well which means changing stepThroughGroupSequence from $cascadedGroups = $cascadedGroup ? array($cascadedGroup) : null; to $cascadedGroups = $cascadedGroup ? (array) $cascadedGroup : null;. Which raises a question that we maybe should rename GroupSequence::$cascadedGroup to GroupSequence::$cascadedGroups.

@blazarecki
Copy link

@enumag I think this is relative to this issue #17696

@enumag
Copy link
Contributor Author

enumag commented Oct 20, 2016

@blazarecki I don't think so. That issue is about propagation of groups for the @All constraint. This is a different problem. And I think it's a regression - cascadeGroup was used by the old Validator implementation that was removed in 3.0 but isn't used by the new RecursiveValidator.

@xabbuh
Copy link
Member

xabbuh commented Oct 20, 2016

@enumag Can you open a new PR with what you have in mind? Not everyone is following already merged PRs so discussions here may get lost.

@enumag
Copy link
Contributor Author

enumag commented Oct 20, 2016

@xabbuh Alright. Hopefully I can find some time this weekend.

@fabpot fabpot mentioned this pull request Oct 27, 2016
fabpot added a commit that referenced this pull request Jun 20, 2019
This PR was squashed before being merged into the 3.4 branch (closes #32044).

Discussion
----------

[Validator] Fix GroupSequenceProvider annotation

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The possibility was added in #19982, just forgot to fix this annotation back then.

Commits
-------

bf6d253 [Validator] Fix GroupSequenceProvider annotation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants