Allow schema references in Single Directory Component prop schemas

Hello e0ipso,

On your "Single Directory Components in Drupal Core " article, there is this code snippet:

props:
  type: object
  properties:
    attributes:
      type: Drupal\Core\Template\Attribute
      title: Attributes

Source: https://www.lullabot.com/articles/getting-single-directory-components-dr...

It is managed by ComponentValidator::getClassProps() but:

• does it means SDC JSON schemas using PHP class as prop types are not proper JSON schema anymore?
• is it relevant to use it outside validation?

Seems like this is SUPER important for DX, to avoid repeating the same definitions over and over again (and preventing subtle inconsistencies)? 🤓

Or am I misunderstanding this? How is it possible that we only have core/modules/sdc/src/metadata.schema.json and core/modules/sdc/src/metadata-full.schema.json today, because based on this issue I'd think there'd be many *.schema.json files? 🤔

Hi Wim,

We "solved" this issue in UI Patterns 2.x.

We have added a new "prop type" plugin type which have a JSON schema annotation.

Examples:

 *   schema = {
 *     "type": "string",
 *     "pattern": "^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$"
 *   }

https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Plugin/U...

 *   schema = {
 *     "type": "string",
 *     "format": "iri-reference"
 *   }

https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Plugin/U...

A developer can use JSON schema references targeting the prop type plugin ID:

props:
  type: object
  properties:
    bg:
      title: "Background"
      "$ref": "ui-patterns://color"
    src:
      title: "Media source"
      "$ref": "ui-patterns://url"

The URL are resolved by justinrainbow/json-schema thanks to a stream wrapper service https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/SchemaMa...

With a bit of help to manage the recursive references: https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/SchemaMa...

So, the "$ref" is replaced by the schema of the prop type plugin annotation.

This system is extensible by adding new prop type plugins.

We would be happy to get rid of our implementation if Core is solving this issue too.

1. IMHO this is super important. Without somewhat precise metadata/schema/definitions of what the expected shapes and semantics for an SDC prop are … ill-fitting (shape: array instead of string, string instead of object …) or nonsensical (semantics: phone number instead of e-mail address, prose instead of URI …) will be passed, and the SDC will fail to render.

   So: bumping to Major.

2. I don't think this should be postponed on #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency. In fact, it's the opposite: this is the issue that needs to be so clearly valuable that it'll convince core committers #3365985 is worth doing! Until we get to that point, the MR for this issue should just add that dependency … and then once this is RTBC, we'll postpone this issue on that issue, and land the dependency addition there first.
3. @pdureau in #17 has demonstrated a very "Drupal ecosystem-friendly" way to do this, and I'm curious about @e0ipso's thoughts on going either with his original proposal in the issue summary and implemented in #9, or with @pdureau's alternative implementation? 🤓 → marking Needs review!

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

One goal that I want to keep in mind is that this needs to have a very low friction.

+1

That's why I initially thought about letting components reference props in other components. This had the benefit of not having to do anything to declare the referenceable schemas. […] Sadly, this is not viable. Mainly because the referenced component may be deleted, and then the schema would break.

I think in #7 and #9, you were already heading in a different direction though, that would not be dependent on component existence? There it'd be individual extensions (modules/themes) that can declare additional JSON schema definitions. Since SDCs always ship as part of an extension (module or theme), it would be guaranteed that that JSON schema file will exist (thanks to containing module/theme's declared dependencies).

So +1 to: I think the easiest is to depend on modules / themes.

#7 proposes a JSON file (with any name) in a module, which can be referenced as module:/modulename/path-to-any-file#json-path-to-schema-definition. This may be too lose, and therefore give pause and be hard to document.

Why would it be too loose? 🤔

Perhaps we want to enforce a specific name for the file (how about components/prop-types.json?), and then reference it component-schemas:/modulename#json-path-to-schema-definition.

👆 YES! 🤩 Heck, it could even be json-schema-definitions://<extension name>#$defs/<DEFNAME>, so for example json-schema-definitions://image_gallery#$defs/galleryTitleAndCaption and core could provide commonly used things like json-schema-definitions://core#$defs/image!

Tangentially related, but off-topic:

This stems from the fact that there is no dependency chain between components (which I believe it's a good thing).

🤔 I was wondering about this. Does that also mean it's impossible to compose SDCs using only SDCS? I.e. put SDC B into SDC A's slot, and declare that result as SDC C?

In the PoC I'm working on for Experience Builder, I was able to transform this SDC prop metadata:

    test-string-format-uri-image:
      title: 'String, format=uri, images only'
      type: string
      format: uri
      # @see \Drupal\image\Plugin\Field\FieldType\ImageItem::defaultFieldSettings()
      pattern: '\.(png|gif|jpg|jpeg|webp)$'

to

    test-string-format-uri-image:
      title: 'String, format=uri, images only'
      $ref: "json-schema-definitions://experience_builder.module/image-uri"

thanks to a new ./schema.json file in my module named experience_builder, as well as the more complex SDC prop metadata

    test-object-drupal-image:
      type: object
      required:
        - src
      properties:
        src:
          title: 'Image URL'
          type: string
          format: uri
          # @see \Drupal\image\Plugin\Field\FieldType\ImageItem::defaultFieldSettings()
          pattern: '\.(png|gif|jpg|jpeg|webp)$'
        alt:
          title: 'Alternative text'
          type: string
        width:
          title: 'Image width'
          type: integer
        height:
          title: 'Image height'
          type: integer

to

    test-object-drupal-image:
      $ref: "json-schema-definitions://experience_builder.module/image"

thanks to:

{
  "$defs": {
    "date-range": {
      "title": "date range",
      "type":  "object",
      "required": ["from", "to"],
      "properties": {
        "from": {
          "title": "Start date",
          "type": "string",
          "format": "date"
        },
        "to": {
          "title": "End date",
          "type": "string",
          "format": "date"
        }
      }
    },
    "image-uri": {
      "title": "Image URL",
      "type": "string",
      "format": "uri",
      "pattern": "\\.(png|gif|jpg|jpeg|webp)$"
    },
    "image": {
      "title": "image",
      "type":  "object",
      "required": ["src"],
      "properties": {
        "src": {
          "title": "Image URL",
          "$ref": "json-schema-definitions://experience_builder.module/image-uri"
        },
        "alt": {
          "title": "Alternative text",
          "type": "string"
        },
        "width": {
          "title": "Image width",
          "type": "integer"
        },
        "height": {
          "title": "Image height",
          "type": "integer"
        }
      }
    }
  }
}

plus a new stream wrapper service inspired by @pdureau's superb work in ui_patterns 2.0.x (see #17 — I'd never have found it otherwise!).

The SdcPropToFieldTypePropTest kernel test is still passing, meaning it's still finding Drupal field type props that match 1:1 shape-wise and semantically into exactly those SDC props. (Grep for the string ℹ︎␜entity:node:foo␝field_silly_image␞␟{src↝entity␜ℹ︎␜entity:file␝uri␞␟value, alt↠alt, width↠width, height↠height} — that's the string representation of the expression that defines the mapping from Drupal field type props into SDC props).

Code: https://git.drupalcode.org/project/experience_builder/-/commit/f784af412...

I would advocate for a name that is more specific to components. I propose components/shared-schemas.json. This is because it is scoped to the components/ folder, and it conveys that you only need this if you want to share definitions.

I actually did that intentionally: the idea would be that other things in Drupal core or the general Drupal (contrib) ecosystem could use this same infrastructure. For SDC's purposes, we'd verify that there'd be a top-level $defs (with definitions underneath), but anything else in that file it'd just ignore.
… but that's probably a premature abstraction, so I'm fine with changing that — it was just a proposal 🤓 I liked the simplicity of /schema.json, but I don't feel strongly about it.

which accomplishes the same. The advantage is that by following their pattern we can resolve schemas recursively every time

Are you sure that actually works? https://github.com/justinrainbow/json-schema/issues/427 suggests it does not?

@lauriii in DM:

$ref: sdc-prop-type://experience_builder.module/image seems still a bit too verbose to remember by heart. Ideally it would be as simple as type: 'drupal-image'.

My response:
That’s not valid JSON Schema. Defining new types is not permitted. Although https://json-schema.org/understanding-json-schema/reference/schema#vocab... seems to be changing that … 🤞 Per https://github.com/json-schema-org/json-schema-vocabularies, there aren’t a whole lot of examples just yet.

@lauriii again:

I understand that, but we need to find a way to workaround that somehow because $ref: sdc-prop-type://experience_builder.module/image is not acceptable DX.

So I'm curious what you think, @e0ipso.

Still, trying to indulge Lauri here, by taking the above to its logical conclusion: :nerd_face:

The use of a dialect like I mentioned above would allow us to declare:

image:
  type: object
  format: drupal-image

instead of

image:
  $ref: sdc-prop-type://experience_builder.module/image

