Add validation constraints to system.date

Created on 1 April 2024, 12 days ago
Updated 12 April 2024, about 13 hours ago

Problem/Motivation

system.date has 4 property path that are not yet validatable:

./vendor/bin/drush config:inspect --filter-keys=system.date --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.date                               Correct   67%           โœ…โ“   ValidKeys: '<infer>'                      
   system.date:                             Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'                      
   system.date:_core                        Correct   Validatable   โœ…โœ…   ValidKeys:                                
                                                                             - default_config_hash                   
   system.date:_core.default_config_hash    Correct   Validatable   โœ…โœ…   NotNull: {  }                             
                                                                           Regex: '/^[a-zA-Z0-9\-_]+$/'              
                                                                           Length: 43                                
                                                                           โ†ฃ PrimitiveType: {  }                     
   system.date:country                      Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'                      
   system.date:country.default              Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  
   system.date:first_day                    Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  
   system.date:timezone                     Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'                      
   system.date:timezone.default             Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  
   system.date:timezone.user                Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'                      
   system.date:timezone.user.configurable   Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }                     
   system.date:timezone.user.default        Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here  
   system.date:timezone.user.warn           Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }                     
 ----------------------------------------- --------- ------------- ------ ------------------------------------------ 

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=system.date --detail --list-constraints

Proposed resolution

Add validation constraints to:

  1. system.date:country.default
  2. system.date:first_day
  3. system.date:timezone.default
  4. system.date:timezone.user.default

For examples, search *.schema.yml files for the string constraints: ๐Ÿ˜Š

Reach out to @borisson_ or @wimleers in the #distributions-and-recipes.

Remaining tasks

User interface changes

None.

API changes

Data model changes

More validation ๐Ÿš€

Release notes snippet

None.

๐Ÿ“Œ Task
Status

RTBC

Version

10.3 โœจ

Component
Systemย  โ†’

Last updated about 12 hours ago

No maintainer
Created by

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

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

Merge Requests

Comments & Activities

  • Issue created by @narendraR
  • Status changed to Needs work 12 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    narendraR โ†’ changed the visibility of the branch 3437325-add-validation-constraints to hidden.

  • Merge request !7271Initial code โ†’ (Open) created by narendraR
  • Pipeline finished with Failed
    12 days ago
    Total: 478s
    #134743
  • Pipeline finished with Failed
    11 days ago
    Total: 512s
    #135165
  • Pipeline finished with Failed
    11 days ago
    Total: 630s
    #135270
  • Pipeline finished with Failed
    11 days ago
    Total: 1315s
    #135601
  • Pipeline finished with Success
    11 days ago
    Total: 767s
    #135624
  • Status changed to Needs review 11 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 10 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    10 days ago
    Total: 624s
    #136055
  • Pipeline finished with Failed
    10 days ago
    Total: 576s
    #136080
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Interesting failures:

    Drupal\FunctionalTests\Installer\InstallerSiteConfigProfileTest::testInstaller
    Behat\Mink\Exception\ExpectationException: The field "date_default_timezone" value is "Africa/Abidjan", but "America/Los_Angeles" expected.

    and

    Drupal\FunctionalTests\Installer\InstallerTest::testInstaller
    Failed asserting that null matches expected 'Australia/Sydney'.
    

    This is looking very close though! :)

  • Pipeline finished with Failed
    10 days ago
    Total: 636s
    #136253
  • Pipeline finished with Failed
    10 days ago
    Total: 659s
    #136315
  • Pipeline finished with Failed
    10 days ago
    Total: 542s
    #136511
  • Pipeline finished with Failed
    10 days ago
    Total: 692s
    #136566
  • First commit to issue fork.
  • Pipeline finished with Failed
    8 days ago
    Total: 601s
    #137880
  • Pipeline finished with Canceled
    8 days ago
    Total: 271s
    #137922
  • Pipeline finished with Success
    8 days ago
    #137936
  • Status changed to Needs review 8 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 8 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Success
    8 days ago
    Total: 689s
    #138385
  • Status changed to Needs review 8 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 8 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review 8 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    No, bot, it does not need work.

  • Status changed to Needs work 8 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Only one tiny comment, then I don't really see a problem here.

  • Pipeline finished with Failed
    8 days ago
    Total: 612s
    #138890
  • Status changed to Needs review 5 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 5 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“Œ Remove country setting from the installer Active landed today! It conflicts with this MR.

    I don't see how this could land without at least an update path โ€” fortunately it's a trivial one! I suspect we'll also need explicit tests for \Drupal\system\Form\RegionalForm.

  • Pipeline finished with Canceled
    4 days ago
    Total: 143s
    #141466
  • Pipeline finished with Failed
    4 days ago
    Total: 1632s
    #141468
  • Pipeline finished with Failed
    4 days ago
    Total: 190s
    #141543
  • Pipeline finished with Failed
    4 days ago
    Total: 1142s
    #141873
  • Pipeline finished with Success
    4 days ago
    Total: 1016s
    #141900
  • Pipeline finished with Success
    4 days ago
    Total: 964s
    #141963
  • Status changed to Needs review 4 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 4 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review 3 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • Status changed to Needs work 2 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Good progress!

    In the final stretch to the finish line now :)

    Problems I spotted in review:

    1. validation of timezone.user.default is misleading โ€” easily fixed ๐Ÿ‘
    2. form test coverage is giving a false sense of security โ€” it should be a functional test, not a kernel test

    P.S.: please push #2951046: Allow parsing and writing PHP constants and enums in YAML files โ†’ forward so that we can land โœจ [PP-1] Enum cases stored in config Postponed โ€” that'd make validation of timezone.user.default in this issue simpler/more maintainable/clearer. It'd also help many other (config) validation issues!

  • Pipeline finished with Success
    about 19 hours ago
    Total: 1138s
    #144836
  • Status changed to Needs review about 19 hours ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work about 15 hours ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Some minor points, but this generally looks pretty good to me.

  • Pipeline finished with Failed
    about 15 hours ago
    Total: 195s
    #145033
  • Pipeline finished with Failed
    about 14 hours ago
    Total: 1168s
    #145038
  • Pipeline finished with Success
    about 14 hours ago
    Total: 1017s
    #145067
  • Status changed to Needs review about 14 hours ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Pipeline finished with Running
    about 14 hours ago
    #145082
  • Status changed to RTBC about 13 hours ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Ship it!

Production build https://api.contrib.social 0.62.1