Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
system.date has 4 property path that are not yet validatable:
./vendor/bin/drush config:inspect --filter-keys=system.date --detail --list-constraints
➜ 🤖 Analyzing…
Legend for Data:
✅❓ → Correct primitive type, detailed validation impossible.
✅✅ → Correct primitive type, passed all validation constraints.
----------------------------------------- --------- ------------- ------ ------------------------------------------
Key Status Validatable Data Validation constraints
----------------------------------------- --------- ------------- ------ ------------------------------------------
system.date Correct 67% ✅❓ ValidKeys: '<infer>'
system.date: Correct Validatable ✅✅ ValidKeys: '<infer>'
system.date:_core Correct Validatable ✅✅ ValidKeys:
- default_config_hash
system.date:_core.default_config_hash Correct Validatable ✅✅ NotNull: { }
Regex: '/^[a-zA-Z0-9\-_]+$/'
Length: 43
↣ PrimitiveType: { }
system.date:country Correct Validatable ✅✅ ValidKeys: '<infer>'
system.date:country.default Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.date:first_day Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.date:timezone Correct Validatable ✅✅ ValidKeys: '<infer>'
system.date:timezone.default Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.date:timezone.user Correct Validatable ✅✅ ValidKeys: '<infer>'
system.date:timezone.user.configurable Correct Validatable ✅✅ ↣ PrimitiveType: { }
system.date:timezone.user.default Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.date:timezone.user.warn Correct Validatable ✅✅ ↣ PrimitiveType: { }
----------------------------------------- --------- ------------- ------ ------------------------------------------
Steps to reproduce
- Get a local git clone of Drupal core
11.x. composer require drupal/config_inspector— or manually install https://www.drupal.org/project/config_inspector/releases/2.1.5 or newer (which supports Drupal 11!)composer require drush/drushvendor/bin/drush config:inspect --filter-keys=system.date --detail --list-constraints
Proposed resolution
Add validation constraints to:
system.date:country.defaultsystem.date:first_daysystem.date:timezone.defaultsystem.date:timezone.user.default
For examples, search *.schema.yml files for the string constraints: 😊
Reach out to @borisson_ or @wimleers in the #distributions-and-recipes.
Remaining tasks
User interface changes
None.
API changes
Data model changes
More validation 🚀
Release notes snippet
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3437325-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #17 | 3437325-nr-bot.txt | 2.21 KB | needs-review-queue-bot |
| #13 | 3437325-nr-bot.txt | 2.21 KB | needs-review-queue-bot |
| #11 | 3437325-nr-bot.txt | 2.21 KB | needs-review-queue-bot |
| #7 | 3437325-nr-bot.txt | 2.21 KB | needs-review-queue-bot |
Issue fork drupal-3437325
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:
- 3437325-system-date
changes, plain diff MR !7271 - 3437325-add-validation-constraints compare
Comments
Comment #2
narendrarComment #3
narendrarComment #6
narendrarComment #7
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 necessarily 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 #8
wim leersInteresting failures:
and
This is looking very close though! :)
Comment #10
narendrarComment #11
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 necessarily 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 #12
narendrarComment #13
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 necessarily 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 #14
phenaproximaNo, bot, it does not need work.
Comment #15
phenaproximaOnly one tiny comment, then I don't really see a problem here.
Comment #16
narendrarComment #17
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 necessarily 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 #18
wim leers#3439439: Remove country setting from the installer landed today! It conflicts with this MR.
I don't see how this could land without at least an update path — fortunately it's a trivial one! I suspect we'll also need explicit tests for
\Drupal\system\Form\RegionalForm.Comment #19
narendrarComment #20
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 necessarily 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 #21
phenaproximaComment #22
wim leersGood progress!
In the final stretch to the finish line now :)
Problems I spotted in review:
timezone.user.defaultis misleading — easily fixed 👍P.S.: please push #2951046: Allow parsing and writing PHP class constants and enums in YAML files forward so that we can land #3402178: [PP-1] Enum cases stored in config — that'd make validation of
timezone.user.defaultin this issue simpler/more maintainable/clearer. It'd also help many other (config) validation issues!Comment #23
narendrarComment #24
phenaproximaSome minor points, but this generally looks pretty good to me.
Comment #25
narendrarComment #26
phenaproximaShip it!
Comment #27
alexpott\Drupal\system\Form\RegionalForm::getEditableConfigNames can be removed and the form can use \Drupal\Core\Form\RedundantEditableConfigNamesTrait now.
Comment #28
narendrarComment #29
wim leers3 nits, one of which seems indisputably an improvement, so applied that. The other 2 I'll leave to the core committer reviewing this.
Other than that: looks great! 😊
Comment #30
alexpottI don't think we should be suing static methods and \Drupal::service - see MR comments.
Also do we have any BC concerns about moving from empty string to NULLs?
Comment #31
phenaproximaComment #32
wim leershttps://git.drupalcode.org/project/drupal/-/merge_requests/7271/diffs?co... → 🤩
Too bad you reverted and went with a custom constraint in the end. I think what you did originally would've been more broadly applicable.
OTOH … what you've done now is declarative and is easier to interpret on the client side! This is now essentially a validator for https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2. It's similar to Symfony's
\Symfony\Component\Validator\Constraints\Country, but that depends onsymfony/intl, which Drupal core does not use.Conclusion: this change is 👍😄
P.S.: I think a change record to allow others to use the new
CountryCodevalidation constraint would be great 😊 But that can easily happen post-commit.Comment #33
phenaproximaYeah, I was disappointed I had to revert also. But I realized that there was a very subtle bug in my original approach, just waiting to give somebody a splitting headache. The problem is that
getAllCountryCodes()is not part ofCountryManagerInterface. So the validation constraint was coupled to a non-interface service method.If someone (or some module) swapped out core's
CountryManagerfor a different implementation ofCountryManagerInterface-- one without the validation method -- they would have run into some very obscure, hard-to-trace errors.In light of that, I decided it was best to go with a custom constraint that wraps specifically around
CountryManagerInterface::getList(), which is an interface method and therefore won't break if the implementation is swapped out.I agree that making
Choiceaware of the callable resolver is likely to be useful in the future, and as soon as we have a concrete case for it, I'll happily restore it. :)Comment #34
alexpottI yhtink the test is in the wrong location. I suggested a kernel test but a unit test would work just as well i think
Comment #35
wim leers#33 is a great point — it made swapping out the default implementation impossible indeed! 👍
The unit test @alexpott requested already passed, so re-RTBC'ing even before CI is completely done.
P.S.: change record? 🤞
Comment #36
phenaproximaDone. https://www.drupal.org/node/3441838
Comment #37
alexpottCommitted and pushed 8cd02c7f00 to 11.x and 338d4d046a to 10.3.x. Thanks!
Comment #40
wim leersWow, excellent call, @phenaproxima! 👏
Comment #42
berdirThis breaks commerce hard, because it expects that country is a string, so this change results in a fatal error there atm. See #3173772: TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given. While it can easily be fixed there and the change makes sense anyway, instead of returning a Country object with an empty country string, this is still a pretty hard BC break.
Comment #43
wim leers#42: I don't think that's entirely accurate; that module does something rather special: #3173772-15: TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given — it uses core config as if it was its own config (with schema it controls). I do not think that is a widespread practice?
If anything, after this (somewhat painful — although the changes in the module seem trivial) transition, any module doing something like this will have stronger guarantees after the value is being strictly validated: they'll know much more precisely what data to expect.
Comment #44
berdirThe combined usage with the strict type type is special, but using that config is not.
From your comment in the other issue:
> IOW: this is all caused by this module using core config as if it was this module's config.
That's not a thing. Drupal has the concept of a default country, and reading this config file is the only way to access it. There is no rule that you're not allowed to read config of other modules.
Using config like that is common. See https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22get%2...
https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... is IMHO a bit confusing, as it then only lists composer related config, but I understand that as changing/removing config structure is covered by our BC policy.
Comment #45
wim leersHm, good point! 😬
What path forward do you propose?
Comment #46
berdir> What path forward do you propose?
I don't know. The commerce issue has been fixed and will be available in a release soon. It's now in the alpha release, so I guess reverting this would just cause a bigger confusion. Might hit people trying out the alpha release, but nothing can be done about that at this point I guess except getting out a new commerce release out asap.