References must not be required to guess props' JSON schema

wim leers → credited mherchel → .

Wow, you even assigned the right issue queue component, amazing 🤩

I think the concerns would be reduced if ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed is done and core provided a handful of common definitions — such as for images. But even then, I think you're right:

1. Agreed that matching the resolved schema should also work. I already have the foundations for doing that in place: \Drupal\experience_builder\PropShape\PropShape::$resolvedSchema.
2. Slightly tricky: if the ordering of the properties is different, it should also match. For example: not src/alt/width/height, but width/height/alt/src. I assume you agree those should all be treated as equivalent?
3. The compatible subset example … is an interesting observation 😅 I hadn't thought of that one for sure! I want to think that through some more, but purely logically speaking, it's hard to refute.

The logic that needs to be improved is

      SdcPropJsonSchemaType::OBJECT => match (TRUE) {
        array_key_exists('$ref', $schema) => match ($schema['$ref']) {
          // @see \Drupal\image\Plugin\Field\FieldType\ImageItem
          // @see media_library_storage_prop_shape_alter()
          'json-schema-definitions://experience_builder.module/image' => new StorablePropShape(shape: $shape, fieldWidget: 'image_image', fieldTypeProp: new FieldTypeObjectPropsExpression('image', [

in \Drupal\experience_builder\JsonSchemaInterpreter\SdcPropJsonSchemaType::computeStorablePropShape().

(Not dealing with the resolved schema allowed this to remain simpler. I knew that we eventually needed to change this.)

We don't really know what is happening here (maybe it is because of an other problem we got: InvalidComponentException: [props.properties.image.properties.src.pattern] Invalid regex format ^(/|https?://)?.*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ ), but it is remembering me an issue from last summer: 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Fixed

Is #3454173 really the issue you were trying to link to? It seems unrelated?

This exception can indeed be triggered by changing XB's image component to use the resolved schema instead:

diff --git a/components/image/image.component.yml b/components/image/image.component.yml
index 80dc4cf22..12c65f742 100644
--- a/components/image/image.component.yml
+++ b/components/image/image.component.yml
@@ -8,9 +8,23 @@ props:
     # @todo Consider pulling all these props up a level, and move to components/simple/image: https://www.drupal.org/project/experience_builder/issues/3468944
     image:
       title: 'Image'
-      $ref: json-schema-definitions://experience_builder.module/image
-      # @todo `type: object` should not be necessary, it's because \Drupal\sdc\Component\ComponentValidator::getClassProps() does not yet support $ref
-      type: object
+      type:  object
+      required: ["src"]
+      properties:
+        src:
+          title: "Image URL"
+          type: string
+          format: uri-reference
+          pattern: "^(/|https?://)?.*\\.(png|gif|jpg|jpeg|webp)(\\?.*)?(#.*)?$"
+        alt:
+          title: "Alternative text"
+          type: string
+        width:
+          title: "Image width"
+          type: integer
+        height:
+          title": "Image height"
+          type: integer
       examples:
         - src: 600x400.png
           alt: 'Boring placeholder'

But … what is invalid about it?

If I look at https://json-schema.org/understanding-json-schema/reference/regular_expr..., I don't see any problem?

I debugged this and … found it's a bug in justinrainbow/json-schema that was fixed years ago 😬 See https://github.com/jsonrainbow/json-schema/issues/508. The problem is Drupal core is still on version 5.3 of the dependency which still has the broken function validateRegex(), version 6 and newer have the fixed version. Related core issue: 📌 Vet require-dev dependency justinrainbow/json-schema as a require dependency Active .

If it not already the case, let's not look at the parts of the URI string ("experience_builder", "image") to guess if it is an image or not. Those URI, like every URI, → are opaque. Let's use JSON schema reference as expected. It is a standard, solid, mechanism.

This is necessary to distinguish non-image URLs from image URLs. Without that additional restriction, we would end up also matching e.g. link and uri fields against image-shaped SDC props.

See:

•               $trailing_uri_regex_pattern = '\.(' . preg_replace('/ +/', '|', preg_quote($entity_constraints['FileExtension']['extensions'])) . ')(\?.*)?(#.*)?$';
  

  — \Drupal\experience_builder\ShapeMatcher\SdcPropToFieldTypePropMatcher::matchEntityPropsForScalar()

•             // When referencing an entity, enrich the EntityDataDefinition with
              // constraints that are imposed by the entity reference field, to
              // narrow the matching.
              // @todo Generalize this so it works for all entity reference field types that do not allow *any* entity of the target entity type to be selected
              if (is_a($field_definition->getItemDefinition()->getClass(), FileItem::class, TRUE)) {
                $field_item = $this->typedDataManager->createInstance("field_item:" . $field_definition->getType(), [
                  'name' => $field_definition->getName(),
                  'parent' => NULL,
                  'data_definition' => $field_definition->getItemDefinition(),
                ]);
                assert($field_item instanceof FileItem);
                $target->addConstraint('FileExtension', $field_item->getUploadValidators()['FileExtension']);
              }
  

• IOW: Drupal file (and any subtypes such as image) specify a FileExtension validation constraint. We use this to match only relevant field types. We eventually convert those (configured) validation constraints to equivalent JSON Schema representations. The best I could come up with is "pattern": "^(/|https?://)?.*\\.(png|gif|jpg|jpeg|webp)(\\?.*)?(#.*)?$". If you have a different idea, I'm all ears.

I think the concerns would be reduced if ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed is done and core provided a handful of common definitions — such as for images.

Indeed, i have added a extensive comment ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed with the hope we will move fast on this subject. However, it may be a complicated task if we want to do things right.

Slightly tricky: if the ordering of the properties is different, it should also match. For example: not src/alt/width/height, but width/height/alt/src. I assume you agree those should all be treated as equivalent?

Of course, order of properties in a JSON object doesn't matter as far as I know. You can have a look on UI Patterns 2's Canonicalizer service: we are ksorting the properties before working on them.

The compatible subset example … is an interesting observation 😅 I hadn't thought of that one for sure! I want to think that through some more, but purely logically speaking, it's hard to refute.

You can have a look on UI Patterns 2's CompatibilityChecker service. We have happily used this for one year for similar needs.

It would be great to resolve this, but I don't think this is something that should block a stable XB release. MRs are welcome 🤗

So, let's try to fix this in Core ( ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed ) as soon as possible, to avoid XB to be shipped with an incorrect management of JSON Schema references which will be harmful for both XB (which will not be able to handle prop shape it is already managing) and the SDC contrib space (which will need to decide between avoiding XB or being dependent of XB).

I am looking for contributors but I am afraid we will need to wait 11.3 now.