Problem/Motivation

Date formats have 1 property paths that are not yet validatable:

./vendor/bin/drush config:inspect --filter-keys=block_content.type.banner_block --detail --list-constraints --fields=key,validatability,constraints
➜  🤖 Analyzing…

 ------------------------------------------------------------ ------------- --------------------------------------------------------------------------------------------- 
  Key                                                          Validatable   Validation constraints                                                                       
 ------------------------------------------------------------ ------------- --------------------------------------------------------------------------------------------- 
  block_content.type.banner_block                              91%           ValidKeys: '<infer>'                                                                         
   block_content.type.banner_block:                            Validatable   ValidKeys: '<infer>'                                                                         
   block_content.type.banner_block:_core                       Validatable   ValidKeys:                                                                                   
                                                                               - default_config_hash                                                                      
   block_content.type.banner_block:_core.default_config_hash   Validatable   NotNull: {  }                                                                                
                                                                             Regex: '/^[a-zA-Z0-9\-_]+$/'                                                                 
                                                                             Length: 43                                                                                   
                                                                             ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:dependencies                Validatable   ValidKeys: '<infer>'                                                                         
   block_content.type.banner_block:description                 Validatable   Regex:                                                                                       
                                                                               pattern: '/([^\PC\x09\x0a\x0d])/u'                                                         
                                                                               match: false                                                                               
                                                                               message: 'Text is not allowed to contain control characters, only visible characters.'     
                                                                             ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:id                          Validatable   Regex:                                                                                       
                                                                               pattern: '/^[a-z0-9_]+$/'                                                                  
                                                                               message: 'The %value machine name is not valid.'                                           
                                                                             Length:                                                                                      
                                                                               max: 166                                                                                   
                                                                             ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:label                       Validatable   Regex:                                                                                       
                                                                               pattern: '/([^\PC])/u'                                                                     
                                                                               match: false                                                                               
                                                                               message: 'Labels are not allowed to span multiple lines or contain control characters.'    
                                                                             NotBlank: {  }                                                                               
                                                                             ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:langcode                    Validatable   NotNull: {  }                                                                                
                                                                             Choice:                                                                                      
                                                                               callback: 'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllValidLangcodes'  
                                                                             ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:revision                    NOT           ⚠️  @todo Add validation constraints to config entity type: block_content.type.*             
   block_content.type.banner_block:status                      Validatable   ↣ PrimitiveType: {  }                                                                        
   block_content.type.banner_block:uuid                        Validatable   Uuid: {  }                                                                                   
                                                                             ↣ PrimitiveType: {  }                     

On my local umami install, they are also with 4 in the top 10 of config objects the closest to 100% validatability. (See ./vendor/bin/drush config:inspect --todo=50)

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=block_content.type.banner_block --detail --list-constraints

Proposed resolution

Add validation constraints to:

  1. block_content.type.*:revision

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

  1. block_content.type.*:revision

User interface changes

None.

API changes

None.

Data model changes

More validation 🚀

Release notes snippet

None.

Issue fork drupal-3397493

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:

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

This is a bit tricky, because it is actually currently not a correct date type.

This is the current schema:

    revision:
      type: integer
      label: 'Create new revision'

The same thing happens on node types, and that looks a bit different:

    new_revision:
      type: boolean
      label: 'Whether a new revision should be created by default

As far as I understand it, these actually do the same thing, and they are validatable, because they are booleans.

We can't just make it the boolean type, because it might be the same in the database (0/1), but in the schema it is written as true/false instead of the integer, so I think this one needs an upgrade path as well?

I don't think we should update the key to new_revision, because while that would be more consistent; and easier to understand, that will probably be more difficult to land? I'm curious to see what the block content maintainers think.

I'll add a Merge request that changes the data type and let's see what breaks.

borisson_’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Seems to have some test failures in the pipeline

borisson_’s picture

Status: Needs work » Needs review

I think I fixed all the broken tests, also merged 11.x back into this.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Ran locally and update hook ran fine. And change to boolean makes sense. LGTM.

wim leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs update path tests

sourabhjain made their first commit to this issue’s fork.

wim leers’s picture

@sourabhjain You applied my suggestion locally and then pushed it. But you added a trailing blank line, which is now causing a phpcs failure. More tests will fail too. Can you address those things too? 🙏😊

sourabhjain’s picture

Yes @wim-leers. I saw your suggestion on PR and after that applied it manually on local and pushed it.
And sorry for trailing blank line issue. I have pushed it again. Thanks

wim leers’s picture

Good! 👍

Now there are 114 kernel test failures, 77 functional test failures and 17 functional JS test failures. Can you push those forward too? 🙏

sourabhjain’s picture

I apologize for any inconvenience, @wim-leers. I misunderstood the suggestion, thinking it was a provided task that anyone could work on. I will be more careful in the future and ensure that such misunderstandings don't happen again. Rest assured, everyone at Valuebound will be mindful of this.

wim leers’s picture

Causing test failures is fine of course! 😄 I make a hundred mistakes every day. You're right that anybody can work on this. But … only applying a suggestion that takes a single click to apply is not quite helpful. A suggestion is meant to make it easier for whichever person is working on the merge request, to speed up addressing the feedback in a review. IOW: blindly applying suggestions is not helpful; it causes confusion about the state of a merge request.

Looking forward to our next interaction! 😊

borisson_’s picture

Addressed most of Wim's suggestions. We will need to:
1. Fix all the tests, the test failures are caused by the configuration not matching the schema, so we will need to add `'revision' => FALSE,` in a whole bunch of places.
2. Add an upgrade path test, I haven't done that before and I'm not sure where I'll find the time to learn how to write one.

Both of these can definitely be picked up by someone else.

borisson_’s picture

I started changed some of the tests to improve the state of the pr, but I think I need to make a lot of changes like this in tests:

@@ -37,6 +37,7 @@ public function testReusableBlocksOnlyAreDerived() {
       'id' => 'spiffy',
       'label' => 'Mucho spiffy',
       'description' => "Provides a block type that increases your site's spiffiness by up to 11%",
+      'revision' => FALSE,
     ]);

This doesn't make sense to me, is there a better way to do this?

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for update path tests.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs update path tests

Back to you!

smustgrave’s picture

Status: Needs review » Needs work

Seems to have test failures.

phenaproxima’s picture

Status: Needs work » Needs review

Failures a-fixed!

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Sweet! Seems validation for revision is good and didn’t break anything. Excited to see these improvements

wim leers’s picture

Status: Reviewed & tested by the community » Needs work

Found 2 actual bugs, 1 question.

phenaproxima’s picture

Assigned: Unassigned » wim leers
Status: Needs work » Needs review
borisson_’s picture

I'm not sure if i can rtbc the latest changes since I've worked on this issue a lot as well, but I agree with @phenaproxima that this is committable as is.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community
Related issues: +#3413144: Improve NotBlank DX in config (schema) validation

The last commit (aka my suggestion) introduced a small regression, sorry:

Exception: Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated

Easy fix though: add ?? '' ← applied this trivial change 👍

P.S.: one of my remarks on the MR was indeed wrong, as @phenaproxima pointed out. Opened an issue for that (which does not block this one): #3413144: Improve NotBlank DX in config (schema) validation 👍

  • larowlan committed 4b5138e6 on 11.x
    Issue #3397493 by borisson_, phenaproxima, Wim Leers: Add validation...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 11.x

Thanks

wim leers’s picture

Yay, this brought us to 0.9 → 1.1% of all types being fully validatable, and bumped the Standard install profile from 4.4 to 4.9%! https://project.pages.drupalcode.org/config_inspector/ 🚀

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.