Add validation constraints to system.file

Created on 26 March 2024, about 1 year ago

Problem/Motivation

System module settings has 2 property paths that are not yet validatable:

vendor/bin/drush config:inspect --filter-keys=system.file --detail --list-constraints
โžœ  ๐Ÿค– Analyzingโ€ฆ

 Legend for Data: 
  โœ…โ“  โ†’ Correct primitive type, detailed validation impossible.
  โœ…โœ…  โ†’ Correct primitive type, passed all validation constraints.
 ---------------------------------------- --------- ------------- ------ ------------------------------------------ 
  Key                                      Status    Validatable   Data   Validation constraints                    
 ---------------------------------------- --------- ------------- ------ ------------------------------------------ 
  system.file                              Correct   67%           โœ…โ“   ValidKeys: '<infer>'                      
   system.file:                            Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'                      
   system.file:_core                       Correct   Validatable   โœ…โœ…   ValidKeys:                                
                                                                            - default_config_hash                   
   system.file:_core.default_config_hash   Correct   Validatable   โœ…โœ…   NotNull: {  }                             
                                                                          Regex: '/^[a-zA-Z0-9\-_]+$/'              
                                                                          Length: 43                                
                                                                          โ†ฃ PrimitiveType: {  }                     
   system.file:allow_insecure_uploads      Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }                     
   system.file:default_scheme              Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  
   system.file:temporary_maximum_age       Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  

Steps to reproduce

Proposed resolution

Add validation constraints to:

  1. system.file:default_scheme
  2. system.file:temporary_maximum_age

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
Systemย  โ†’

Last updated about 3 hours ago

No maintainer
Created by

