Discuss whether to decouple from Symfony Validator

Another issue is performance, the way we use it, with our complex entity objects, validation is also very slow, which always made form submissions quite slow and with recent "improvements" to contexts and context validation as part of the layout builder initiative, this becomes more and more visible also when just viewing content. See #3054042: Views performance regression related to renders. (Not blaming anyone here, they simply use the tools that are available).

Our own implementation might also be written to better support our specific use cases (that said, we need to keep it generic to support both content entity and config-based validation).

Of course switching away from that is going to be very challenging, as always, we somehow need omnidirectional BC (both for things calling the validation as well as still supporting symfony validation constraints).

Maybe another option is to try to decouple validation from message generation / translation but do this upstream.

The main issue we have is that we're exposing Symfony interfaces directly to contrib and custom code (I have custom validators written on client projects), which means any change Symfony makes directly impacts implementations.

Ultimately this combined with #3005229: Provide optional support for using composer.json for dependency metadata would mean that if you are directly using symfony interfaces that are subject to change, you are responsible for specifying your compatible symfony versions. This is (or will be) essentially something new to the contrib culture, so we should document and blog about it when the time comes.

2. How much code is there in the component itself that's not the validators? What if we froze the API and forked the internals (just changing the Symfony interfaces to a Drupal one). Are there other components that depend on Validator we'd then be incompatible with and would we actually be affected by this?

Quickly checking the composer.json of the other symfony components, It seems to be a dev requirement of a few components, but not an actual requirement, particularly if we don't use the symfony form system.

I'm not sold on replacing it versus refactoring to use it properly, however the performance issues mentioned by @berdir do give me pause. I still wonder though if a new validator is the solution to those performance issues, or if those are still more about being more selective about when/how we use the validator.

1. Can we write some kind of layer so that validators implement Drupal rather than Symfony interfaces, then make them work for the Symfony internals?

This may be possible, but it seems likely to exacerbate rather than ease any performance issues with the validator as it sits. It also still leaves us with a maintenance burden as the validator changes, though hopefully all in one place and more manageable.

That all having been said, There haven't actually been a lot of changes to the validator in the 4.x cycle. There was #2937542: Not setting the strict option of the Choice constraint to true is deprecated since Symfony 3.4 and will throw an exception in 4.0, but that change was deprecated and announced in 3.4.0. #3029540: [Symfony 4] Sub class \Symfony\Component\Validator\ConstraintViolation and use that in \Drupal\Core\TypedData\Validation\ExecutionContext::addViolation() isn't a deprecation, it's just using php to enforce the interface that was already there that we are already violating.

If we take the primary current issue with strict typing in the violation messages, what we really need bridge-wise is a way to preserve our translatable markup through the violation collection and display process. I think there is enough storage in the Symfony Violation value object to do that, and I think the constraint system actually already does, we just need to convert to markup when rendering the messages instead of when building them. Internally we have Drupal\Core\Validation\DrupalTranslator implementing Drupal\Core\Validation\TranslatorInterface, which is separate from Symfony's TranslatorInterface, but ours still declares trans(..) will return a string, yet it returns TranslatableMarkup. If THAT interface were honored correctly, we would see the same problems now that we see in SF4, so I worry that forking the validator won't solve the core issue of us passing around Markup where we declare strings.

For me the symfony validation implementation in Drupal feels really alien:

• Our usage is totally different to what the original documentation talks about: https://symfony.com/doc/current/validation.html
• Instead validation is really basically something we put on top of typed data rather than symfony which treats it as a orthogonal concept. Conceptually for typed data to be useful it needs to validate it's data, so separating that out feels weird
• The developer experience we have is really weird. In symfony the constraint and the actual validation logic is separated, probably because of reusability
• We over string translation, so generating the wrong english message isn't really a problem, you can translate it different

Another issue is performance, the way we use it, with our complex entity objects, validation is also very slow, which always made form submissions quite slow and with recent "improvements" to contexts and context validation as part of the layout builder initiative, this becomes more and more visible also when just viewing content.

Do you know where the slowness is really coming from?

