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.