Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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/drush
vendor/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 |
---|---|---|---|
#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:
Comments
Comment #3
Wim LeersJust like
filter.settings
before 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
ExistsIn
constraint 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 CreditAttribution: 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
ExistsIn
anyway 👍Comment #7
smustgrave CreditAttribution: 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 CreditAttribution: needs-review-queue-bot as a volunteer 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 CreditAttribution: 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 CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: smustgrave at Mobomo 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.