but with one significant downside: now you need a custom JSON schema implementation everywhere you want to run validation. See https://json-schema.org/understanding-json-schema/reference/schema#guide....

I understand that, but we need to find a way to workaround that somehow because $ref: sdc-prop-type://experience_builder.module/image is not acceptable DX.

What about $ref: module://experience_builder/image ?

This proposal with static declarations in shared-schemas.json is exciting.

But, while implementing it, let's not block the use of alternative and complementary reference systems, like the one we already use in UI Patterns 2: ui-patterns://color where "color" is a plugin ID.

They all can live together inside a single reference solver mechanism, because the implementation is the same until the stream wrappers. What happens after the stream wrappers can differ.

This proposal with static declarations in shared-schemas.json is exciting.

+1!

Nothing to add to #27 — thanks for articulating it so clearly, @e0ipso 😊

Creating components is a common enough task that it should be really easy for someone who is a) not familiar with Drupal b) not a senior developer and hence c) does not know JSON-Schema. This is highly relevant for workflows where the user has a pre-existing design system that they want to integrate with Drupal. For this persona, type: 'drupal-image'would be the north star experience because they don't care whether we use JSON-Schema or not.

That said, using JSON-Schema as a starting point makes sense, but we should not let it define what is the developer experience we provide. We just don't want to require users to learn JSON-Schema in order to be successful. Because of this, we need to consider how we can simplify some of the more complex workflows instead of being dogmatic about implementing the specification.

What's being proposed in #26 seems to be making this more reasonable already. However, even that could be seen as a strange by someone who is not familiar with the syntax. This user might ask; what is difference between $ref and type, why do I now need these two now, and why does the value need to follow a strange pattern with ://.

@e0ipso are you saying that in your mind, the syntax proposed in #26 leads into a better DX for a user who is a) not familiar with Drupal b) not a senior developer c) and hence does not know JSON-Schema? Is it because it's able to better solve the problems you are raising in #30?

I haven't read all the discussion here, but I noticed some discussion about the DX of JSON schema, or certain decisions we might want to make about the details of the JSON schema we want to use, so I want to quickly mention that https://github.com/vega/ts-json-schema-generator is a pretty great library that lets you convert TypeScript definitions to JSON schema. In other words, if we want to optimize for DX, we might want to consider a DX where people can write TypeScript definitions instead of writing JSON schema by hand, and then the JSON schema creation can be automated which would allow us to optimize the JSON schema to suit SDC's needs rather than optimizing for manually writing it.

A declarative format is better than an executable format for such definitions. No runtime needed, no performance issue, no security risk.

https://en.m.wikipedia.org/wiki/Rule_of_least_power

we are not writing JSX components in Drupal (yet).

Yet? Oh god 😳

Agreed that the source of truth should always be a JSON schema definition.

But I think @effulgentsia was merely stating that it's possible to provide a TypeScript-only DX that generates such a JSON schema definition. I doubt he's proposing to enforce it, nor make it the default.

The fact that it's possible is … nice, interesting and potentially powerful eventually :)

Ended up here through #3514072: SDC slots can set expectations and cardinality , I like $ref: module://experience_builder/image from #26.

We agreed on JSON-schema for props, let's follow-though on our decision and build on this as explained by @e0ipso and @pdureau (our 2 SDC maintainers). The DX concerns can be handled once things actually work, there are ways to address them as demonstrated by @effulgentsia in #33. We can have a whole layer of SDC generation that smooth out things for end users in contrib if it's not nice enough by default. The important thing here is that it works reliably in a non-drupal specific way

A little up-to-date summary of the 2 existing JSON Schema stream wrappers in contrib.

Experience Builder

  Drupal\experience_builder\JsonSchemaDefinitionsStreamwrapper:
    public: true
    tags:
      - { name: stream_wrapper, scheme: json-schema-definitions }

Example: json-schema-definitions://foo.module/bar

JsonSchemaDefinitionsStreamwrapper service is:

1. Extracting the extension and the JSON schema $def ID from the URI. Example: foo module as extension and bar $def
2. Load the schema.json file from the extension path. Example: /modules/contrib/foo/schema.json
3. Return the extracted $def

Pro: is generic, any module can provide JSON schema definition without any dependency to Experience builder outside this stream wrapper.
Con: only static declaration

UI Patterns 2

  ui_patterns.schema_stream_wrapper:
    class: Drupal\ui_patterns\SchemaManager\StreamWrapper
    arguments:
      - "@plugin.manager.ui_patterns_prop_type"
    tags:
      - { name: stream_wrapper, scheme: ui-patterns }

