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

Hi Wim,

Thanks for this valuable feedback. It makes sense for props (and also slots) machine names to follow machine_name data type: https://www.drupal.org/node/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.

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 alter 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?

@wim-leers the schema lives here now: https://git.drupalcode.org/project/drupal/-/tree/11.x/core/assets/schema...

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

#3470003: Update components to use core metadata.schema.json instead of old SDC module one

@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?

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 delegating the schema to http://json-schema.org/draft-04/schema#

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.

Novice issue reserved for the Mentored Contribution during the DrupalCon Atlanta 2025 contribution day. After 2025.03.27, this issue returns to being open to all. Thanks

During DrupalCon Atlanta Kwiseman helped me update the proposed resolution.

@lirak : can you do a MR please?

Added #3519917: Disable additionalProperties in slots in SDC json schema as related.

The patternProperties regexp means that properties that match that regexp will require those validation rules. But not that properties MUST match that regexp.

I am removing the Novice tag from this issue because it does not have clear steps at this point.

I’m using this documentation as a source: https://www.drupal.org/community/contributor-guide/task/triage-novice-is...

I'm working on this.

Expanded issue title to slots too (still needing IS update).

@pdureau #10: Apparently it's possible!

      "allOf": [
        { "$ref": "http://json-schema.org/draft-04/schema#" },
        {
          "properties": {
            "properties": {
              "additionalProperties": false,
              "patternProperties": {
                "^[a-zA-Z_][a-zA-Z0-9_]*$": {}
              }
            }
          }
        }

Tested this and it worked.

I went a bit further and constrained to: ^[a-zA-Z_][a-zA-Z0-9_]*$, AKA the php valid variable validator, which in absence of more concrete twig docs, it's what twig supports: start with letter or underscore, contain letters, digits, underscores. The only difference is that php allows non-ASCII characters. I think we are fine being more restrictive, because we weren't allowing that before anyway.

Expanded test coverage covering these, both for props and slots.
We were already using additionalProperties: false, so are enforcing all of them.

For variants we don't need this, they don't convert to php/twig variables. Left meta:enum untouched too.

I used $comment for documenting this too. This is hard to maintain, so any help should be welcome. But happy to remove that if you don't agree.

Disclosure: I used AI (Claude - Opus 4.7) for validating my assumptions with variants and meta:enums not affected, and for comparing our new regex with the one at https://www.php.net/manual/en/language.variables.basics.php and explaining the differences.

Updated IS update.

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.

@pdureau #10: Apparently it's possible! Tested this and it worked.

Wow! That sounds great. I am a bit surprised with a schema without a type at the root level, my JSON schema knowledge is still expanding.

For variants we don't need this, they don't convert to php/twig variables. Left meta:enum untouched too.

Indeed, they are variable values, not variables names.

draft-06 is supported by justinrainbow, and that would have support for propertyNames.

Seems there's 1 open thread from @penyaskito and 1 comment about a follow up.

Thanks for responding to it. Believe all feedback has been addressed for this one.

Debated whether to add a change record but as far as I can see if you tried to do this before it never would have worked properly anyway.

As this is a minor behaviour change let's just ship it in 11.5 and 12.0 only.

Committed and pushed 7dd6a4c9d48 to main and ea8be0f9dc3 to 12.0.x and 29fac2e9834 to 11.x. Thanks!

Canvas has test-slot, test-slot-only-required, new-slot and 0-slot, which is how I came here :) While that indeed would not have worked, this fails Canvas tests on what will become 11.5. Opened https://git.drupalcode.org/project/canvas/-/work_items/3592273 for this.

This may or may not indicate a change record is needed since dashes only worked in a limited way in tests, not in actual rendering, even in Canvas.