๐Ÿ‡ฎ๐Ÿ‡ณIndia srishtiiee

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @srishtiiee
  • Merge request !7187add validation constraints โ†’ (Open) created by srishtiiee
  • Pipeline finished with Failed
    about 1 year ago
    Total: 182s
    #129396
  • First commit to issue fork.
  • Pipeline finished with Failed
    about 1 year ago
    Total: 477s
    #129456
  • Pipeline finished with Failed
    about 1 year ago
    Total: 508s
    #129506
  • Pipeline finished with Failed
    about 1 year ago
    Total: 180s
    #130305
  • Status changed to Closed: duplicate about 1 year ago
  • Status changed to Needs work about 1 year ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia srishtiiee

    srishtiiee โ†’ changed the visibility of the branch 3436096-add-validation-constraints to hidden.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 506s
    #148036
  • Pipeline finished with Failed
    about 1 year ago
    Total: 513s
    #148068
  • Pipeline finished with Failed
    12 months ago
    Total: 544s
    #150101
  • Pipeline finished with Failed
    12 months ago
    Total: 1081s
    #150231
  • Pipeline finished with Failed
    12 months ago
    Total: 195s
    #162152
  • Pipeline finished with Failed
    12 months ago
    Total: 1151s
    #162153
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Tests are failing and I think we can fix that by just pushing the latest origin/11.x to this issue fork ๐Ÿคž

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Review posted on MR โ€” if I'd found nothing in my review, then I'd have done #9 for you โ€” but since I can't RTBC this yet anyway, I'll let you handle #9 too ๐Ÿค“

  • Pipeline finished with Failed
    12 months ago
    Total: 521s
    #166069
  • Pipeline finished with Failed
    11 months ago
    Total: 1558s
    #168648
  • Pipeline finished with Failed
    11 months ago
    Total: 631s
    #168663
  • Pipeline finished with Failed
    11 months ago
    Total: 674s
    #169499
  • Pipeline finished with Failed
    11 months ago
    Total: 610s
    #171500
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

    yash.rode โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    10 months ago
    Total: 170s
    #210495
  • Pipeline finished with Failed
    10 months ago
    Total: 186s
    #210500
  • Pipeline finished with Failed
    10 months ago
    Total: 538s
    #210538
  • Pipeline finished with Canceled
    10 months ago
    Total: 320s
    #212794
  • Pipeline finished with Success
    10 months ago
    Total: 549s
    #212800
  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune
  • Status changed to RTBC 10 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    On a standard profile install on 11.x
    Installed configuration inspector and applied the MR

    Shows that the system.file is fully validated.

    Checking the MR everything appears to have a return type

    Believe this one is good to go.

  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    If path is deprecated we should deprecate it. See https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... โ†’

    Added some review comments.

  • Pipeline finished with Failed
    10 months ago
    Total: 517s
    #215530
  • Pipeline finished with Canceled
    10 months ago
    Total: 215s
    #215552
  • Pipeline finished with Success
    10 months ago
    Total: 506s
    #215557
  • Pipeline finished with Failed
    10 months ago
    Total: 552s
    #216492
  • Pipeline finished with Failed
    10 months ago
    Total: 581s
    #216574
  • Pipeline finished with Failed
    9 months ago
    Total: 570s
    #219494
  • Pipeline finished with Success
    9 months ago
    Total: 479s
    #219500
  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

    Addressed feedback from #14

  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    I think as per #14, a deprecation message needs to be added for path in system.schema.yml before removing it completely.

  • Pipeline finished with Failed
    9 months ago
    Total: 695s
    #219930
  • Pipeline finished with Failed
    9 months ago
    Total: 508s
    #220074
  • Pipeline finished with Success
    9 months ago
    Total: 583s
    #220365
  • Pipeline finished with Failed
    9 months ago
    Total: 532s
    #220427
  • Pipeline finished with Failed
    9 months ago
    Total: 490s
    #220444
  • Pipeline finished with Canceled
    9 months ago
    Total: 414s
    #220463
  • Pipeline finished with Success
    9 months ago
    Total: 449s
    #220466
  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune
  • Status changed to RTBC 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    Changes looks good to me.

  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    One question on the MR.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    I agree with catch in #19, moving this to Drupal\Core\Config seems like a better solution, let's do that.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    17 days ago
    Total: 54s
    #461879
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Updated the class namespace as suggested, did do a rebase to clean up.

  • Pipeline finished with Success
    17 days ago
    Total: 590s
    #461881
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    Back to rtbc now that it is moved.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    If we changing the 10.3 database dumps because of invalid schema this points to us needing an update function. I think we should not be changing the dumps.

    Also I think we should be adding a new constraint that works like callback but uses the class resolver service to my more flexible.

  • Pipeline finished with Failed
    16 days ago
    Total: 178s
    #462592
  • Pipeline finished with Failed
    16 days ago
    Total: 155s
    #462593
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    So, the path config key was deprecated in Drupal 8.8 it seems ( https://www.drupal.org/node/3039255 โ†’ ). That kinda feels out of scope of this change, the fact is that it will need to go through deprecation? I don't even really see any usage in core, but that could be my search skills failing me.

    But i don't feel we should do that here. Hopefully you agree.

  • Pipeline finished with Failed
    16 days ago
    Total: 161s
    #462599
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Waiting for build, since i had some cs issues ;x

  • Pipeline finished with Failed
    16 days ago
    Total: 156s
    #462607
  • Pipeline finished with Failed
    16 days ago
    Total: 129s
    #462609
  • Pipeline finished with Failed
    16 days ago
    Total: 109s
    #462779
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Adjusted it to actually do what was asked for.

    Did revert the deprecation message for the path property. Would like to know if we need to make the change here, or if we should defer that. Seems like that has been here for quite a while. I do think we might be able to ignore it here, since it doesnt seem to be used.

  • Pipeline finished with Failed
    16 days ago
    Total: 2605s
    #462792
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands
  • Pipeline finished with Success
    15 days ago
    Total: 672s
    #463106
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Ok, lets try again. Seems good now.

  • Pipeline finished with Failed
    15 days ago
    Total: 642s
    #463116
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    ๐Ÿ“Œ Remove system.file.path config from system.schema.yml Active created to do the deprecation and upgrade path

  • Pipeline finished with Running
    14 days ago
    #464409
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Postpone on child issue.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Child is committed

  • Pipeline finished with Failed
    13 days ago
    Total: 170s
    #465614
  • Pipeline finished with Failed
    13 days ago
    Total: 117s
    #465615
  • Pipeline finished with Failed
    13 days ago
    Total: 449s
    #465622
  • Pipeline finished with Success
    13 days ago
    Total: 417s
    #465641
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    Pipe is all green after the upgrade stuff in the child. Think we are there.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    ๐Ÿ“Œ Remove system.file.path config from system.schema.yml Active was merged, rebased this one.

  • Pipeline finished with Failed
    6 days ago
    Total: 455s
    #471110
  • Pipeline finished with Success
    6 days ago
    Total: 696s
    #471123
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Using configuration inspector

    Does the new constraint need a CR? I vote yes but don't want to hold it up on that.

    Rest appears to be addressed

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    Committed 63187d5 and pushed to 11.x. Thanks!

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands
Production build 0.71.5 2024