Problem/Motivation

There are many validators provided by Symfony that are not by default accessible as Drupal Constraint Validators

Steps to reproduce

Try to use any Symfony Validation Constraint that isn't currently in core - ex Positive, PositiveOrZero, Url (important one), Json, [and about 50 others)
Positive/PositiveOrZero validators are specificly needed for Spambot module. Url for JsonLD module.
Others are more theoretical need, but could get use cases for most of the weirder ones even with a few modules from commerce and address ecosystems I believe.

Proposed resolution

Add missing validators

Issue fork drupal-3365498

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

NickDickinsonWilde created an issue. See original summary.

nickdickinsonwilde’s picture

Patch attached for review.
I believe that since this is just making the symfony validators accessible to Drupal core that no tests are directly needed.

nickdickinsonwilde’s picture

Done with discussion with Wim Leers and Rosie La Faive

nickdickinsonwilde’s picture

Status: Active » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.93 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

borisson_’s picture

Based on #5, we should probably tell cspell that luhn, issn and iban are actually correct words?

nickdickinsonwilde’s picture

Status: Needs work » Needs review
Issue tags: -DrupalCon Pittsburgh 2023 +Pittsburgh2023
StatusFileSize
new14.2 KB

Never ran cspell locally! Let's try this...

Status: Needs review » Needs work

The last submitted patch, 7: 3365498-add-validators-7.patch, failed testing. View results

nickdickinsonwilde’s picture

Status: Needs work » Needs review
StatusFileSize
new10.06 KB

Fix a couple duplications

wim leers’s picture

Status: Needs review » Needs work

Looks GREAT!

