Disable additionalProperties in slots, props in SDC json schema

Great find! 🤩

The best part: this should not require any update path, nor is it a BC break; this is just tightening SDC's existing JSON Schema! 👏

So we can do this for slots, because we have our own slotDefinition, but not for props, that are using http://json-schema.org/draft-04/schema#: see https://github.com/orgs/json-schema-org/discussions/502#discussioncommen....

Maybe we can use allOf instead of $ref here.

Added 📌 Twig disallows dashes in variable names, so SDC should disallow it in prop names Active as related.

allOf didn't work, as this is an object not an array, so reverted that.

Added a test with invalid slot name, and apparently additionalProperties is ignored, becase this test fails to trigger the expected exception.

Type was never defined in schema for slots, but some components were using it.

I opted for removing it, as slots should be any html fragment, and we aren't validating them anyway.

Even if this is not a BC break technically, it would be in practice, as the components I had to change in core show.

So probably a change record won't hurt, and we might not want to cherry-pick to 11.1.

Hi Penyakisto,

Thanks you for this quick proposal.

You added additionalProperties twice, at the upper slot names level, and at the lower slot definition level:

    "slotDefinition": {
      "type": "object",
+      "additionalProperties": false,
      "patternProperties": {
        "^[a-zA-Z0-9_-]+$": {
          "type": "object",
+          "additionalProperties": false,

The higher level constraint is the main task of this ticket.

Adding the lower level constraint was useful:

• to detect some errors in Core components, where you removed the unexpected type property
• to add the forgotten required property to the schema

But I am still not sure it is a constraint we need to add in this MR (while keeping the other changes anyway). I will need to think a bit about that ;)

I reviewed this and despite knowing basically nothing about SDC architecture, the patch actually makes sense to me. type: string doesn't seem to belong in...a slot definition. :)

Needs work for the change record, but otherwise I feel comfortable RTBCing.

OK for me.

Who is doing the change notice? The one doing the MR (penyaskito)? A Core maintainer?

+1 I think adding additionalProperties at the lower level will be too much of a BC break.. shame we cannot do this via deprecation but for now let's consider that out of scope for now and just add it at the top level. We can still fix those issues that the lower level change found here.

Created child issue for each slot validation as was requested to be split: 🐛 Disable additionalProperties for each SDC slot, enforcing validation Active

I don't think this needs a change record. The other one definitely will.

#13 Anyone can create the change record.

In practice, [a-zA-Z0-9_-]+$ was not actually enforced by the json schema in slot names, but using those in twig as variables was probably already enforcing that.

So don't think we will need the change record here.

11.2.0 release target

Maintainers, please credit larowlan as he only commented on the MRs, but was definitely key on discovering the issue and providing code reviews on gitlab.

nod_ → credited larowlan → .

it is great you have split the subject in 2 tickets.

I will review this very small change and merge it if everything is OK

This is ready to go in my book - thanks for reducing the scope
Over to @pdureau to commit

larowlan → closed merge request !11898

larowlan → reopened merge request !11898

Sorry closed the wrong MR

OK, I will commit

We are only affecting slots, fixed issue title.

Committed d3fa5fa and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.