Add validation constraints to field_ui.settings

Created on 27 January 2024, 3 months ago
Updated 9 April 2024, 4 days ago

Problem/Motivation

field ui settings has 1 property path that is not yet validatable:

 ./vendor/bin/drush config:inspect --filter-keys=field_ui.settings --detail --list-constraints --fields=key,validatability,constraints 
โžœ  ๐Ÿค– Analyzingโ€ฆ

 ---------------------------------------------- ------------- ------------------------------------------ 
  Key                                            Validatable   Validation constraints                    
 ---------------------------------------------- ------------- ------------------------------------------ 
  field_ui.settings                              75%           ValidKeys: '<infer>'                      
   field_ui.settings:                            Validatable   ValidKeys: '<infer>'                      
   field_ui.settings:_core                       Validatable   ValidKeys:                                
                                                                 - default_config_hash                   
   field_ui.settings:_core.default_config_hash   Validatable   NotNull: {  }                             
                                                               Regex: '/^[a-zA-Z0-9\-_]+$/'              
                                                               Length: 43                                
                                                               โ†ฃ PrimitiveType: {  }                     
   field_ui.settings:field_prefix                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=field_ui.settings --detail --list-constraints

Proposed resolution

Add validation constraints to:

  1. field_ui.settings:field_prefix

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. field_ui.settings:field_prefix

User interface changes

None.

API changes

Data model changes

More validation ๐Ÿš€

Release notes snippet

None.

๐Ÿ“Œ Task
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Field UIย  โ†’

Last updated about 5 hours ago

Created by

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

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

Merge Requests

Comments & Activities

  • Issue created by @borisson_
  • First commit to issue fork.
  • Merge request !7221Initial commit โ†’ (Open) created by narendraR
  • Pipeline finished with Success
    16 days ago
    Total: 494s
    #131220
  • Status changed to Needs review 16 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 16 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“ I don't think this is quite right yet.

  • Status changed to Needs review 15 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Pipeline finished with Success
    15 days ago
    Total: 550s
    #132374
  • Status changed to Needs work 15 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Success
    15 days ago
    Total: 594s
    #132615
  • Status changed to Needs review 15 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to RTBC 15 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    This looks good to me, i don't think we should decrease the size even further.

  • Status changed to Needs review 15 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    What would happen if you had an existing field prefix that's invalid according to the new validation?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Is this something that can be fixed though after the field is created?

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

    I think that is already handled in core/modules/field/src/Entity/FieldStorageConfig::__construct

    if (!preg_match('/^[_a-z]+[_a-z0-9]*$/', $values['field_name'])) {
    throw new FieldException("Attempt to create a field storage {$values['field_name']} with invalid characters. Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character");
    }

  • Status changed to Needs work 11 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #11++ โ€ฆ and #13++ for answering it ๐Ÿ˜„

    But โ€ฆ @narendraR, you literally quoted the code that proves that the validation constraints you added to the config schema are incorrect ๐Ÿ˜…

  • Pipeline finished with Success
    11 days ago
    Total: 506s
    #135304
  • Pipeline finished with Failed
    11 days ago
    Total: 526s
    #135323
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    Adding _ in the end of field prefix failed core/modules/field_ui/tests/src/Functional/ManageFieldsFunctionalTest::testFieldPrefix. Either that test needs to be adjusted as per new validation rule or _ in the end of field prefix is not necessary. For now, I am removing the _ from field prefix end.

  • Status changed to Needs review 11 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Pipeline finished with Success
    11 days ago
    Total: 522s
    #135341
  • Status changed to Needs work 11 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #15: that's not what I meant โ€” I meant disallowing the field name (and hence also the prefix) to start with [0-9].

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

    Sorry, but I don't get #17. As per #13, Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character.

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

    Ah, I see โ€” I had only seen the two underscore-related commits (eafb29c0 and 5b82dd9a), not the one that fixed the regex (f7ea3188) โ€” sorry about that! ๐Ÿ™ˆ

    Indeed, I don't think a trailing _ is required for the prefix โ€” it could also be just foo as a prefix for example.

    I think this looks ready now, except for one nit (left a suggestion for it) and one question/bit of ambiguity.

  • Pipeline finished with Success
    11 days ago
    Total: 493s
    #135581
  • Pipeline finished with Success
    11 days ago
    Total: 586s
    #135595
  • Status changed to Needs review 11 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to RTBC 10 days ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Looks good now!

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    With #11 I actually meant what happens if the field prefix is over 30 characters already (although glad my vague question found a real bug).

  • Status changed to Needs review 10 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Back to needs review for #22.

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

    I'm guessing we need an update path to handle that case. :(

  • Pipeline finished with Success
    8 days ago
    Total: 520s
    #138438
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs review 8 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 8 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Sorry I'm not sure about the update path either - this is silently updating someone's config and field names aren't changeable once they make a new field, which they might do without realising.

    I don't really believe anyone has a 40 (or even 31) character field prefix, but if their field prefix was iloveantidisestablishmentarianismverymuch would we really want to shorten that to <code>iloveantidisestablishmentarianismverymu.

    If all that happens when the field prefix is too long, is they get a validation error if they try to save that form again, could we add a hook_requirements() warning, then they can fix it to a shorter string of their choosing? Or something like that seems OK as long as config import/export is unaffected.

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

    #27: a 40-character long prefix is impossible already thanks to \Drupal\field\Entity\FieldStorageConfig::preSaveNew() doing:

        // Field name cannot be longer than FieldStorageConfig::NAME_MAX_LENGTH
        // characters. We use mb_strlen() because the DB layer assumes that column
        // widths are given in characters rather than bytes.
        if (mb_strlen($this->getName()) > static::NAME_MAX_LENGTH) {
          throw new FieldException('Attempt to create a field storage with an name longer than ' . static::NAME_MAX_LENGTH . ' characters: ' . $this->getName());
        }
    

    I agree that should be better documented though.

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