Twig disallows dashes in variable names, so SDC should disallow it in prop names

Created on 1 August 2024, about 2 months ago
Updated 23 August 2024, about 1 month ago

Problem/Motivation

Strictly speaking, this is not a bug in SDC. But … I just lost an hour over something SDC could've easily warned against:

LOLOL an hour worth of debugging only to find out that SDC prop names may contain dashes but Twig variable names must not: https://github.com/twigphp/Twig/issues/1967 — for example

    {{ test-REQUIRED-string }}

is compiled by Twig to:

        // line 5
        yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, ((($context["test"] ?? null) - ($context["REQUIRED"] ?? null)) - ($context["string"] ?? null)), "html", null, true);

… which means that the DX of SDC is leaving a lot to be desired: if Twig doesn't support this, and SDC supports only Twig, then I expect SDC to not allow this either.

#3463583-15: Use the `all-props` component for testing form backend + frontend integration

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component
single-directory components 

Last updated 1 day ago

Created by

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @wim leers
  • 🇫🇷France pdureau Paris

    Hi Wim,

    Thanks for this valuable feedback. It makes sense for props (and also slots) machine names to follow machine_name data type : [#2954832]

    With:

      constraints:
        Regex: '/^[a-z0-9_]+$/'
        Length:
          max: 64
    

    So:

    foo ✅
    foo1 ✅
    111 ✅
    foo_bar ✅
    foo-bar ❌
    foo bar ❌
    

    We can enforce it to patternProperties in JSON Schema.

  • 🇺🇸United States cosmicdreams Minneapolis/St. Paul

    I started looking into making this. On the surface this seems to be the responsibility of Drupal/Core/Theme/ComponentPluginManager.php. That would be a place where we could later the schema validation logic of a Component definitition.

    But the configuration shown above seems to be a solution that can be applied to each individual component schema definition. As in, it would be the responsibility of the developer to include the schema constraint per property.

    Are you saying it's possible to modify the schema of a component, through the same config-based fix, that would impact the default definition of any property the component might have?

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    #3++ … except … that

    #4: No, @pdureau is proposing to update https://git.drupalcode.org/project/sdc/-/raw/1.x/src/metadata.schema.json … which interestingly lives outside Drupal core 😱😅 (Is there an issue to change that? Was that a conscious choice? I cannot find docs covering that. 🤔)

  • 🇺🇸United States Kristen Pol Santa Cruz, CA, USA

    Thanks for that... added an XB issue as a result:

    📌 Update components to use core metadata.schema.json instead of old SDC module one Active

  • First commit to issue fork.
  • 🇬🇷Greece vensires

    @pdureau, I am not so familiar with the *.schema.json syntax etc but we currently have the following:

    "props": {
      "$ref": "http://json-schema.org/draft-04/schema#"
    },
    "slots": {
      "$ref": "#/$defs/slotDefinition"
    },
    

    So, where could these constraints be added in the schema.json file?

  • 🇫🇷France pdureau Paris

    Hello Panagiotis,

    This would be my (non tested) proposal for slots. Remove the - from /"$defs"/slotDefinition/patternProperties/"^[a-zA-Z0-9_-]$". So: /"$defs"/slotDefinition/patternProperties/"^[a-zA-Z0-9_]$"

    Unfortunately, I don't think it is possible to enforce it for props because we are edelegating the schema to http://json-schema.org/draft-04/schema#

Production build 0.71.5 2024