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

Command icon 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:

Comments

kunal.sachdev created an issue. See original summary.

kunal.sachdev’s picture

Status: Active » Needs review
znerol’s picture

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems straight forward.

znerol’s picture

Given the info from #2820364-77: Entity + Field + Property validation constraints are processed in the incorrect order, I'd be quite surprised if removing the LogicException would be sufficient to support composite validators.

longwave’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

bbrala made their first commit to this issue’s fork.

bbrala’s picture

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

bbrala’s picture

Issue summary: View changes
Issue tags: -Needs tests
bbrala’s picture

Title: Remove the restriction from RecursiveContextualValidator that prevents using custom groups. » Create AtLeastOneOf constraint
Issue summary: View changes
Status: Needs work » Needs review
bbrala’s picture

I found a way to support the AtLeastOneOf constraint without any changes to groups. This enables some fun stuff, like:

            constraints:
              AtLeastOneOf:
                constraints:
                  - PluginExists:
                      manager: plugin.manager.menu.link
                      interface: 'Drupal\Core\Menu\MenuLinkInterface'
                  - IdenticalTo:
                      value: ''

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

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

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

bbrala’s picture

Status: Reviewed & tested by the community » Needs work

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

bbrala’s picture

Status: Needs work » Reviewed & tested by the community

Conclusion, it seems all good. SOrry for the confusion.

bbrala’s picture

bbrala’s picture

longwave’s picture

Status: Reviewed & tested by the community » Needs work

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

bbrala’s picture

Status: Needs work » Reviewed & tested by the community

Since the changes were so minimal, setting back to RTBC, ill also make an issue for the Composite contraints

bbrala’s picture

bbrala’s picture

Test failure was unrelated nightwatch test. Rerunning, but seen that one too much to worry.

  • longwave committed c84246a8 on 11.x
    Issue #3461720 by bbrala, kunal.sachdev, longwave, penyaskito: Create...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

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

wim leers’s picture

Awesome! Can’t wait to start using this 😄

bbrala’s picture

Yeah 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?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.