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

  1. Get a local git clone of Drupal core 11.x.
  2. 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!)
  3. composer require drush/drush
  4. vendor/bin/drush config:inspect --filter-keys=book.settings --detail --list-constraints

Proposed resolution

  1. Add validation constraints.
  2. Mark FullyValidatable.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#10 3422862-nr-bot.txt13.79 KBneeds-review-queue-bot

Issue fork drupal-3422862

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Just 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....

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
Related issues: +#2920441: Add config validation for workflow entities

This 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.

smustgrave’s picture

Asked 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

Wim Leers’s picture

Don't feel strongly about it either way. But was definitely going to create a new issue for ExistsIn anyway 👍

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

So I asked @catch and doesn't see this as an issue even though book is deprecated.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Left some comments on the MR>

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#3402178: [PP-1] Enum cases stored in config

Rerolled.

We may want to block this on #3402178: [PP-1] Enum cases stored in config.

Needs feedback on how to proceed.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
13.79 KB

The 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.

Wim Leers’s picture

Status: Needs work » Needs review
smustgrave’s picture

So 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?

smustgrave’s picture

So 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Actually since this is a blocker for book subtree split going to mark now, hope that's okay!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

We've not really resolved the threads on the MR yet. Why is this a blocker for the sub tree split?

Wim Leers’s picture

Why is this a blocker for the sub tree split?

I'd love to understand this too.

smustgrave’s picture

Tagged ya both in the thread in #d11readiness. Guess I shouldn't say blocker but more advised to wait to avoid doing multiple subtree splits

smustgrave’s picture

So is the open question if this should be postponed on #3402178: [PP-1] Enum cases stored in config

Wim Leers’s picture

Title: Add validation constraints to book.settings » [PP-2] Add validation constraints to book.settings
Status: Needs review » Postponed

+1

Wim Leers’s picture

Project: Drupal core » Book
Version: 11.x-dev » 1.0.x-dev
Component: book.module » Code
Priority: Normal » Minor
Related issues: +#3413932: Deprecate Book module

#3413932: Deprecate Book module happened, so these settings will be gone in Drupal 11.

Moving to https://www.drupal.org/project/book.