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
- 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=block_content.type.banner_block --detail --list-constraints
Proposed resolution
Add validation constraints to:
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
block_content.type.*:revision
User interface changes
None.
API changes
None.
Data model changes
More validation 🚀
Release notes snippet
None.
Issue fork drupal-3397493
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:
- 3397493-add-validation-constraints
changes, plain diff MR !5170
Comments
Comment #2
borisson_This is a bit tricky, because it is actually currently not a correct date type.
This is the current schema:
The same thing happens on node types, and that looks a bit different:
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.
Comment #4
borisson_Comment #5
smustgrave commentedSeems to have some test failures in the pipeline
Comment #6
borisson_I think I fixed all the broken tests, also merged 11.x back into this.
Comment #7
smustgrave commentedRan locally and update hook ran fine. And change to boolean makes sense. LGTM.
Comment #8
wim leersComment #10
wim leers@sourabhjain You applied my suggestion locally and then pushed it. But you added a trailing blank line, which is now causing a
phpcsfailure. More tests will fail too. Can you address those things too? 🙏😊Comment #11
sourabhjainYes @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
Comment #12
wim leersGood! 👍
Now there are 114 kernel test failures, 77 functional test failures and 17 functional JS test failures. Can you push those forward too? 🙏
Comment #13
sourabhjainI 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.
Comment #14
wim leersCausing 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! 😊
Comment #15
borisson_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.
Comment #16
borisson_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:
This doesn't make sense to me, is there a better way to do this?
Comment #17
phenaproximaSelf-assigning for update path tests.
Comment #18
phenaproximaBack to you!
Comment #19
smustgrave commentedSeems to have test failures.
Comment #20
phenaproximaFailures a-fixed!
Comment #21
smustgrave commentedSweet! Seems validation for revision is good and didn’t break anything. Excited to see these improvements
Comment #22
wim leersFound 2 actual bugs, 1 question.
Comment #23
phenaproximaComment #24
borisson_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.
Comment #25
wim leersThe last commit (aka my suggestion) introduced a small regression, sorry:
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 👍
Comment #27
larowlanCommitted to 11.x
Thanks
Comment #29
wim leersYay, 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/ 🚀
Comment #31
wim leers