Example: ui-patterns://bar

Streamwrapper service is:

1. Asking the PropType plugin manager for the plugin with bar ID
2. Loading the plugin and return the schema from the PHP attributes

Pro: $defs are shipped alongside some niceties (type converter, data normalizer...) as a cohesive mechanism.
Con: a dependency to plugin.manager.ui_patterns_prop_type service

Proposal

We can go the Experience Builder way because it is easier to ask UI Patterns 2 maintainers to add and maintain a schema.json file in their module instead of introducing a new plugin type to Core.

Also, this may be the opportunity to:

• simplify a bit the URI scheme (previous proposal was $ref: module://experience_builder/image but we can propose another schema if "module" or "theme" are too generic) because this kind of URL will be typed a lot by component authors, with a lot of chance of mistakes
• considerate Mateu's advice to use URI anchors to target the $defs ($ref: <uri>#<locator>) to leverage existing JSON schema mechanisms instead of extracting manually like JsonSchemaDefinitionsStreamwrapper is actually doing (however, I would advise to explore $anchor keyword instead of the propsoed JSON pointer)

So, if we mix both, we can have something like that: $ref: module://experience_builder#image

This may not be enough

However, this proposal is not solving another issue reported by @finnsky : #3480464: [2.1.x] Make the SDC scheme more universal.

We are becoming more and more generic, but it is still a Drupal only proposal (module and theme scheme, schema.json convention...).

What can we do to make those references working outside of Drupal (for example: in storybook)?

Modules and themes are sharing a single namespace, so we have the opportunity to do something like drupal:// or extension:// instead of module:// and theme://

What can we do to make those references working outside of Drupal (for example: in storybook)?

Or if we have an image component that we want to be able to use with and without Experience Builder.

Is it possible to have a fallback value? Maybe something like

$ref: 
  - module://experience_builder#image
  - theme://mytheme#image

As far as I know, Json schema allows only one value in $ref

https://json-schema.org/understanding-json-schema/structuring

We can't use module:// and theme:// because the schemes are already "taken" by #1308152: Add stream wrappers to access .json files in extensions.

By the way, we can go fully the #1308152: Add stream wrappers to access .json files in extensions way instead of adding our dedicated wrapper.

Also, do we still need a schema resolver to help justinrainbow/json-schema resolving recursive structures like UI Patterns is doing? ( justinrainbow/json-schema got new releases since last year)

So, component users, will need to explicitly :

$ref: module://experience_builder/schema.json#image
$ref theme://mytheme/schema.json#image

This proposal is interesting because it is not a SDC specific proposal with magic loading of data in an arbitray named file, but a combination of:

• Upcoming Drupal wide stream wrappers
• A standard, JSON Pointer, to locate a data sub-structure within a file.

Schemas: proposal for inclusion

So, this MR may be empty of PHP :) That would be nice. But it will also be the opportunity to add some schemas directly in Core modules.

In a perfect Drupal world, neither UI Patterns nor Experience builder will bring its own schemas to avoid dependencies from themes to those display building modules.

So, proposal:

type: object
required:
- src
properties:
  src:
    "$ref": module://image/schema.json#image-uri
  alt:
    type: string
  width:
    type: integer
  height:
    type: integer

type: string
format: uri-reference
pattern: "^(/|https?://)?.*\\.([Pp][Nn][Gg]|[Gg][Ii][Ff]|[Jj][Pp][Gg]|[Jj][Pp][Ee][Gg]|[Ww][Ee][Bb][Pp]|[Aa][Vv][Ii][Ff])(\\?.*)?(#.*)?$"

type: object
patternProperties:
  ".+":
    anyOf:
    - type:
      - string
      - number
    - type: array
      items:
        anyOf:
        - type: number
        - type: string

type: array
items:
  type: object
  properties:
    title:
      type: string
    url:
      type: string
      format: iri-reference
    attributes:
      "$ref": module://system/schema.json#attributes
    link_attributes:
      "$ref": module://system/schema.json#attributes
    below:
      type: array
      items:
        type: object

type: string
pattern: "(?:--|-?[A-Za-z_\\x{00A0}-\\x{10FFFF}])[A-Za-z0-9-_\\x{00A0}-\\x{10FFFF}\\.]*"

What else?

Giving a try. Still targeting11.3

