When setting `NotNull` constraint on type: required_label, two validation errors appear

Addressed!

I'm wondering if at this point a separate test method with @dataProvider or @testWith wouldn't be clearer though 😅

Forgot to update that one. Green again 👍

👍 Thanks, @alexpott!

Just pushed an implementation of option 1.

Why?

1. Option 1 is the most tightly scoped systemic fix.
2. Option 2 is a BC break.
3. Option 3 is the most interesting choice, but also a far bigger change. Essentially, we could automatically convert
   

   - Foo
   - NotNull
   - Bar
   

   to

   Sequentially:
     - NotNull
     - 
       - Foo
       - Bar
   

   — https://symfony.com/doc/current/reference/constraints/Sequentially.html

   i.e. if NotNull is present, run it first, and if it doesn’t pass, don’t execute any of the other constraints; if it does pass, run all other constraints like we do today.

   But that's a huge change, with unpredictable ripple effects, and there's much more nuance to it than that — there's other validation constraint execution ordering issues, and they have been known for >7 years: #2820364 — see #2820364-85: Entity + Field + Property validation constraints are processed in the incorrect order for a recent update.

   IOW: I think special-casing NotNull only for now (prior to fixing that entire massive issue) might be okay, but I’m not 100% certain.

4. This alternative to option 1 is insufficient if both NotNull and NotBlank are declared simultaneously (like for string__not_null__not_blank in the test coverage), but @alexpott says we could disallow this and trigger a warning. I'd be fine with that, but it's technically a BC break too, and we historically have no verification whatsoever that data types or config schema types make sense — that's why #3401837: Add basic validation to config schema definitions and #3404431: Filter settings schema types are incorrect exist. I'd definitely be in favor for starting to add validation for them though, that'd be an important first step to improving their DX!

I just tried implementing Sequentially.

Looks like my fear was unfounded: the "unpredictable ripple effects" did not happen, thanks to \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validate() being very narrow in what it accepts:

  public function validate($data, $constraints = NULL, $groups = NULL, $is_root_call = TRUE): static {
    if (isset($groups)) {
      throw new \LogicException('Passing custom groups is not supported.');
    }

    if (!$data instanceof TypedDataInterface) {
      throw new \InvalidArgumentException('The passed value must be a typed data object.');
    }

… because \Symfony\Component\Validator\Constraints\SequentiallyValidator::validate() calls Drupal's RecursiveContextualValidator::validate() (the first ~10 lines of which are displayed above), and $data === 'en' at this point (for the langcode), it throws the exception with the The passed value must be a typed data object. message.

IOW: to adopt Symfony's Sequentially, we need to revise a lot about how Drupal uses the symfony/validator component. For #2820364: Entity + Field + Property validation constraints are processed in the incorrect order we probably will need validation groups, which as you can see above are also explicitly not supported.

alexpott 32 minutes ago
I think option 4 is better than option 1.

alexpott 31 minutes ago
I think we should be doing as little as possible as changing you constraints is quite surprising.

Will figure out how to make that work 😊

Implemented option 4.

Due to

  public function getConstraints() {
    $constraints = $this->definition['constraints'] ?? [];
    $constraints += $this->getTypedDataManager()->getDefaultConstraints($this);
…

any change I make in getDefaultConstraints() gets overwritten automatically.

So this change must happen in the quoted code (\Drupal\Core\TypedData\DataDefinition::getConstraints()).

Done 😄

Thanks!

This unblocked #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support but also simplified #3379091 (see #3379091-25: Make NodeType config entities fully validatable) as well as any future work on adding validation constraints where NotBlank is relevant! 👍