- Issue created by @nickdickinsonwilde
- 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
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. - 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
Done with discussion with Wim Leers and Rosie La Faive
- Status changed to Needs review
over 1 year ago 9:18pm 7 June 2023 - last update
over 1 year ago Custom Commands Failed - Status changed to Needs work
over 1 year ago 11:02am 8 June 2023 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.
- Status changed to Needs review
over 1 year ago 1:07pm 8 June 2023 - last update
over 1 year ago 29,421 pass, 3 fail - 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
Never ran cspell locally! Let's try this...
The last submitted patch, 7: 3365498-add-validators-7.patch, failed testing. View results →
- last update
over 1 year ago 29,436 pass - 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
Fix a couple duplications
- Status changed to Needs work
over 1 year ago 3:38pm 8 June 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Looks GREAT!
Could you please explain why this is a contributed project blocker? Where did you encounter a need for this? 😊
-
+++ 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! :)
-
+++ 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? 😅🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
+++ 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 😅
-
+++ 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" 🤓
-
+++ 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 🤓
-
+++ 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" 🤓
-
+++ 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" 🤓
-
- Status changed to Needs review
over 1 year ago 4:01pm 8 June 2023 - last update
over 1 year ago 29,436 pass - 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
Patch updated with adjustments as per #11.
Note, also changed existing few definitions to use formatBic::class
rather than string of current FQCN - Status changed to RTBC
over 1 year ago 4:06pm 8 June 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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!
- last update
over 1 year ago 29,437 pass - 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
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 - 🇬🇧United Kingdom catch
I have one concern here which boils down to 📌 Discuss whether to decouple from Symfony Validator Active - 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?
- 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
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?
- 🇬🇧United Kingdom catch
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?
- 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
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.
- last update
over 1 year ago 29,442 pass - 🇬🇧United Kingdom longwave UK
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?
- 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
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.
- 🇬🇧United Kingdom catch
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?
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
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?
- 🇬🇧United Kingdom longwave UK
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.
- last update
over 1 year ago 29,448 pass - last update
over 1 year ago 29,486 pass - last update
over 1 year ago 29,499 pass - last update
over 1 year ago 29,499 pass - 🇺🇸United States dww
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:
-
+++ 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)"?
-
+++ 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)?
-
+++ 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 -
- last update
over 1 year ago 29,505 pass - last update
over 1 year ago 29,551 pass - last update
over 1 year ago 29,553 pass - last update
over 1 year ago 29,559 pass - last update
over 1 year ago 29,563 pass - last update
over 1 year ago 29,571 pass - last update
over 1 year ago 29,801 pass - last update
over 1 year ago 29,801 pass - last update
over 1 year ago 29,802 pass - last update
over 1 year ago 29,802 pass - last update
over 1 year ago 29,807 pass - last update
over 1 year ago 29,811 pass - last update
over 1 year ago 29,815 pass - last update
over 1 year ago 29,815 pass - last update
over 1 year ago 29,822 pass - last update
over 1 year ago 29,842 pass - last update
over 1 year ago Patch Failed to Apply - Status changed to Needs work
over 1 year ago 4:14pm 24 July 2023 - First commit to issue fork.
- 🇬🇧United Kingdom longwave UK
#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 📌 Fix spelling of "email" Fixed
- last update
over 1 year ago 29,961 pass - @rpayanm opened merge request.
- last update
over 1 year ago 29,961 pass - Status changed to Needs review
over 1 year ago 7:21pm 16 August 2023 - Status changed to Needs work
over 1 year ago 1:15am 22 August 2023 - 🇨🇳China g089h515r806
1, Most constraints could used at Drupal, but not all, Isin is the only one that not works, I test them manually in field validation 3.0.0 version. We will get fatal error when using Isin constraint.
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.
This is the only exception.
Negative is inherit from LessThan, Email use Regex, If symfony validator author change the code, and use 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 at all. We can not prevent them do this change.
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.
- First commit to issue fork.
- last update
about 1 year ago Custom Commands Failed - 🇪🇸Spain nireneko
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 ?
- Status changed to Needs review
about 1 year ago 12:00pm 31 October 2023 - 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
Tests are green again, back to needs review.
- Status changed to Needs work
about 1 year ago 7:15pm 31 October 2023 - 🇺🇸United States smustgrave
So when I rebuilt the dictionary.txt file I get a few more changes. Notably a number of deletions.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I hope I did the right thing, updated dictonary.txt.
- Status changed to Needs review
about 1 year ago 11:23am 1 November 2023 - Status changed to RTBC
about 1 year ago 2:46pm 2 November 2023 - 🇺🇸United States smustgrave
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?
- last update
about 1 year ago 30,485 pass - last update
about 1 year ago 30,486 pass - last update
about 1 year ago 30,488 pass - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
about 1 year ago Not currently mergeable. - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
about 1 year ago Not currently mergeable. - Status changed to Needs work
about 1 year ago 5:39pm 12 November 2023 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.
- First commit to issue fork.
- Status changed to Needs review
about 1 year ago 2:34pm 15 November 2023 - 🇵🇪Peru marvil07
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. - Status changed to RTBC
about 1 year ago 1:44pm 16 November 2023 - last update
about 1 year ago 30,554 pass - Status changed to Needs work
about 1 year ago 2:32pm 16 November 2023 - 🇬🇧United Kingdom longwave UK
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. - Status changed to Needs review
about 1 year ago 5:12pm 16 November 2023 - 🇵🇪Peru marvil07
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 wordsLocally, 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.
- 🇵🇪Peru marvil07
It took an extra bit to make the cspell change OK from phpcs perspective :-)
Now I see CI approving ;-) - 🇨🇳China g089h515r806
Symfony Isin constraint could not be used in Drupal directly:
use Symfony\Component\Validator\Constraints\Isin;
I remind this again.
- Status changed to Needs work
about 1 year ago 10:39am 21 November 2023 - 🇬🇧United Kingdom catch
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.
- 🇵🇪Peru marvil07
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().
- Callback
- Blank
- NotBlank
- Email (extended at
Drupal\Core\Validation\Plugin\Validation\Constraint\EmailConstraint
- Choice
And the set at
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/
, excluding custom constraints directly extendingSymfony\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.
- 🇬🇧United Kingdom catch
There's some discussion (a mixture of old issues and current ones) in 📌 Discuss whether to decouple from Symfony Validator Active . 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.
- 🇬🇧United Kingdom 2dareis2do
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... 🐛 Config inspector stopped working after Drupal 10.2 update Active and https://www.drupal.org/project/search_api_solr/issues/3415861 🐛 ValidKeysConstraintValidator thrown by config inspector Active
Certain values are expected are set as null, but ValidKeysConstraintValidator is expecting it to be an array.