Problem/Motivation
The Book module's settings have 4 property paths that are not yet validatable:
$ vendor/bin/drush pm:install book
[success] Successfully enabled: book
$ ./vendor/bin/drush config:inspect --filter-keys=book.settings --detail --list-constraints --fields=key,validatability,constraints
➜ 🤖 Analyzing…
------------------------------------------ ------------- ------------------------------------------
Key Validatable Validation constraints
------------------------------------------ ------------- ------------------------------------------
book.settings 56% ValidKeys: '<infer>'
book.settings: Validatable ValidKeys: '<infer>'
book.settings:_core Validatable ValidKeys:
- default_config_hash
book.settings:_core.default_config_hash Validatable NotNull: { }
Regex: '/^[a-zA-Z0-9\-_]+$/'
Length: 43
↣ PrimitiveType: { }
book.settings:allowed_types NOT ⚠️ @todo Add validation constraints here
book.settings:allowed_types.0 NOT ⚠️ @todo Add validation constraints here
book.settings:block Validatable ValidKeys: '<infer>'
book.settings:block.navigation Validatable ValidKeys: '<infer>'
book.settings:block.navigation.mode NOT ⚠️ @todo Add validation constraints here
book.settings:child_type 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/drushvendor/bin/drush config:inspect --filter-keys=book.settings --detail --list-constraints
Proposed resolution
- Add validation constraints.
- Mark
FullyValidatable.
Remaining tasks
Review.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | book_settings.png | 27.56 KB | joseph.olstad |
| #10 | 3422862-nr-bot.txt | 13.79 KB | needs-review-queue-bot |
Issue fork drupal-3422862
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:
Issue fork book-3422862
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:
Comments
Comment #3
wim leersJust like
filter.settingsbefore it (see #3395562: Add validation constraints to filter.settings and #1932544: Remove all traces of fallback format concept from the (admin) UI), this simple config violates the basic rule of simple config: that it cannot depend on other config entities. 😬 Fixing that is out of scope here.But we can proceed here, thanks to
\Drupal\Core\Config\Schema\SchemaCheckTrait::$ignoredPropertyPaths👍Did that in https://git.drupalcode.org/project/drupal/-/merge_requests/6712/diffs?co....
Comment #4
wim leersThis MR copied the
ExistsInconstraint that I wrote yesterday for #2920441: Add config validation for workflow entities. That should have its own issue, but that doesn't mean this cannot already be reviewed.Comment #5
smustgrave commentedAsked in #needs-review-queue-initative but since book is deprecated we have postponed all issues for it. What do you think about breaking this apart? Open a new ticket for adding the new constraint and postpone this to have book update in contrib
Comment #6
wim leersDon't feel strongly about it either way. But was definitely going to create a new issue for
ExistsInanyway 👍Comment #7
smustgrave commentedSo I asked @catch and doesn't see this as an issue even though book is deprecated.
Comment #8
alexpottLeft some comments on the MR>
Comment #9
wim leersRerolled.
We may want to block this on #3402178: [PP-1] Enum cases stored in config.
Needs feedback on how to proceed.
Comment #10
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 #11
wim leersComment #12
smustgrave commentedSo do want to mention that book is deprecated and we are trying to do a sub tree split and kinda falls back to what the process is going to be. If we are going to continue with pushing fixes to deprecated modules are we going to have and sub tree split every time?
Comment #13
smustgrave commentedSo I see the concern around core/modules/book/config/schema/book.schema.yml and maintaining that list. For this particular instance I think it's a none problem as book is deprecated and being removed so I don't think it will be a problem for core. I'm going to be helping co-maintain book with @pwolanin and I don't mind maintaining this in contrib.
That may not help answer the question on what core should do for things that are remaining.
So in this instance change LGTM, will leave in review for a few more days for additional eyes.
Comment #14
smustgrave commentedActually since this is a blocker for book subtree split going to mark now, hope that's okay!
Comment #15
alexpottWe've not really resolved the threads on the MR yet. Why is this a blocker for the sub tree split?
Comment #16
wim leersI'd love to understand this too.
Comment #17
smustgrave commentedTagged ya both in the thread in #d11readiness. Guess I shouldn't say blocker but more advised to wait to avoid doing multiple subtree splits
Comment #18
smustgrave commentedSo is the open question if this should be postponed on #3402178: [PP-1] Enum cases stored in config
Comment #19
wim leers+1
Comment #20
wim leers#3413932: Deprecate Book module happened, so these settings will be gone in Drupal 11.
Moving to https://www.drupal.org/project/book.
Comment #21
smustgrave commentedComment #22
smustgrave commentedSo I think I'm going to remove the required part of the settings for book. Part of removing the default book content type and moving it to a recipe means those keys could be empty.
Comment #24
smustgrave commentedComment #25
joseph.olstadtried that validation constraint patch, doesn't help

Comment #26
joseph.olstadThe only way I can get through this form is patch #2 from
#3478378: "child_type_BUNDLE" is not a supported key
Comment #28
smustgrave commented