Discuss whether to decouple from Symfony Validator

My understanding of why the split is the Constraint can have state (e.g. the value for max length) while the constraint validators are stateless and can actually be services that are loaded by \Drupal\Core\DependencyInjection\ClassResolver.

I find a issue in symfony validator, for example, Isin constraint, it proxy part of its validate logic to Luhn constraint,

  return 0 === $this->context->getValidator()->validate($number, new Luhn())->count();

we have to rewrite the Isin constraint totally, otherwise we will get fatal error:

InvalidArgumentException: The passed value must be a typed data object. in Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate() (line 97 of core\lib\Drupal\Core\TypedData\Validation\RecursiveContextualValidator.php).
Drupal\Core\TypedData\Validation\RecursiveValidator->validate('30280378331005', Object) (Line: 79)
Symfony\Component\Validator\Constraints\IsinValidator->isCorrectChecksum('US0378331005') (Line: 63)
Symfony\Component\Validator\Constraints\IsinValidator->validate('US0378331005', Object) (Line: 116)

I report the issue to symfony at https://github.com/symfony/symfony/issues/51440,

They think this is a bug of Drupal Core ,

or do you mean that Drupal's validator supports validating only TypedData objects and not scalars ? In that case, I suggest fixing that in Drupal instead of doing weird hacks forcing use to add extension points in every place calling a validator recursively.

I support decouple from Symfony Validator, we could not stop them write code in symfony way.

@g089h515r806 this is a separate issue to the discussion here about whether to use Symfony Validator in core or switch to something else. You can create a support issue, or try Drupal Slack for help.

In your case you have to pass TypedData object to Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate() .

See this issue 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC where we validate a plain UploadedFile object which is not typed data. We needed to create a new Validator instance for this.

This is an issue with Isin constraint. They can do this way in other constraint. For example, Negative, Email ...

Email is used in Drupal core, if symfony change the validate logic in Email validator:

  return 0 === $this->context->getValidator()->validate($value, new Regex(['pattern' => $email_pattern]))->count();

Other symfony project using Email constraint still works correctly, but Drupal core's Email constraint will broken.

If Drupal core does not "decouple from Symfony Validator", we should prevent them change code this way in the future.

This might be about to bite us again with Symfony 7.

function user_validate_name($name) {
  $definition = BaseFieldDefinition::create('string')
    ->addConstraint('UserName', []);
  $data = \Drupal::typedDataManager()->create($definition);
  $data->setValue($name);
  $violations = $data->validate();

This code seems quite innocuous, and works in Symfony 6, but fails in Symfony 7. This is because FieldItemList adds an extra constraint:

      $constraints[] = $this->getTypedDataManager()
        ->getValidationConstraintManager()
        ->create('Count', [
          'max' => $cardinality,
          'maxMessage' => t('%name: this field cannot hold more than @count values.', ['%name' => $this->getFieldDefinition()->getLabel(), '@count' => $cardinality]),
        ]);

$this->getFieldDefinition()->getLabel() is NULL. While this is technically a bug, in Symfony 6 this doesn't matter because Count::$maxMessage is untyped, is stored as a TranslatableMarkup object and is never cast to string (although it would be if somehow the base field had multiple values).

In Symfony 7, Count::$maxMessage is typed as string and TranslatableMarkup object is cast to string in the Constraint base constructor. This blows up because NULL is not allowed for the %name replacement.

While we can fix this case by adding a label to the BaseFieldDefinition, I am unsure if casting to string early here will have any additional repercussions on translatable constraint messages.

I am unsure if casting to string early here will have any additional repercussions on translatable constraint messages.

I am not a translation expert but it seems to me that this is less about Symfony and more to do with the construction of the message we're setting on maxMessage? As you mention, "This blows up because NULL is not allowed for the %name replacement." That seems like a Drupal problem vs. anything really foisted on us by Symfony.

TL;DR, this code is buggy and slipped through Symfony 6 without causing problems but it seems the correct solution here is to return a string (or a thing castable to a string) as expected.

I am unsure if casting to string early here will have any additional repercussions on translatable constraint messages.

It's annoying, especially given Symfony rejected our MR to allow Stringable in this API, but overall it shouldn't be a big deal - TranslatableMarkup exists mainly to avoid translating strings that will never be rendered, but translating them earlier will still work.

It might be a bug that we aren't specifying a message parameter, but I assume this is a performance regression, now we are translating strings by casting in the Constraint constructor? Previously the translations would be deferred, perhaps until validation failed and errors actually need to be displayed?

We shouldn't be typing message properties as TranslatableMarkup. We have duplicated Symfony's TranslatorInterface into our own \Drupal\Core\Validation\TranslatorInterface so we can convert them to TranslatableMarkup later. See \Drupal\Core\Validation\DrupalTranslator::trans() and \Drupal\Core\Validation\ExecutionContext::addViolation() where we explicitly call our DrupalTranslator to convert message formats to TranslatableMarkup.

Ah, that makes more sense now, and means the fix for Symfony 7 needs reworking in a different way.

But how does potx pick these strings up if they aren't wrapped in t() or TranslatableMarkup?

But how does potx pick these strings up if they aren't wrapped in t() or TranslatableMarkup?

Not sure about that one.

Created 🐛 FieldItemList::getConstraints() incorrectly sets $maxMessage as TranslatableMarkup Active for #41

If we add ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) to the field definition it will skip that max size check too,