It may also be the opportunity to remove this from ComponentMetadata:

      // All props should also support "object" this allows deferring rendering
      // in Twig to the render pipeline.
      $schema_props = $metadata_info['props'];
      foreach ($schema_props['properties'] ?? [] as $name => $prop_schema) {
        $type = $prop_schema['type'] ?? '';
        $schema['properties'][$name]['type'] = array_unique([
          ...(array) $type,
          'object',
        ]);
      }

Which is making a mess with the JSON schema. We need to rely only on what the component author provides. Let's check if this removal breaks tests and contrib projects like UI Patterns or Canvas (it must be OK).

This will require the library justinrainbow/json-schema to be vetted as runtime drupal core dependency. Which is a big blocker.

Indeed. So we are dependent on 2 other issues:

• #1308152: Add stream wrappers to access .json files in extensions
• #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency

My first MR will embed them. Let's have a working proposal first.

Also, let's check if the proposal will work with HTTP URI to see if we cover: #3480464: [2.1.x] Make the SDC scheme more universal.

Merge request opened with 3 commits: https://git.drupalcode.org/project/drupal/-/merge_requests/13457

2 commits from other issues

A squash of #1308152: Add stream wrappers to access .json files in extensions

✅ Seems to work well.

An implementation of #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency

We simply moved the requirement out of dev.

⚠️ However:

 $ composer validate
./composer.json is valid but your composer.lock has some errors

composer update --lock fix this but is making the diff huge.

The scope of the current issue

The addition of ComponentPluginManager::resolveJsonSchemaReferences()

That's cool because we use only "standards":

• a generic, Drupal wide, stream wrapper
• justinrainbow/json-schema with JSON pointers

❌ However, some JSON objects, all under anyOf, are transformed into stdClass instead of an array. That's the reason why phpunit fails.

1 definition coming from Canvas

module://image/schema.json#image as a json-schema-definitions://canvas.module/image replacement.

⚠️ Not exactly the same as Canvas because of this JSON schema error:

Invalid regex format ^(/|https?://)?.*\.([Pp][Nn][Gg]|[Gg][Ii][Ff]|[Jj][Pp][Gg]|[Jj][Pp][Ee][Gg]|[Ww][Ee][Bb][Pp]|[Aa][Vv][Ii][Ff])(\?.*)?(#.*)?$

2 definitions coming from UI Patterns 2

Those one have nested references, so we can check this mechanism is also OK:

• module://system/schema.json#attributes as a ui-patterns://attributes replacement.
• module://system/schema.json#links as a ui-patterns://links replacement.

Update ComponentValidator

Add a little security t in the "Ensure that all property types are strings" logic introduced in Drupal 11.2 in case the $ref was not resolved successfully. It is how the schema resolver of justinrainbow works.

The removal of the {"type": "object"} addition

With an adaptation of ComponentMetadataTest::testMetadata().

A kernel test

core/tests/Drupal/KernelTests/Components/SchemaReferenceResolverTest.php with core/modules/system/tests/themes/sdc_theme_test_references

Updated.

Some pipepline jobs KO but I don't see if it is related to my current work: https://git.drupalcode.org/issue/drupal-3352063/-/pipelines/625236

Locally, phpunit -c core/ --group sdc is OK

Commits from other issues

A squash of #1308152: Add stream wrappers to access .json files in extensions

✅ Seems to work well.

An implementation of #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency

We simply moved the requirement out of dev.

✅ Seems to work well after composer update --lock.

The scope of the current issue

The addition of ComponentPluginManager::resolveJsonSchemaReferences()

That's cool because we use only "standards":

• a generic, Drupal wide, stream wrapper
• justinrainbow/json-schema with JSON pointers

✅ works OK

I was a bit afraid json_decode(json_encode($schema), TRUE) would be too slow but I saw than the equivalent logic drom justinrainbow is doing the same: https://github.com/jsonrainbow/json-schema/blob/main/src/JsonSchema/Cons...

1 definition coming from Canvas

module://image/schema.json#image as a json-schema-definitions://canvas.module/image replacement.

⚠️ Not exactly the same as Canvas because of this JSON schema error:

Invalid regex format ^(/|https?://)?.*\.([Pp][Nn][Gg]|[Gg][Ii][Ff]|[Jj][Pp][Gg]|[Jj][Pp][Ee][Gg]|[Ww][Ee][Bb][Pp]|[Aa][Vv][Ii][Ff])(\?.*)?(#.*)?$

2 definitions coming from UI Patterns 2

Those one have nested references, so we can check this mechanism is also OK:

• module://system/schema.json#attributes as a ui-patterns://attributes replacement.
• module://system/schema.json#links as a ui-patterns://links replacement.

Update ComponentValidator

Add a little security t in the "Ensure that all property types are strings" logic introduced in Drupal 11.2 in case the $ref was not resolved successfully. It is how the schema resolver of justinrainbow works.

The removal of the {"type": "object"} addition

With an adaptation of ComponentMetadataTest::testMetadata().

A kernel test

core/tests/Drupal/KernelTests/Components/SchemaReferenceResolverTest.php with core/modules/system/tests/themes/sdc_theme_test_references

Did a first round of review, found a few concerns. 😇

Looking forward to #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency landing, which will be step #1 to simplify this MR!

Will look at the comment and move the proposed schema to test module. Adding schema will be its own issue for a next Drupal release.

Keeping on my side because of the PHPunit errors: https://git.drupalcode.org/issue/drupal-3352063/-/jobs/6934948/viewer

However, they are all related to Drupal\KernelTests\Core\StreamWrapper\ExtensionStreamTest so it has to be fixed in #1308152: Add stream wrappers to access .json files in extensions

OK for review.

The duplicate of #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency has been removed.

The duplicate of #1308152: Add stream wrappers to access .json files in extensions is still here.

No phpunit error anymore. A functional test error (Drupal\Tests\field_ui\FunctionalJavascript\ManageDisplay) but doesn't seem related to the current work.

@wim, we have 2 ongoing discussions on https://git.drupalcode.org/project/drupal/-/merge_requests/13457/diffs

What are you recommending? We move to dedicated issue or not?

I think that's prudent, yes.

All changes to ComponentMetadata and ComponentMetdataTests has been moved to specific issue #3554720: Remove addition of a object type in all props.

#1308152: Add stream wrappers to access .json files in extensions is merged, I will rebase and send back to review.

Rebase is done.

The MR is full green and only 30-40 line of PHP code added to Core (blank lines, comments and tests removed).

I take it back for:

• This is a silent failure. Shouldn't this be an explicit failure, to inform developers?
• Can you please add a test case that points to module://some_thing or theme://some_thing — i.e. a module or theme name with an underscore in it?

Per https://git.drupalcode.org/project/drupal/-/merge_requests/13457#note_63..., I think #1308152: Add stream wrappers to access .json files in extensions would need to be changed to actually generate valid URIs that JSON Schema would not consider validation errors.

Either that, or justinrainbow/json-schema's \JsonSchema\Tool\Validator\UriValidator::isValid() is wrong 😑

However … most other URI validators are fine with it. So perhaps … it is related to the fact that DNS host names aren't allowed to contain underscores, but non-DNS-based hostnames are fine? It is https://www.rfc-editor.org/rfc/rfc952 that dictates underscores are forbidden, but the same doesn't necessarily apply here.

Hm, checking https://www.ietf.org/rfc/rfc3986.txt:

   URI           = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
…
   hier-part     = "//" authority path-abempty
…
   authority     = [ userinfo "@" ] host [ ":" port ]
…
   host          = IP-literal / IPv4address / reg-name
…
   reg-name      = *( unreserved / pct-encoded / sub-delims )
…
   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"

There it is: underscores are allowed.

So

([a-z0-9.-]+|\[[a-f0-9:.]+\])            # Hostname or IPv6 in brackets

in \JsonSchema\Tool\Validator\UriValidator::isValid() is wrong. Despite it literally referencing RFC 3986. Still wrong in the latest release: https://github.com/jsonrainbow/json-schema/blob/6.6.1/src/JsonSchema/Too....

😑

This will need an upstream bugfix, but we can work around it here … although it will require duplicating literally the entirety of \JsonSchema\Constraints\FormatConstraint, or decorating it and calling it for everything except format: uri|uri-reference|iri|iri-reference 🤠

This will still need that underscore test coverage I requested, because without it, module://content_translation/whatever would cause \JsonSchema\Tool\Validator\UriValidator::isValid() to fail.

Posted a one-line work-around to not have to do the very big amount of work that #65 describes: https://git.drupalcode.org/project/drupal/-/merge_requests/13457/diffs#n... — brought to you by my sibling Canvas MR at https://git.drupalcode.org/project/canvas/-/merge_requests/255.

@Wim as I posted over in #1308152: Add stream wrappers to access .json files in extensions while the grammar allows underscores, the RFC still discourages names that are not valid DNS records, which is why I think validation tools want to ensure they conform:

URI producers should use names that conform to the DNS syntax, even when use of DNS is not immediately apparent

But this is a "should", not a "must".

PHP 8.5 now has a spec-compliant URI parser, and there's also a polyfill for it for PHP 8.1+. I think we should file an upstream issue for JsonSchema to switch out their incorrect regex for this. In the meantime, does JsonSchema support us overriding its UriValidator?

I opened https://github.com/jsonrainbow/json-schema/pull/853 to see if upstream will help us out here.

@effulgentsia because justinrainbow/json-schema is a dependency of Composer, they are extremely conservative in PHP support; the 6.x branch still supports PHP 7.2, so any polyfill would have to go back that far. It's also a much bigger change than just tweaking the regex which I think is enough for us here.

https://github.com/jsonrainbow/json-schema/releases/tag/6.6.2 should fix the issue.

That was so efficient! It was such a good idea to go upstream. Thanks @longwave.

Wow!!!

Can we now bump core's requirement to require 6.6.2?

Also, this was discovered independently by somebody else on Canvas: #3560455: [upstream] Stream wrapper URIs are incorrectly flagged as invalid (hostname misinterpreted) due to bug in `justinrainbow/json-schema` — bump minimum version to 6.6.2 — what a coincidence!

Can we now bump core's requirement to require 6.6.2?

Can I add it this to the MR or does it need a specific MR?

That can be added here, as it's this issue that requires the new version.

Pipeline running...

This is a silent failure. Shouldn't this be an explicit failure, to inform developers?

Done. Simply by commiting Wim's proposal.

Could you make this available for contrib modules (such as Canvas) to call, too?

Done, but I have added a new public method instead of changing the existing one, in order to keep internal mechanisms private. IS it OK for you?

Can you please add a test case that points to module://some_thing or theme://some_thing — i.e. a module or theme name with an underscore in it?

It is already the case. Theme for tests is sdc_theme_test_references with references to URL like such: theme://sdc_theme_test_references/schema.json#image

Can we now bump core's requirement to require 6.6.2?

Done. From ^5.2 || ^6.5.2 to ^6.6.2

Green :)

Given the amazing upstream contributing experience, I asked the jsonrainbow/json-schema maintainer whether they'd be interested in accepting a PR from us to add recursive $ref resolving. That'd mean we could omit ComponentPluginManager::resolveJsonSchemaReferences() from this MR. Or, at least have to not maintain it forever. (It really does make sense to live in that project, after all.)

See https://github.com/jsonrainbow/json-schema/pull/853#issuecomment-3595541100

1. Removed Needs tests thanks to #69 and https://github.com/jsonrainbow/json-schema/releases/tag/6.6.2, and this MR bumping that dependency requirement :)
2. Modifying a $ref of a required prop to become point to a nonsensical URI, that the exception is swallowed (i.e. does not cause a crash) except when verbose error reporting (ERROR_REPORTING_DISPLAY_VERBOSE) is enabled — I then get a \JsonSchema\Exception\ResourceNotFoundException with this message:
   

   file_get_contents(https://example.com/aaasdfsdf): Failed to open stream: HTTP request failed! HTTP/1.1 404 Not Found
   

   (which required one tiny bugfix)

3. Manually tested this MR against Canvas 1.0, with one change:
   

   diff --git a/src/CanvasServiceProvider.php b/src/CanvasServiceProvider.php
   index 50ffa5974..366f0bb19 100644
   --- a/src/CanvasServiceProvider.php
   +++ b/src/CanvasServiceProvider.php
   @@ -57,9 +57,6 @@ class CanvasServiceProvider extends ServiceProviderBase {
          [new Reference(Validator::class)]
        );
    
   -    // @todo Remove this once Canvas relies on a Drupal core version that includes https://www.drupal.org/i/3352063.
   -    $container->getDefinition('plugin.manager.sdc')
   -      ->setClass(ComponentPluginManager::class);
        // @todo Remove in clean-up follow-up; minimize non-essential changes.
        $container->setAlias(ComponentPluginManager::class, 'plugin.manager.sdc');
   

   … which means Canvas starts relying on the updated ComponentPluginManager that this MR introduces. A crucial kernel test passes, meaning Canvas will indeed be able to drop this, and use the exact same resolving logic, which of course is the intent. 👍 (And it's what Canvas forward-ported in #3515074: Shape matching MUST work with the resolved equivalents of $refs AND must be compatible with core's upcoming $ref resolving in SDCs.)

4. Needing release manager sign-off on the tiny new public API: \Drupal\Core\Theme\ComponentPluginManager::resolveJsonSchemaReferences() — see https://git.drupalcode.org/project/drupal/-/merge_requests/13457/diffs#n...

Thanks, @pdureau! 🤝

Quoting myself from #78:

Given the amazing upstream contributing experience, I asked the jsonrainbow/json-schema maintainer whether they'd be interested in accepting a PR from us to add recursive $ref resolving. That'd mean we could omit ComponentPluginManager::resolveJsonSchemaReferences() from this MR. Or, at least have to not maintain it forever. (It really does make sense to live in that project, after all.)

See https://github.com/jsonrainbow/json-schema/pull/853#issuecomment-3595541100

They responded there. It looks like they think we would not need to implement our own recursive $ref resolving. They asked to double-check, so I did in a new test-only MR to avoid disrupting the RTBC MR: I removed the recursion, which results in failures:

1) Drupal\KernelTests\Components\SchemaReferenceResolverTest::testReferenceResolver
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'image' => Array (
-        'type' => 'object'
-        'required' => [...]
-        'properties' => [...]
+        '$ref' => 'theme://sdc_theme_test_refere...#image'
     )
     'attributes' => Array (
-        'type' => 'object'
-        'patternProperties' => [...]
+        '$ref' => 'theme://sdc_theme_test_refere...ibutes'
     )
     'links' => Array (
-        'type' => 'array'
-        'items' => [...]
+        '$ref' => 'theme://sdc_theme_test_refere...#links'
     )
     'unresolvable' => [...]
 )