Another issue is performance, the way we use it, with our complex entity objects, validation is also very slow, which always made form submissions quite slow

Indeed. I've anecdotally heard of it being problematic too. Ideally we'd have numbers for this. Perhaps you do, @Berdir?

Of course switching away from that is going to be very challenging

💯

Instead validation is really basically something we put on top of typed data rather than symfony which treats it as a orthogonal concept. Conceptually for typed data to be useful it needs to validate it's data, so separating that out feels weird

💯

In symfony the constraint and the actual validation logic is separated, probably because of reusability

That's true for the validation constraints we have in Drupal too. I guess you're saying that for Drupal's purpose, these should be one and the same?

Yes, it's specifically a problem around layout builder and its extensive block usage combined with entity contexts.

However, regular entity validation is also problematic, for example when combined with paragraphs as each paragraph needs to be validated, and that sums up quickly once you have 10, 20, ... paragraphs on a page.

@dawehner #7:
> Instead validation is really basically something we put on top of typed data rather than symfony which treats it as a orthogonal concept. Conceptually for typed data to be useful it needs to validate it's data, so separating that out feels weird

+1 to that.

> Instead validation is really basically something we put on top of typed data rather than symfony which treats it as a orthogonal concept. Conceptually for typed data to be useful it needs to validate it's data, so separating that out feels weird

There are other cases where validation is useful. The other day I found myself thinking it would be really nice if Form API element could take a list of validator plugins like this:

$form['my_element'] = [
  '#type' => 'textfield',
  '#validators' => [ // array of validator plugins]
];

Would be pretty nifty, no? And forms are not always tied to entities or typed data.

So I think validators as plugins is an ok architecture.

That said, the current system of needing two classes just to write one validator is a pain and it's bad DX. I understand it's for reusability and swappability, and separating the validation and the messages improves that, but more of the validators I've seen in Drupal don't reuse either class but provide two from scratch.

for history reasons

Symfony 5.2 has changed the constructor of Constraint plugins and so this has bitten us, not in a way that we can't fix though: #3185603: [Symfony 5] Symfony Constraint plugins are not strictly Drupal plugins

Sadly I found no roadmap for SF validator so +1 to find a way back to own implementation

Based on upstream feedback the direction on #3255245: [Symfony 6] Revert 3231603 to use our own TranslatorInterface is to supress the deprecation message for now and continue discussing how to decouple best.

Another thought about the separation of validation logic and error messages into two classes: I am currently fiddling with #3267404: Add a PHPUnit assertion that an entity is valid, to produce errors from entity validation in tests.

If I get a validation error in a test, my steps to fix it are:

1. copy the error message from the terminal, e.g. 'Project lead cannot be empty'
2. search for it in the codebase to find the Constraint class which defines the error message as a property, e.g. ProjectConstraint
3. copy the name of the class variable which holds this message in the Constraint class, e.g. $projectLeadEmpty
4. Go over to the adjacent Validator class, e.g. ProjectConstraintValidator
5. search for the class variable name to see where it's used in the validation logic.

This is not good DX!

Looks core using only 2 interfaces from validator, and "magic widget delta getter" from #3298731: Using ConstraintViolation::$arrayPropertyPath bugs on PHP 8.2

It bite again in 6.2 with #3284422-25: [META] Symfony 6.2 compatibility

3x: Since symfony/validator 6.2: The "loose" mode is deprecated. The default mode will be changed to "html5" in 7.0.

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 must pass TypedData object to Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate() .

See this issue #3375447: Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface 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 not only with symfony 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(It will not happen):

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

For Negative, if they use following code in its validate function, designate its validate logic to LessThan constraint:

0 === $this->context->getValidator()->validate($value, new LessThan(['value'=0]))->count()

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

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

We could not prevent symfony validator do it because Drupal core depend on it.

If Drupal core does not "decouple from Symfony Validator", Symfony Validator has the chance to break all drupal sites in the future, if they change some code in symfony way. I know this will not happen, but they have a chance to do it.

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 so the 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.

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 up front 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 #3431200: FieldItemList::getConstraints() incorrectly sets $maxMessage as TranslatableMarkup for #41

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