Problem/Motivation
system.site has 5 property path that are not yet validatable:
vendor/bin/drush config:inspect --filter-keys=system.site --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.site Correct 69% ✅❓ ValidKeys: '<infer>'
LangcodeRequiredIfTranslatableValues: null
system.site: Correct Validatable ✅✅ ValidKeys: '<infer>'
LangcodeRequiredIfTranslatableValues: null
system.site:_core Correct Validatable ✅✅ ValidKeys:
- default_config_hash
system.site:_core.default_config_hash Correct Validatable ✅✅ NotNull: { }
Regex: '/^[a-zA-Z0-9\-_]+$/'
Length: 43
↣ PrimitiveType: { }
system.site:admin_compact_mode Correct Validatable ✅✅ ↣ PrimitiveType: { }
system.site:default_langcode Correct Validatable ✅✅ NotNull: { }
Choice:
callback: 'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllValidLangcodes'
↣ PrimitiveType: { }
system.site:langcode Correct Validatable ✅✅ NotNull: { }
Choice:
callback: 'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllValidLangcodes'
↣ PrimitiveType: { }
system.site:mail Correct Validatable ✅✅ Email:
message: '%value is not a valid email address.'
↣ PrimitiveType: { }
system.site:mail_notification Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.site:name Correct Validatable ✅✅ Regex:
pattern: '/([^\PC])/u'
match: false
message: 'Labels are not allowed to span multiple lines or contain control characters.'
↣ PrimitiveType: { }
system.site:page Correct Validatable ✅✅ ValidKeys: '<infer>'
system.site:page.403 Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.site:page.404 Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.site:page.front Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
system.site:slogan Correct Validatable ✅✅ Regex:
pattern: '/([^\PC])/u'
match: false
message: 'Labels are not allowed to span multiple lines or contain control characters.'
↣ PrimitiveType: { }
system.site:uuid Correct Validatable ✅✅ Uuid: { }
NotNull: { }
↣ PrimitiveType: { }
system.site:weight_select_max Correct NOT ✅❓ ⚠️ @todo Add validation constraints here
---------------------------------------- --------- ------------- ------ ---------------------------------------------------------------------------------------------
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/drush
vendor/bin/drush config:inspect --filter-keys=system.site --detail --list-constraints
Proposed resolution
Add validation constraints to:
system.site:mail_notification
system.site:page.403
system.site:page.404
system.site:page.front
system.site:weight_select_max
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.
Comments
Comment #2
narendrarComment #5
mikelutzComment #6
mikelutzComment #7
carsoncho commentedThis all looks correct to me. Config inspector is reporting this at 100% now. Is the reason this is back to Needs work because of the pipelines failing?
Comment #8
narendrarComment #9
smustgrave commentedSo applied the MR locally.
Update hook ran just fine
I'm still able to update the settings without issue
The proposed schema changes believe make sense.
+1 from me.
Comment #10
quietone commentedI read the IS and the MR. There are new methods in the MR that should be documented.
Comment #11
carsoncho commentedI've added docblock info for those new methods. Ready for review.
Comment #12
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 #13
mikelutzFixed coding standards and rebased to see if the validation checker can resolve updated dependencies.
Comment #14
smustgrave commentedLeft 1 question on the MR.
Comment #15
narendrarFeedback addressed.
Comment #16
smustgrave commentedBelieve feedback has been addressed.
Comment #17
alexpottI've added some comments to the MR and 1 that I think should have a follow up created for.
Comment #18
borisson_Followup created: #3457297: [PP-1] Add PHP_INT_MAX to integer schema configuration
Comment #19
narendrarTest https://git.drupalcode.org/issue/drupal-3443432/-/jobs/2012360#L1253 was failing due to change in https://git.drupalcode.org/project/drupal/-/merge_requests/7723/diffs?commit_id=bc72ebbe5abb48bc994fc4df237eda7c30f37b51#01036d2820f2e116fab98a13152a9294069e8f55_116_114, hence reverted.
Comment #20
kunal.sachdev commentedLooks good. Just have one minor doubt.
Comment #21
alexpottremoving the UUID constraint feels like a step in the wrong direction. NULL is never really a valid value for site UUID.
Comment #22
phenaproximaThanks, @narendraR; that makes sense.
However, I agree with @alexpott that NULL is never valid for the site UUID, and so it shouldn't be nullable.
Comment #23
alexpottI think this issue points to us needing a new constraint which is something like NotBlankAfterInstall - so a very can be blank during site install but once the site is up and running it can not be. This is true for uuid, name and email.
I also think that none of these should be nullable. PHP 8 makes swapping between NULLs and strings hard.
Comment #24
narendrar@alexpott, I tried to create a new constraint as suggested in #23. But I don't understand how to bypass existing
NotNullConstraintfor UUID. https://git.drupalcode.org/project/drupal/-/merge_requests/7723/diffs?commit_id=4fcb5099b2b23f22643948c901321db04b89b0db#4d658554b6066a85600dfbe03dc34c3003f8a3e1_0_32Comment #25
borisson_I don't understand having to bypass NotNullConstraint? I think we have to reimplement it in here?
We should probably start with a reroll here.
Comment #27
bbralaRebased thsi issue.
@alexpott
> I think this issue points to us needing a new constraint which is something like NotBlankAfterInstall - so a very can be blank during site install but once the site is up and running it can not be. This is true for uuid, name and email.
You say this, which is kinda the same problem as #3445976: Add validation constraints to contact.form.* comment #32. There it is a recipe which wants to use mail to replace a value, which is doesn't get since mail is not yet in config when installing from a recipe with the drupal command.
So there is also some sort of 'during install' issue, where during an install is appearantly possible to have invalid config.
Perhaps we even need to fix it ealier? Although i don't really feal like ignoring validation for config while installing is a good idea. Since it is pretty valuable, what if you are to install some sort of Drupal implementation (like a decoupled Drupal) and it comes with invalid config, would we want to ignore that?
Comment #28
wim leersThis will allow Experience Builder to remove a hack that #3503412: Allow Content Author to set site's homepage from navigator is adding.
Comment #30
dries commented