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

Looks great, we have sufficient coverage and this is a small bugfix.

One of the kernel tests is failing do to this change... see https://git.drupalcode.org/issue/drupal-3404061/-/pipelines/56281/test_r...

Forgot to update that one. Green again 👍

smustgrave → credited phenaproxima → .

Reran the failing javascript test and it passed so it was random.

1) Drupal\KernelTests\Config\TypedConfigTest::testSimpleConfigValidation
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'This value should not be null.'
+'This value should not be blank.'
/builds/issue/drupal-3404061/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3404061/core/tests/Drupal/KernelTests/Config/TypedConfigTest.php:174
/builds/issue/drupal-3404061/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 47, Failures: 1.

Ran the test-only feature and got this so test coverage is there.

Crediting @phenaproxima for the MR review but his name wasn't appearing on the ticket.

Seems all feedback has been addressed though.

Agreeing with @smustgrave, all feedback has been addressed, and this is a good improvement.

alexpott → credited longwave → .

@longwave, @Wim Leers and I have discussed this issue at length. The discussion resulted in wanting to explore some other options, namely:

1. Do it at a different level, for example \Drupal\Core\TypedData\Validation\TypedDataMetadata::getConstraints(), which is what the validation system always calls — and detect presence of both and configure the other one correctly
2. Override NotBlankValidator and just make allowNull: true do nothing.
3. Look into \Symfony\Component\Validator\Constraints\Sequentially
4. Change getDefaultConstraints() to configured NotBlank correctly instead of adding NotNull when $definition->isRequired() and NotBlank is present already.

👍 Thanks, @alexpott!

Just pushed an implementation of option 1.

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 🐛 Entity + Field + Property validation constraints are processed in the incorrect order Needs work 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 😊

Re #18 - I think we'd need our own Sequentially.

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

Only one comment about a comment, but otherwise this looks right to me and I will RTBC it once my feedback is addressed in some way. :)

Done 😄

Ship it!

Committed 5cd249e and pushed to 11.x. Thanks!

Thanks!

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

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