Plus, even if it works upstream we still have our own special-casing now: “swallow all errors for invalid $refs unless they are for a required required prop AND verbose error logging is enabled".

They asked to double-check, so I did in a new test-only MR to avoid disrupting the RTBC MR: I removed the recursion, which results in failures

I got the same results.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

3352063-11.x-pdureau and
3352063-prove-to-upstream-this-is-needed has been rebased.

They asked to double-check, so I did in a new test-only MR to avoid disrupting the RTBC MR: I removed the recursion, which results in failures.

So I hide the experimental branch which is failing phpunit by deisgn: 3352063-prove-to-upstream-this-is-needed

Test are passing. Additional tests didn't get triggered but I guess it is in purpose.

Someone want to have a fresh look?

There is still at least one open topic to discuss: from @longwave with @effulgentsia about justinrainbow/json-schema being extremely conservative in PHP support

And I am not sure about our modifications of the root's composer.json

@deviantintegral left a review in the MR.

I will address some of Andrew's and Christian's feedbacks

And I am not sure about our modifications of the root's composer.json

Fixed, following documentation: https://www.drupal.org/about/core/policies/core-dependency-policies-and-...

It was also the opportunity to bump the dependency as suggested by Wim in #79, in order to retrieve Longwave's upstream contribution.

I will address some of Andrew's and Christian's feedbacks

