Problem/Motivation
Form modes have 3 property paths that are not yet validatable:
vendor/bin/drush config:inspect --filter-keys=core.entity_form_mode.user.register --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
---------------------------------------------------------------- --------- ------------- ------ ---------------------------------------------------------------------------------------------
core.entity_form_mode.user.register Correct 79% ✅❓ ValidKeys: '<infer>'
core.entity_form_mode.user.register: Correct Validatable ✅✅ ValidKeys: '<infer>'
core.entity_form_mode.user.register:_core Correct Validatable ✅✅ ValidKeys:
- default_config_hash
core.entity_form_mode.user.register:_core.default_config_hash Correct Validatable ✅✅ NotNull: { }
Regex: '/^[a-zA-Z0-9\-_]+$/'
Length: 43
↣ PrimitiveType: { }
core.entity_form_mode.user.register:cache Correct Validatable ✅✅ ↣ PrimitiveType: { }
core.entity_form_mode.user.register:dependencies Correct Validatable ✅✅ ValidKeys: '<infer>'
core.entity_form_mode.user.register:dependencies.module Correct NOT ✅❓ ❌ @todo Add validation constraints to ancestor type: config_dependencies
core.entity_form_mode.user.register:dependencies.module.0 Correct Validatable ✅✅ NotBlank: { }
ExtensionName: { }
ExtensionExists: module
↣ PrimitiveType: { }
core.entity_form_mode.user.register:description Correct Validatable ✅✅ Regex:
pattern: '/([^\PC\x09\x0a\x0d])/u'
match: false
message: 'Text is not allowed to contain control characters, only visible characters.'
↣ PrimitiveType: { }
core.entity_form_mode.user.register:id Correct NOT ✅❓ ⚠️ @todo Add validation constraints to config entity type: core.entity_form_mode.*.*
core.entity_form_mode.user.register:label Correct Validatable ✅✅ Regex:
pattern: '/([^\PC])/u'
match: false
message: 'Labels are not allowed to span multiple lines or contain control characters.'
NotBlank: { }
↣ PrimitiveType: { }
core.entity_form_mode.user.register:langcode Correct Validatable ✅✅ NotNull: { }
Choice:
callback: 'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllValidLangcodes'
↣ PrimitiveType: { }
core.entity_form_mode.user.register:status Correct Validatable ✅✅ ↣ PrimitiveType: { }
core.entity_form_mode.user.register:targetEntityType Correct NOT ✅❓ ⚠️ @todo Add validation constraints to config entity type: core.entity_form_mode.*.*
core.entity_form_mode.user.register:uuid Correct Validatable ✅✅ Uuid: { }
↣ 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=core.entity_form_mode.user.register --detail --list-constraints
Proposed resolution
Add validation constraints to:
core.entity_form_mode.*.*:dependencies.modulecore.entity_form_mode.*.*:idcore.entity_form_mode.*.*:targetEntityType
This requires looking at the existing code and admin UI (if any) to understand which values could be considered valid. Eventually this needs to be reviewed by the relevant subsystem maintainer.
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
None.
Data model changes
More validation 🚀
Release notes snippet
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #32 | 3448457-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #25 | 3448457-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #20 | Screenshot 2024-06-10 at 9.40.16 AM.png | 100.01 KB | smustgrave |
| #5 | Screenshot 2024-05-22 at 10.23.13 AM.png | 76.83 KB | smustgrave |
Issue fork drupal-3448457
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:
- 3448457-add-validation-constraints
changes, plain diff MR !8135
Comments
Comment #3
narendrarComment #5
smustgrave commentedApplied some nitpicky typehint return voids
Applied the MR to a 11.x install using standard profile
Believe the only entity_form_mode is for core.entity_form_mode.user.register
Ran updb and hook ran without issue
Manually downloaded config_inspector to 11.x setup and confirmed seeing fully validatable now
Comment #6
catchOne issue with the presave hook on the MR - missing a deprecation notice. #3443942: Add proper deprecation notices in config entity presave bc layers has more details.
Comment #7
narendrarComment #8
smustgrave commentedLeft some comments on MR.
Comment #9
narendrarComment #10
smustgrave commentedDeprecation appears to be correct to me.
Comment #11
catchOne small comment on the deprecation message. Hard to fit this sort of thing into the standard format.
Comment #12
narendrarComment #13
smustgrave commentedMessage seems good
Comment #14
larowlanLeft one question on the MR, great work here - bit more involved with the update hook!
Comment #15
narendrarThis issue is ready for re-review. I have added the return FALSE. Thanks
Comment #16
phenaproximaLooks okay but I have a couple of questions here.
Comment #17
narendrarComment #18
phenaproximaOnly one tiny thing and then I think this looks good.
Comment #19
narendrarComment #20
smustgrave commentedApplied MR on a standard install for 11.x with config inspector and getting all green checks.
Looking at MR 8135 and from what I can tell all feedback has been addressed.
Comment #21
alexpottIf I go to admin/structure/display-modes/form/ and edit the user registration form and save the form mode with no description then the system_entity_form_mode_presave() will trigger a deprecation notice.
We need to add something to the form to convert empty strings to NULLs... normally we'd say use #config_target but we've not worked out how to using #config_target like stuff with config entities yet.
Comment #22
narendrarComment #23
smustgrave commentedTested the scenario in #21 has been addressed with https://git.drupalcode.org/project/drupal/-/merge_requests/8135/diffs?co...
Comment #24
alexpottI'm not sure that a NULL description is any better then an empty string. Do we think that this part of the change is actually worth it?
Comment #25
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 #26
phenaproximaI don't feel strongly one way or the other, so I'm assigning this to Wim for input. I suspect he has a clearer reason why NULL would be useful here.
Comment #27
bbralaGoing through the config issues a bit to see where things are hanging.
I read through all this, and have not seen any discussion around the NULL change. I understand the text in the CR ("The reason is because an empty string makes no sense for this field. "" is never a useful description of a form mode."). But I dont really see why this is as much better as it is a change.
If i try to argue why it should be null: Now i want no desription is equal to en empty description. This unfortunately meant he field is not really optional, when you wouldn't post an description you technically tend a NULL, which cannot be stored right now. But if we allow null, we would be able to save without the field, since null is fine.
This could have reasons in jsonapi, maybe we can then post without the field and make it null, instead of requiring an empty string. This is as far as i get. But does make me beleive null is a good idea.
Comment #28
bbralaRemoving wim from assignment, that will probably not make this go faster.
My conclusion in #27 is that null is a good idea. I rebased and if tests don't fail I think this can move forward as is. Since my additions were minimal, setting RTBC.
Comment #29
bbralaSeems fixtures might need work. Not sure how those work.
Comment #30
bbralaComment #31
borisson_Found the tiniest of nitpicks, this is just a version number change in the deprecation, so leaving at rtbc.
Comment #32
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 #33
bbralaRebaed.
Comment #34
alexpottWe need to move the hook implementation to the correct new OOP place... \Drupal\system\Hook\SystemHooks
Comment #35
bbralaThanks, you are right. By now we have the OOP hooks. I think this issue is present in the other validation issues also. I'll do a round.
Comment #36
bbralaI think this change is so minimal i can RTBC again. I ran the upgrade test after moving the hook code to SystemHooks.php and it upgraded fine.
Comment #37
alexpottCommitted 4f3432b and pushed to 11.x. Thanks!
Comment #39
bbralaCR was out of date regarding versions, does it need publishing also? Probably?
Comment #40
quietone commentedI'll take the 'probably' as a 'yes'. I have updated and published the CR.
Comment #41
bbralaHehe yeah, it would need publishing, ty. Didn't want to do it myself for possible text issues.