Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] Allow Sequentially constraints on classes + target guards#35815
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Validator] Allow Sequentially constraints on classes + target guards#35815
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| thrownewConstraintDefinitionException(sprintf('The constraint "%s" cannot be put on classes.',\get_class($constraint))); | ||
| } | ||
| if ($constraintinstanceof Sequentially ||$constraintinstanceof Compound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All could probably be added to this list, but could it lead to a BC break somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All could probably be added to this list
Why? It does not allow classes already thanks to theif before 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wrong path. I meant inMemberMetadata 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, what about a check againstComposite instead? It may be needed for both then, and even target 3.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alright, let's consider this a bug fix. I'll move the second commit to another PR for 3.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#35843
HeahDude left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
src/Symfony/Component/Validator/Tests/Mapping/ClassMetadataTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedFeb 25, 2020
Thank you@ogizanagi. |
…ints (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Validator] Add target guards for Composite nested constraints| Q | A| ------------- | ---| Branch? | 3.4 <!-- see below -->| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fix#35815 (review) <!-- prefix each issue number with "Fix #", if any -->| License | MIT| Doc PR | N/ACommits-------a08ddf7 [Validator] Add target guards for Composite nested constraints
…ints (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Validator] Add target guards for Composite nested constraints| Q | A| ------------- | ---| Branch? | 3.4 <!-- see below -->| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fixsymfony/symfony#35815 (review) <!-- prefix each issue number with "Fix #", if any -->| License | MIT| Doc PR | N/ACommits-------a08ddf7636 [Validator] Add target guards for Composite nested constraints
There is no reason to limit this constraint to properties, so let's add classes as targets.
Additionally, let's ensure embedded constraints matches allowed targets too.