Could you please explain why this is a contributed project blocker? Where did you encounter a need for this? 😊

  1. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'class' => '\Symfony\Component\Validator\Constraints\Bic',
    

    Could you update these to be of the format Bic::class? 🙏

    That way any future refactoring would be simpler! :)

  2. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Cidr'),
    

    I think these should all be capitalized? 😅🤓

  3. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('CssColor'),
    

    I think the label should be "CSS Color" 🤓

  4. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Iban'),
    

    I think the label should be "IBAN" 🤓

  5. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Ip'),
    

    I think the label should be "IP address" 🤓

  6. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Isbn'),
    

    I think the label should be "ISBN" 🤓

  7. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Isin'),
    

    I think the label should be "ISIN" 🤓

  8. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Json'),
    

    I think the label should be "JSON" 🤓

  9. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +    $this->getDiscovery()->setDefinition('NotCompromisedPassword', [
    +      'label' => new TranslatableMarkup('Not compromised password'),
    +      'class' => '\Symfony\Component\Validator\Constraints\NotCompromisedPassword',
    +      'type' => 'string',
    +    ]);
    

    🤯 Holy crap!

    This uses https://haveibeenpwned.com/API/v2#SearchingPwnedPasswordsByRange, which is an external API. Let's omit this one 😅

  10. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('NotIdenticalTo'),
    

    I think the label should be "Not identical to" 🤓

  11. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Positive Or Zero'),
    

    I think the label should be "Positive or zero", for consistent capitalization with other similar labels 🤓

  12. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Ulid'),
    

    I think the label should be "ULID" 🤓

  13. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -95,11 +95,221 @@ public function registerDefinitions() {
    +      'label' => new TranslatableMarkup('Url'),
    

    I think the label should be "URL" 🤓

nickdickinsonwilde’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new11.3 KB

Patch updated with adjustments as per #11.
Note, also changed existing few definitions to use format Bic::class rather than string of current FQCN

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

🤩

wim leers’s picture

Oh, arguably we should remove

/**
 * Implements hook_validation_constraint_alter().
 */
function ckeditor5_validation_constraint_alter(array &$definitions) {
  // Add the Symfony validation constraints that Drupal core does not add in
  // \Drupal\Core\Validation\ConstraintManager::registerDefinitions() for
  // unknown reasons. Do it defensively, to not break when this changes.
  if (!isset($definitions['Choice'])) {
    $definitions['Choice'] = [
      'label' => 'Choice',
      'class' => Choice::class,
      'type' => FALSE,
      'provider' => 'core',
      'id' => 'Choice',
    ];
  }
}

and instead do that here too 🤓

I'm curious what release managers think!

borisson_’s picture

I agree with #12, this is a great thing that we get new free validators in core. Ideally everything that is added to core should have at least one implementation, but I think we can forget about that requirement here because it is added code that is tested and used in symfony itself.
RTBC+1

catch’s picture

I have one concern here which boils down to #3054535: Discuss whether to decouple from Symfony Validator - Symfony Validator has had the most churn of any Symfony component we use, in a way that often impacts Symfony minor version updates.

One the one hand, making their validators available means we'd get more benefit from using the component since we'd actually be using more of the code instead of (mostly) just their API, but this is also the drawback, that it exposes more of their code as API surface.

If contrib starts using one and then we decided we don't want to expose it, how would we handle that given we can't deprecate their code?

Also if we do eventually decide to move away from Symfony validator, would we then need to re-implement all of these in core compared to currently just the ones we're actually using?

nickdickinsonwilde’s picture

Hmmm. I see that now. Currently about uh 5? 6? of the Symfony validators are exposed already - two with PHP annotation/inheritance/overrides and a few existing ones in the current Constraint Manager.

So not a totally new problem, but could make things significantly worse. Since they are being exposed as plugins, we can deprecate them as per https://www.drupal.org/about/core/policies/core-change-policies/drupal-d.... If that enough of protection? Combined with ability to copy validator code if future need arose, I believe without breaking BC, we could change the PHP path behind a plugin as long as the plugin name was unchanged yes?

catch’s picture

Add @trigger_error('...', E_USER_DEPRECATED) to the plugin constructor.

We'd need to subclass the Symfony plugins in order to be able to do that ... maybe that is fine?

nickdickinsonwilde’s picture

Yeah, that was my thought/hope. As long as we don't have to do that, quicker/less overhead LoC/files to do it in the manager but then if we need to change, subclass and deprecate/do changes.

longwave’s picture

Is it possible to write a test that ensures all Symfony validators are covered, so if more are added in a future Symfony release we will be automatically notified?

And if this is possible, is it possible to discover and register them in the same way, so we don't need to manually define each one?

nickdickinsonwilde’s picture

hmm... I don't see why not. Just PHP files. Unit test should be able to do that fine.

Slight caveat/yes, and... we are intentionally skipping some validators that aren't really relevent or are uh bit of out of scope for core - like the one that checks password hashes against the HaveIBeenPwned database (aka going to an external service). We would need/could easily have a skipped validators list.

If it helps get confidence for including it now, I'd be very open to writing such a test, but personally thinks that that test would add minimal value.

catch’s picture

Auto-registering with a skip list is tempting, although then we'd get any new validators added any time we update, so maybe the current include list is better anyway?

borisson_’s picture

Auto-registering with a skip list is tempting, although then we'd get any new validators added any time we update, so maybe the current include list is better anyway?

I agree with this, is there a checklist of things we check when updating to a new symfony version, we should probably add "check for new validators" to that list?

longwave’s picture

There isn't really a checklist that anyone is guaranteed to follow, this is why it's a nice idea to have a test that catches it.

dww’s picture

StatusFileSize
new11.34 KB
new1.66 KB

While we decide if/what we're going to do here, I fixed some capitalization nits in the labels. 😅 Leaving RTBC since core committers really need to decide what's next.

Some other nitty review thoughts:

  1. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -87,22 +131,222 @@ public function create($name, $options) {
    +      'label' => new TranslatableMarkup('Business Identifier Code'),
    

    If this were "BIC" as an acronym, it'd be all caps. But does each word need to be capitalized like this?

    Also, this is the only acronym that we spell out. Should we leave this as just "BIC"?

    Or both: "Business identifier code (BIC)"?

  2. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -87,22 +131,222 @@ public function create($name, $options) {
    +      'label' => new TranslatableMarkup('CIDR'),
    

    If we're going to "unwind" BIC at all, should we do the same for these others (CIDR, ISBN, etc)?

  3. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
    @@ -87,22 +131,222 @@ public function create($name, $options) {
    +      'label' => new TranslatableMarkup('Luhn'),
    

    I confess to not knowing about https://en.wikipedia.org/wiki/Luhn_algorithm until now. 🤓 Would "Luhn checksum" or "Luhn algorithm" be a more self-documenting label for this one?

Thanks!
-Derek

longwave’s picture

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

#24 doesn't apply and needs rerolling.

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

longwave’s picture

#24.1 the ISO standard seemingly uses the title case "Business Identifier Code" as per https://en.wikipedia.org/wiki/ISO_9362 so we probably should as well.

#24.2 I think CIDR could be qualified a bit more, something like "CIDR range"?

#24.3 I like "Luhn checksum" here but open to suggestions.

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
@@ -87,22 +131,222 @@ public function create($name, $options) {
-      'label' => new TranslatableMarkup('Email'),
...
+      'label' => new TranslatableMarkup('E-mail'),

This is incorrect, see #3051548: Fix spelling of "email"

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

I rerolled the patch and applied the #27's suggestions. Please review.

g089h515r806’s picture

Status: Needs review » Needs work

1, Most symfony constraints can be used at Drupal directly, but not all, Isin is the only one that not works in the list, I test them manually in field validation 3.0.0 version. We will get fatal error when using Isin constraint and populate a valid value for example "US0378331005".

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 at :https://github.com/symfony/symfony/issues/51440.

IsinValidator use following code in isCorrectChecksum function :

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

drupal has own typed data validator base on symfony validator. Drupal use:

core\lib\Drupal\Core\TypedData\Validation\RecursiveContextualValidator

Instead of symfony's. At here $number is not a typed data. It throw a fatal exception.

Negative is inherit from LessThan, Email use regex, If symfony validator author change the code, and use following logic in Negative's validate function, designate part of validate logic to another constraint, for example:

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

It still works for most normal symfony project, but it does not works at Drupal. We can not prevent them do similar change, symfony's maintainer think this is drupal core bug, Drupal core's typed data validator should following symfony way instead of hack it.

2, If a constraint has a message, and the message has a token, symfony use a different way for text translation, here is the example:


/**
 * NotEqualTo constraint.
 *
 * @Constraint(
 *   id = "EqualTo",
 *   label = @Translation("EqualTo", context = "Validation"),
 * )
 */
class EqualToConstraint extends EqualTo {

  public $message = 'This value should be equal to %compared_value.';
  /**
   * {@inheritdoc}
   */
  public function validatedBy(): string {
    return EqualToValidator::class;
  }

}

in symfony, it is :

public $message = 'This value should be equal to {{ compared_value }}.';

We have to extends the EqualTo, and override the message string. I am not sure Drupal core 11.x support {{ compared_value }} token, if it does not support, EqualTo, LessThan, GreaterThan have this issue, we have to extends them instead register them directly.

3, Language, Locale, constraints need install symfony/intl and PHP's intl extension, we need change the composer.json to support them and change Docs to let user install PHP's intl extension.

4, Because of 1, We need write test for each constraint to make sure they work correctly in Drupal.

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

nireneko’s picture

Like suggested by @g089h515r806, overrided some constraints to replace the message strings.

This is the list of Symfony constraints with dependency on intl component:

  • Bic
  • Currency
  • Language
  • Locale
  • Timezone

I didn't touch that constraints because we must add a new requirement in composer.json, the php extension "intl", and I think, that should be done in Drupal 11, no in 10.X ?

borisson_’s picture

Status: Needs work » Needs review

Tests are green again, back to needs review.

smustgrave’s picture

Status: Needs review » Needs work

So when I rebuilt the dictionary.txt file I get a few more changes. Notably a number of deletions.

borisson_’s picture

I hope I did the right thing, updated dictonary.txt.

borisson_’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Believe you did! Not sure what the proper workflow is.

Like if another issue lands with dictionary changes. Does the manual merge work or does it need to be generated?

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

marvil07’s picture

Status: Needs work » Needs review

I merged in mainline at commit d797300b9ec841621a32d3efa2f84657ac46c012, so branch applies cleanly again.
One trivial merge conflict solved on cspell dictionary, union merge.
Back to NR.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc now that it is rerolled.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

Some of the dictionary changes look a bit suspect: dflydev, grasmash, phpowermove, psysh, robo look like the dictionary was not generated from a clean environment.

Instead of adding words to the dictionary it is OK to add cspell:ignore lines to the file(s) where the new words are used.

marvil07’s picture

Status: Needs work » Needs review

Some of the dictionary changes look a bit suspect: dflydev, grasmash, phpowermove, psysh, robo look like the dictionary was not generated from a clean environment.

@longwave, indeed, those changes seem to be unrelated.
They may be related to recent changes on mainline, and there seems to be quite some changes there.

$ git log --oneline --since="2 months ago" 11.x -- core/misc/cspell/
7e75964e69 Issue #3395891 by quietone: Fix spelling for words in test modules
a10e0f8d1f Issue #3328741 by quietone, smustgrave, xjm, catch, alexpott, longwave: Add a dictionary for Drupal-specific words
3c1a449c37 Issue #3391788 by quietone, xjm: Fix spelling of function names in tests
0940ba003b Issue #3399123 by Spokje, longwave, smustgrave: Upgrade JavaScript dependencies to latest minor/patch releases
71f949cb0b Issue #3395913 by quietone: Correct spelling in Config tests
0852886ca3 Issue #3368102 by finnsky, Gauravvvv, smustgrave, lauriii, e0ipso, andypost, mrinalini9: Create new SDC component for Umami (umami common footer block)
c7e751a532 Issue #3379121 by quietone, longwave: Skip spellcheck for 20 nonsense words that are parts of hashes
41b777c6be Issue #3395871 by quietone: Correct spelling of words in test modules and dramallama
cb123cf554 Issue #3392425 by quietone, smustgrave: Fix variable name spelling in non-tests, part1
36ab456505 Issue #3389283 by quietone: Fix spelling of words only misspelled in tests, part 1
eafbd246a9 Issue #3394093 by quietone: Correct spelling of corefake
6118e0dc37 Issue #3389282 by quietone, smustgrave, alexpott: Fix spelling for words that are only misspelled  in comments
ae55070f94 Issue #3391268 by quietone: Fix spelling of words only misspelled in tests, part 4
b78ac3f08a Issue #3390955 by quietone, smustgrave: Fix spelling of words only misspelled in tests, part 3
a8f52aa253 Issue #3352459 by catch, Bhanu951, Wim Leers, smustgrave, Fabianx, heddn, alexpott: Add OpenTelemetry Application Performance Monitoring to core performance tests
987548947c Issue #3389286 by quietone: Fix spelling of words only misspelled in tests, part 2
dbb54acf6e Issue #3312072 by fjgarlin, penyaskito, markconroy, lauriii, smustgrave, ckrina, Spokje: Display category-related recipes when seeing a recipe full page
8224c90c08 Issue #3387339 by bbrala, longwave: Integrate codequality reports into Gitlab

I went on and revert the dictionary change, and merged mainline again, to be sure to be on the latest status.
Then, I ran the spell check, and it still passed.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 0 in 0 files

There are actually some valid additions, that are related to the newly added validators.
I reverted all additions to the dictionary, and the spell check did not pass, as expected.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 9 in 1 files

Looking into the details, it is all coming from the added core/lib/Drupal/Core/Validation/ConstraintManager.php.

$ yarn cspell -c .cspell.json --root .. "core/lib/Drupal/Core/Validation"
yarn run v1.22.19
$ core/node_modules/.bin/cspell -c .cspell.json --root .. 'core/lib/Drupal/Core/Validation'
 1/40 ./lib/Drupal/Core/Validation/Annotation/Constraint.php 299.41ms
 2/40 ./lib/Drupal/Core/Validation/BasicRecursiveValidatorFactory.php 19.78ms
 3/40 ./lib/Drupal/Core/Validation/ConstraintFactory.php 16.03ms
 4/40 ./lib/Drupal/Core/Validation/ConstraintManager.php 83.91ms X
./core/lib/Drupal/Core/Validation/ConstraintManager.php:26:45 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:31:45 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:169:42 - Unknown word (IBAN)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:193:43 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:194:42 - Unknown word (ISSN)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:195:18 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:218:43 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:219:42 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:220:18 - Unknown word (Luhn)
 5/40 ./lib/Drupal/Core/Validation/ConstraintValidatorFactory.php 20.18ms
 6/40 ./lib/Drupal/Core/Validation/DrupalTranslator.php 35.70ms
 7/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraint.php 9.90ms
 8/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php 48.61ms
 9/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CidrConstraint.php 12.12ms
10/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraint.php 32.69ms
11/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php 9.25ms
12/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CountConstraint.php 7.68ms
13/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/DivisibleByConstraint.php 7.41ms
14/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EmailConstraint.php 6.89ms
15/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EmailValidator.php 7.49ms
16/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/GreaterThanConstraint.php 7.71ms
17/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/GreaterThanOrEqualConstraint.php 10.27ms
18/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IdenticalToConstraint.php 6.43ms
19/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ImageConstraint.php 16.00ms
20/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IsNullConstraint.php 6.39ms
21/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IsNullConstraintValidator.php 8.05ms
22/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php 11.36ms
23/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LessThanConstraint.php 7.69ms
24/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LessThanOrEqualConstraint.php 15.47ms
25/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotEqualToConstraint.php 8.93ms
26/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotIdenticalToConstraint.php 6.50ms
27/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotNullConstraint.php 5.44ms
28/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotNullConstraintValidator.php 7.44ms
29/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraint.php 5.48ms
30/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php 16.61ms
31/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RangeConstraint.php 8.29ms
32/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RangeConstraintValidator.php 14.08ms
33/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RegexConstraint.php 8.47ms
34/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/TypeConstraint.php 6.32ms
35/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldConstraint.php 6.95ms
36/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php 28.14ms
37/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UuidConstraint.php 5.73ms
38/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php 17.82ms
39/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php 14.34ms
40/40 ./lib/Drupal/Core/Validation/TranslatorInterface.php 7.78ms
CSpell: Files checked: 40, Issues found: 9 in 1 files

I agree that ignoring the few instances on the one expected file using it makes sense.
So, I have pushed the following changes to the topic branch.

- a4cc65801f Revert "Update cspell dictionary"
- e44f62a124 Get mainline changes
- 830cae3bb9 Revert actual additions to the dictionary
- fb8d79efa0 Ignore spelling for three symfony validator specific words

Locally, after the changes it looks like the following.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 0 in 0 files

Let us see if the test run is OK too.

marvil07’s picture

It took an extra bit to make the cspell change OK from phpcs perspective :-)
Now I see CI approving ;-)

g089h515r806’s picture

I remind this again:
Symfony Isin constraint(Symfony\Component\Validator\Constraints\Isin) could not be used in Drupal directly.

I suggest remove it or replace it with Drupal's version.

See my comment at https://www.drupal.org/project/drupal/issues/3365498#comment-15200789.

I am the author of "field validation" module, I did the same work "Add Missing Symfony Validators to Drupal Constraint Validator" in field validation 3.0.0, Isin is the only constraint that could be used directly in Drupal. This is why it not work:

https://github.com/symfony/symfony/issues/51440

catch’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I don't feel like #15 has been addressed here, it's adding a huge amount of API surface directly from Symfony, with no real indication that these are going to be used anywhere.

What happens when Symfony removes one of these, changes the API etc., are we going to have to start to add bc layers ourselves? We've already got enough API churn from this component as it is.

marvil07’s picture

Direction

I don't feel like #15 has been addressed here, it's adding a huge amount of API surface directly from Symfony, with no real indication that these are going to be used anywhere.

@catch, yes, the original intention here is to make general symfony validators available as constraint plugins, so they can be used; but they have not been used directly anywhere, at least yet.

What happens when Symfony removes one of these, changes the API etc., are we going to have to start to add bc layers ourselves? We've already got enough API churn from this component as it is.

That is fair to say, I appreciate this surfacing again and reminding the possible consequences.
I have not followed closely how symfony validator has impacted core, since it was added.

From what it is mentioned, it sounds like the possible impact can be more expensive than not exposing them by core.
Maybe the right solution for core is just to not expose them, or at least not all of them, and leave the rest for contrib modules like field_validation.

Already there

There is already a subset of symfony constraint, or at least the related validators, that is exposed by core.
This issue is about exposing all the rest.

A static set added atDrupal\Core\Validation\ConstraintManager::registerDefinitions().

And the set at core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/, excluding custom constraints directly extending Symfony\Component\Validator\Constraint.

  • AllowedValuesConstraint, extends Choice; along with AllowedValuesConstraintValidator, that extends ChoiceValidator.
  • CountConstraint, extends Count.
  • EmailConstraint extends Email, and uses EmailValidator, as mentioned on the previous set.
  • IsNullConstraint, extends IsNull, using a custom IsNullConstraintValidator on top of symfony one.
  • LengthConstraint, extends Length.
  • NotNullConstraint, extends NotNull, using a custom NotNullConstraintValidator.
  • RangeConstraint, extends Range, using RangeConstraintValidator.
  • RegexConstraint, extends Regex.
  • UuidConstraint, extends Uuid.

In other words, currently it seems core exposes 13 symfony constraints to be used, of the 71 that symfony currently provides.

Next steps

Likely, a decision about what to do needs to be taken.

Taking into account the suggested upgrade problems from the near past as stated on #15, it sounds like the best approach may be to not take any action here, and close this issue.

That may avoid some of the possible burden of needing to add compatibility layers if upstream decides to change not trivially, or at least reduce it, since core already exposes 13 of the symfony validators, directly or indirectly.

catch’s picture

There's some discussion (a mixture of old issues and current ones) in #3054535: Discuss whether to decouple from Symfony Validator. The validators themselves haven't necessary been the barrier to updating Symfony versions, a lot of it's been 'internal' Symfony classes that we nonetheless have to subclass, but that also could be because we're not exposing that many yet.

2dareis2do’s picture

Is this related to issue where if config value is set to null you get a UnexpectedTypeException?

e.g. https://www.drupal.org/project/config_inspector/issues/3416934#comment-1... and https://www.drupal.org/project/search_api_solr/issues/3415861

Certain values are expected are set as null, but ValidKeysConstraintValidator is expecting it to be an array.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.