Needs work
Project:
Drupal core
Version:
main
Component:
configuration system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Jun 2023 at 21:14 UTC
Updated:
13 Feb 2024 at 20:31 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
nickdickinsonwildePatch attached for review.
I believe that since this is just making the symfony validators accessible to Drupal core that no tests are directly needed.
Comment #3
nickdickinsonwildeDone with discussion with Wim Leers and Rosie La Faive
Comment #4
nickdickinsonwildeComment #5
needs-review-queue-bot commentedThe 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.
Comment #6
borisson_Based on #5, we should probably tell cspell that luhn, issn and iban are actually correct words?
Comment #7
nickdickinsonwildeNever ran cspell locally! Let's try this...
Comment #9
nickdickinsonwildeFix a couple duplications
Comment #10
wim leersLooks GREAT!
Could you please explain why this is a contributed project blocker? Where did you encounter a need for this? 😊
Could you update these to be of the format
Bic::class? 🙏That way any future refactoring would be simpler! :)
I think these should all be capitalized? 😅🤓
I think the label should be "CSS Color" 🤓
I think the label should be "IBAN" 🤓
I think the label should be "IP address" 🤓
I think the label should be "ISBN" 🤓
I think the label should be "ISIN" 🤓
I think the label should be "JSON" 🤓
🤯 Holy crap!
This uses https://haveibeenpwned.com/API/v2#SearchingPwnedPasswordsByRange, which is an external API. Let's omit this one 😅
I think the label should be "Not identical to" 🤓
I think the label should be "Positive or zero", for consistent capitalization with other similar labels 🤓
I think the label should be "ULID" 🤓
I think the label should be "URL" 🤓
Comment #11
nickdickinsonwildePatch updated with adjustments as per #11.
Note, also changed existing few definitions to use format
Bic::classrather than string of current FQCNComment #12
wim leers🤩
Comment #13
wim leersOh, arguably we should remove
and instead do that here too 🤓
I'm curious what release managers think!
Comment #14
borisson_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
Comment #15
catchI 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?
Comment #16
nickdickinsonwildeHmmm. 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?
Comment #17
catchWe'd need to subclass the Symfony plugins in order to be able to do that ... maybe that is fine?
Comment #18
nickdickinsonwildeYeah, 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.
Comment #19
longwaveIs 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?
Comment #20
nickdickinsonwildehmm... 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.
Comment #21
catchAuto-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?
Comment #22
borisson_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?
Comment #23
longwaveThere 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.
Comment #24
dwwWhile 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:
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)"?
If we're going to "unwind" BIC at all, should we do the same for these others (CIDR, ISBN, etc)?
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
Comment #25
longwave#24 doesn't apply and needs rerolling.
Comment #27
longwave#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.
This is incorrect, see #3051548: Fix spelling of "email"
Comment #29
rpayanmI rerolled the patch and applied the #27's suggestions. Please review.
Comment #30
g089h515r806 commented1, 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".
I report the issue at :https://github.com/symfony/symfony/issues/51440.
IsinValidator use following code in isCorrectChecksum function :
drupal has own typed data validator base on symfony validator. Drupal use:
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:
in symfony, it is :
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.
Comment #32
nireneko commentedLike suggested by @g089h515r806, overrided some constraints to replace the message strings.
This is the list of Symfony constraints with dependency on intl component:
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 ?
Comment #33
borisson_Tests are green again, back to needs review.
Comment #34
smustgrave commentedSo when I rebuilt the dictionary.txt file I get a few more changes. Notably a number of deletions.
Comment #35
borisson_I hope I did the right thing, updated dictonary.txt.
Comment #36
borisson_Comment #37
smustgrave commentedBelieve 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?
Comment #38
needs-review-queue-bot commentedThe 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.
Comment #40
marvil07 commentedI merged in mainline at commit d797300b9ec841621a32d3efa2f84657ac46c012, so branch applies cleanly again.
One trivial merge conflict solved on cspell dictionary, union merge.
Back to NR.
Comment #41
borisson_Back to rtbc now that it is rerolled.
Comment #42
longwaveSome 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:ignorelines to the file(s) where the new words are used.Comment #43
marvil07 commented@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.
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.
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.
Looking into the details, it is all coming from the added
core/lib/Drupal/Core/Validation/ConstraintManager.php.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.
Let us see if the test run is OK too.
Comment #44
marvil07 commentedIt took an extra bit to make the cspell change OK from phpcs perspective :-)
Now I see CI approving ;-)
Comment #45
g089h515r806 commentedI 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
Comment #46
catchI 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.
Comment #47
marvil07 commentedDirection
@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.
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().
Drupal\Core\Validation\Plugin\Validation\Constraint\EmailConstraintAnd the set at
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/, excluding custom constraints directly extendingSymfony\Component\Validator\Constraint.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.
Comment #48
catchThere'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.
Comment #49
2dareis2do commentedIs 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.