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

  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=olivero.settings --detail --list-constraints

Proposed resolution

Add validation constraints to:

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

  1. purge_batch_size

User interface changes

None.

API changes

None.

Data model changes

More validation 🚀

Release notes snippet

None.

Issue fork drupal-3395627

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.

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

hdnag’s picture

I took this one from DrupalCon Lille Contribution day

borisson_’s picture

Issue summary: View changes
Status: Active » Needs review

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?

smustgrave’s picture

Status: Needs review » Needs work

Do think a min makes sense. Can't purge -10 files right?

borisson_’s picture

Status: Needs work » Postponed
Related issues: +#3365498: Add Missing Symfony Validators to Drupal Constraint Validator

Let's postpone this on #3365498: Add Missing Symfony Validators to Drupal Constraint Validator, that introduces a GreaterThan

borisson_’s picture

Status: Postponed » Needs work

Actually, it looks like the range constraint can also do this, unpostponing this issue.

svendecabooter changed the visibility of the branch 11.x to hidden.

svendecabooter changed the visibility of the branch 11.x to active.

svendecabooter’s picture

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

svendecabooter changed the visibility of the branch 11.x to hidden.

borisson_’s picture

Adding tag.

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

Looks great

wim leers’s picture

Agreed with RTBC 😊

catch’s picture

Component: file.module » field system
catch’s picture

Status: Reviewed & tested by the community » Needs review

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

wim leers’s picture

I realise that's the status quo that you can set it to zero, but feels like codifying the existing bug further.

D'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 0 would not have worked anyway, so nobody would ever have had a need to set it to that?)

catch’s picture

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

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

👍 Done. Re-RTBC'ing since it's a trivial change.

  • catch committed 2256cd28 on 11.x
    Issue #3395627 by svendecabooter, hdnag, Wim Leers, borisson_, catch,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2256cd2 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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