Done in dedicated commits.

So, Wim, we are back in "Needs review", with also an additional feedback from Andrew (@deviantintegral): https://git.drupalcode.org/project/drupal/-/merge_requests/13457#note_73... I believe you are the best to answer this specific subject.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I have rebased the branch and fix the conflicts.

There is a phpunit warning, but I saw it on other current MR targeting main and there is no specific fail to check: https://git.drupalcode.org/project/drupal/-/jobs/9759002

Andrew (@deviantintegral)'s note is still the remaining part to check: https://git.drupalcode.org/project/drupal/-/merge_requests/13457#note_73

The 3 commits since my review in #81 look like nice tidying. The upstream bugfix landed, and this now bumps the required upstream version instead.

I didn't re-test against Canvas like I did in #79.3, but I don't see how one of those 3 commits could cause any problems!

Thanks for making this happen! 🤩

Added comments to the MR.

Also the change record seems to be full of history and not instructions for developers and what this change means for them.

There is no change record yet. The linked change record is for Canvas, because it forward-ported this. 😅

Unlinked CR, added tag instead.

Hi Alex,

Thanks for your feedbacks. I am assigning the ticket to you because the last MR note is between us.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I will do the rebase, and it will be the opportunity to think again about Alex feedback.

Rebased.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Rebasing and answering Wim questions.

Change record: https://www.drupal.org/node/3591541

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Rebased.

There is a PHPUnit Functional Javascript error:

    Settings Tray Block Form (Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockForm)
     ✘ Blocks
    Failed asserting that a NULL is not empty.

Investigating

Running core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php locally is OK. So, it may not be related to the current change.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.