Problem/Motivation
Field settings have 1 property paths that are not yet validatable:
$ ./vendor/bin/drush config:inspect --filter-keys=field.settings --detail --list-constraints --fields=key,validatability,constraints
➜ 🤖 Analyzing…
------------------------------------------- ------------- ------------------------------------------
Key Validatable Validation constraints
------------------------------------------- ------------- ------------------------------------------
field.settings 75% ValidKeys: '<infer>'
RequiredKeys: '<infer>'
field.settings: Validatable ValidKeys: '<infer>'
RequiredKeys: '<infer>'
field.settings:_core Validatable ValidKeys:
- default_config_hash
RequiredKeys: '<infer>'
field.settings:_core.default_config_hash Validatable NotNull: { }
Regex: '/^[a-zA-Z0-9\-_]+$/'
Length: 43
↣ PrimitiveType: { }
field.settings:purge_batch_size 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=olivero.settings --detail --list-constraints
Proposed resolution
Add validation constraints to:
purge_batch_size
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
purge_batch_size
User interface changes
None.
API changes
None.
Data model changes
More validation 🚀
Release notes snippet
None.
Issue fork drupal-3395627
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:
- 3395627-add-validation-constraints
changes, plain diff MR !6338
- 11.x
changes, plain diff MR !5077
Comments
Comment #3
hdnag commentedI took this one from DrupalCon Lille Contribution day
Comment #5
borisson_This is a good improvement over the current state, and it being not null is correct, but I think we should add a minimum value as well?
Comment #6
smustgrave commentedDo think a min makes sense. Can't purge -10 files right?
Comment #7
borisson_Let's postpone this on #3365498: Add Missing Symfony Validators to Drupal Constraint Validator, that introduces a GreaterThan
Comment #8
borisson_Actually, it looks like the range constraint can also do this, unpostponing this issue.
Comment #12
svendecabooterThere were some problems with the 11.x branch on this issue's repo.
After discussion with borisson_ I added a new merge request to the other branch that existed on this issue.
The change by hdnag is also added to this branch now.
Comment #14
borisson_Adding tag.
Comment #15
borisson_Looks great
Comment #16
wim leersAgreed with RTBC 😊
Comment #17
catchComment #18
catchI don't think zero is really a valid size here, we would never purge anything and never warn anyone that nothing was purged. I realise that's the status quo that you can set it to zero, but feels like codifying the existing bug further.
If we think it's a feature to be able to set this to zero (to prevent purging in order to debug), then there should probably be a hook_requirements() warning when it's set to zero to remind people to enable it again. If that's the case then fine with opening a follow-up for it. Or making the minimum range 1.
Comment #19
wim leersD'oh, I should've spotted this 😬 Setting this to zero means nothing would ever happen 😅
Disallowing a previously allowed value in principle means we need to provide an update path, to reset it to the default. But in this case, that seems such an extremely unlikely thing … that I wonder if we can get away with just changing the minimum to 1? 🤔 (Because
0would not have worked anyway, so nobody would ever have had a need to set it to that?)Comment #20
catchI guess the only question is - does it need to be left open to prevent field purging? I could see someone maybe using that in a situation like #3409173: Enforced dependency bundles are deleted without validation when modules are uninstalled but also you would only find out about that issue either before or after that would ever be useful. So I don't really think it is a valid use case and we should set the minimum to 1.
Comment #21
wim leers👍 Done. Re-RTBC'ing since it's a trivial change.
Comment #23
catchCommitted 2256cd2 and pushed to 11.x. Thanks!