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

Created on 1 August 2024, 8 months 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 about 14 hours ago

Created by

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
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#

  • Status changed to Needs work 8 days ago
  • 🇺🇸United States mradcliffe USA

    I performed Novice Triage on this issue. I added the Novice issue tag and the Needs issue summary update issue tag so that the notes provided by @pdureau can be added to the issue summary to clarify the scope of the issue.

  • During DrupalCon Atlanta Kwiseman helped me update the proposed resolution.

Production build 0.71.5 2024