Problem/Motivation
There are a few issues (like #3441434: Add validation constraints to core.menu.schema.yml) that would love to have a AtLeastOneOf validator. Which would be really helpfull for config that might need a validation and might be allowed as empty string for example.
It was first thought this would need a change to RecursiveContextualValidator to allow groups. But after investigation this seems unneeded!
This MR adds a AtLeastOneOf validator and some tests. This will make the config validation journey a lot easier.
Original IS:
While working in #3441434: Add validation constraints to core.menu.schema.yml we found that the RecursiveContextualValidator prevents us from using custom groups. But we want to use AtLeastOneOf constraint there and it's validator requires every constraint to have a group.
Steps to reproduce
Run tests, its works :P
Proposed resolution
Create a custom AtLeastOneOf validator that works with Drupals Validators (which disallow custom groups).
Remaining tasks
Review
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3461720
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3461720-remove-the-restriction
changes, plain diff MR !8784
Comments
Comment #3
kunal.sachdev commentedComment #4
znerol commentedComment #5
smustgrave commentedSeems straight forward.
Comment #6
znerol commentedGiven the info from #2820364-77: Entity + Field + Property validation constraints are processed in the incorrect order, I'd be quite surprised if removing the
LogicExceptionwould be sufficient to support composite validators.Comment #7
longwaveIf we're going to do this it would be good to have some test coverage to prove that it does work (or indeed, as #6 implies, doesn't work).
Comment #9
bbralaTests are indeed needed, this one would be nice though to have. Commenting to put on my list. Think tests will not be very complicated here.
Comment #10
bbralaComment #11
bbralaComment #12
bbralaI found a way to support the AtLeastOneOf constraint without any changes to groups. This enables some fun stuff, like:
Which is awesome and will help in other validation issues to support vlaidation without the need to make a lot of incompatible changes to migrate to needs null etc.
I think this is ready to get a proper review. :)
Comment #13
penyaskitoI reviewed this, looks perfect to me.
Test coverage is great.
I wanted to create an issue for supporting all composite symfony constraints without having to reimplement them, but never got to it.
The good thing is that we ever end up doing it, this should be easily replaceable with that.
For consistency and DX, we should implement all of them. Maybe we need sibling issues for each of the Composites in
Symfony\Component\Validator\Constraints?I'm setting this to RTBC meanwhile.
Comment #14
bbralaNot sure this is good, while writing tests for #3518146: Rename constraint ValidKeys to ValidMappingKeys i noted using these tests ends in an error where it thinks the constraint name is an array key. This means in on of the two something is weird i think. Setting abck to NW until i can investigate.
Comment #15
bbralaConclusion, it seems all good. SOrry for the confusion.
Comment #16
bbralaComment #17
bbralaComment #18
longwave+1 to creating sibling issue(s) as per @penyaskito in #13 to add the other compound constraints from Symfony.
NW for some nits but otherwise this looks good.
Comment #19
bbralaSince the changes were so minimal, setting back to RTBC, ill also make an issue for the Composite contraints
Comment #20
bbrala#3519515: [META] Support all composite Symfony contraints
Comment #21
bbralaTest failure was unrelated nightwatch test. Rerunning, but seen that one too much to worry.
Comment #23
longwaveCommitted c84246a and pushed to 11.x. Thanks!
This could technically be backported to 10.5.x as an API addition but not sure it's necessary, anything that needs this can just depend on 11.2 or above.
Also published the change record.
Comment #25
wim leersAwesome! Can’t wait to start using this 😄
Comment #26
bbralaYeah I think this one is very useful in a lot of cases. But there are more if you look at the linked meta. Which there you think would help most?