Adding the Image component results in a state considered invalid

Created on 24 January 2025, about 1 month ago

Overview

After adding the XB shipped Image component to the page, I'm getting following unrecoverable error state:

Drupal\experience_builder\Exception\ConstraintViolationException: Validation errors exist: Array.model.295d63ae-7d57-40d2-adaa-5d13913eb055.image.src: File '/sites/default/files/600x400.png' not found. Object(Drupal\node\Entity\Node).model.295d63ae-7d57-40d2-adaa-5d13913eb055.image: The property image is required in Drupal\experience_builder\Controller\ApiPreviewController->clientModelToInput() (line 144 of /var/www/html/drupal/modules/custom/experience_builder/src/Controller/ClientServerConversionTrait.php).

Proposed resolution

User interface changes

๐Ÿ› Bug report
Status

Active

Version

0.0

Component

Page builder

Created by

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • First commit to issue fork.
  • ๐Ÿ‡ฎ๐Ÿ‡ฑIsrael heyyo Jerusalem

    heyyo โ†’ changed the visibility of the branch 3501902-adding-the-image to hidden.

  • ๐Ÿ‡ฎ๐Ÿ‡ฑIsrael heyyo Jerusalem

    heyyo โ†’ changed the visibility of the branch 3501902-adding-the-image to hidden.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    This works fine on a fresh install hook_install needs to run to create the placeholder image and associated media entity.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    I had just re-installed before submitting this issue. Re-installed again and still seeing the same error message.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Can you put a breakpoint in the install hook and advise where it exits?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    How does the default image support work? Do extensions shipping components with default images have to provide an install hook to use an image by default?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Can you put a breakpoint in the install hook and advise where it exits?

    The file /sites/default/files/600x400.png exists if I visit it manually ๐Ÿค” The media item that was added looks correct.

    I'm a bit confused about the whole underlying change. I'm not sure that it's the right behavior that we're adding the default images to the media library. The image property is not supposed to be tightly coupled with media library and we may want to add support for image uploads in future too: ๐Ÿ› Redux support for ImageWidget: `[image] String value found, but an object is required` Postponed .

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    It is working fine for me with what the install hook provides.

    To narrow down what might be different it's probably worth checking admin/content/media to see if the media entity created via the install hook actually got created, and if so, is it accessing the image properly

    There are two steps in the process that can be checked.

    First Step

    First, the image in experience_builder/images/600x400.png is added as a managed file and copied to the root of public://

      $file = \Drupal::service(FileRepositoryInterface::class)->writeData(\file_get_contents(__DIR__ . '/images/600x400.png') ?: '', 'public://600x400.png');
    

    Questions:
    Is the file being copied?
    Where is it being copied to?

    Second Step

    A media entity is created referencing the managed file created in the prior step.

    if (MediaType::load('image')) {
        Media::create([
          'bundle' => 'image',
          'name' => 'Default placeholder image',
          'field_media_image' => [
            [
              'target_id' => $file->id(),
              'alt' => 'Placeholder image',
              'title' => 'Placeholder image',
            ],
          ],
        ])->save();
      }
    

    The "Default placeholder image" entity pictured below should exist and the preview should actually provide an image .

    Questions:
    Does this media entity exist?
    Is it previewing a valid image/path?

    If everything checks out above, please share path of public::// on the site that is failing on this.
    The sdc has the preview image path configured as - src: /sites/default/files/600x400.png which seems like it's making some assumptions regarding the stream path so I'd like to rule that out if there's nothing earlier in the chain failing.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Media item exists and preview fine in the media UI:

    File exists:

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    I was able to narrow done potential reason for this; I have the image multiple times in the /files directory because I've re-installed XB multiple times. This means that the file in the file entity has a suffix _4 and cannot be found with \Drupal\Core\Entity\EntityStorageInterface::loadByProperties.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Deleted the file, tried re-installing but now I have the file existing in the file entities but the media entity doesn't exist. ๐Ÿคทโ€โ™‚๏ธ

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    All of this is temporary code until we can delete ::findTargetForProps which is what ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active and its children are for.

    I think in the meantime using FileExists:Replace will smooth the issue Lauri experienced

  • Merge request !588Issue #3501902 Replace file if exists โ†’ (Closed) created by larowlan
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Opened an MR to replace the file if it exists, that should smooth things until we get a chance to work on ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Still running into the same problem from #13 that the media entity doesn't get created when I first install XB on a fresh install. I uninstalled XB and installed once more and now the image component is working. This allows me to UAT ๐Ÿ“Œ Media Library dialog styling Active but keeping this open because I bet that others will run into this problem.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    FWIW, I'm getting this error even when selecting existing images from the media library:

    Drupal\experience_builder\Exception\ConstraintViolationException: Validation errors exist: Array.model.b95fc0dd-f47f-4f1b-8d9f-b5cf59d8ef58.image.src: File '/sites/default/files/2025-01/Screen%201%20Teams%20Admin%20Section%20card.png' not found. Object(Drupal\experience_builder\Entity\Page).model.b95fc0dd-f47f-4f1b-8d9f-b5cf59d8ef58.experience_builder:image/image: The property image is required. in Drupal\experience_builder\Controller\ApiPreviewController->clientModelToInput() (line 144 of /var/www/html/drupal/modules/custom/experience_builder/src/Controller/ClientServerConversionTrait.php).
    
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    I'm very keen to keep going on ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active so we can kill off ::findTargetForProps which is where this issue is coming from - but it sounds like we need to do something else in the meantime as a band-aid

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #14++
    #19++

    I'd prefer to have @larowlan to focus hard on getting that stuff on track because we have to get it on track anyway.

    Let's not put a band-aid on a band-aid ๐Ÿ˜…๐Ÿ™

    I'd like to make it @larowlan's top priority in the next 2 weeks to keep pushing forward on ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active and all its child issues/blockers to solve this at the root.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“Œ Implement saving block settings forms Active introduced this, by the way.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States effulgentsia
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    re #20

    I'd like to make it @larowlan's top priority in the next 2 weeks to keep pushing forward on #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. and all its child issues/blockers to solve this at the root.

    That issue seems like the right priority, When that work is done, would it still be necessary for every prop-example image to become a media entity? It's fine with our dev-collection of components, but as more custom components start happening I could see that causing issues, such as someone deleting that placeholder image would make the component un-addable. It might be helpful to have a placeholder image generating controller, similar to placehold.it, where the URL determines width, height, color, content, then the required src for preview won't be at risk of deletion and repos don't need to have as many image files just to placehold stuff.

    Again, the data model work seems like the priority to me, but if the above seems worthwhile this issue could be the home for it as it would make the band-aid unnecessary.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    I hit this on a fresh install and can confirm the file is created, but the media entity is not, which would indicate the media type doesn't exist when experience_builder.install is running - likely because we're now installing modules in bulk ๐Ÿ“Œ Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active (I'm running 11.x HEAD)

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    I hit this on a reinstall in the same way as #12. As per #23 I don't think we can rely on files existing at specific paths; perhaps in the images case at least we should be more lenient, if we can't find the underlying file then should we really be throwing an error message?

    @larowlan for now I think we should only consider compatibility with 11.1, we have a few months and ๐Ÿ› PHPUnit Next Major tests failing Active to solve compatibility with 11.2.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I wonder if everyone who is hitting this is testing using 11.x HEAD? ๐Ÿค”

    #25: โ€” if we don't, then the SDC subsystem itself will. At least generally speaking, I suspect that in this case, you're implying "just generate <img src="http://example.com/cat.jpg" alt="a cat"> and let the browser then fall back to showing the text alternative?

    #23: thanks, @bnjmnm โ€” much appreciated! And is a very intriguing idea, with a lot of potential indeed ๐Ÿค”

    Related: ๐Ÿ“Œ Update XB's `image` SDC to comply with best practices, and document those best practices Needs review .

    What you wrote actually ties in nicely with my alternative idea, in case #20 doesn't work out:

    • we need an SDC to be able to specify a default, and for generic SDCs, what you describe makes perfect sense
    • but we also need the ability for A) a site-specific SDC with an SDC-provided default image in its single directory, B) for a site to assign a site builder-specified default image when making a generic SDC available as component. (And this implies C) the ability to override an

      Both A and B can be solved by the same infrastructure: if the XB Component config entity could contain (and export/import like any config) the default image, then it'd be guaranteed to work. In case A ("SDC-provided"), the initial assignment happens automatically (upon creating the Component config entity), in case B ("site builder-provided") it'd happen manually (requires editing a created Component config entity).
      After that initial step, the config entity would carry a dependency on the MediaType config entity, the Media content entity UUID, and the base64-encoded image data. Upon saving this Component config entity, that'd result in the Media entity with that UUID being recreated if it doesn't already exist.

      Which is exactly what we already PoC'd many months ago, and which last surfaced in September:

      FYI: we'll need something like #6 too for allowing, among others:
      an SDC will need to be able to define a default image, per ๐Ÿ“Œ [SPIKE] Comprehensive plan for integrating with SDC Active
      that image will have be a file inside the SDC's directory
      โ€ฆ which means that it won't exist as a File or a Media entity
      โ€ฆ which means its must be auto-created.

      This is exactly why @f.mazeikis in his work on ๐Ÿ“Œ Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed exempted any SDC whose StorablePropShape ended up using an EntityReferenceItem (entity reference field type) or subclass.

      The infrastructure I PoC'd would allow that to start working, and would allow a default image to be specified by the SDC and correctly tracked in the Component config entity, when combined with @f.mazeikis' prior work on auto-encoding content entity dependencies into the config entity and auto-restoring upon config import.

      โ€” yours truly at #3473336-8: The UI only handles resolved props values, hence complex widgets do not show the current value in props form (e.g. image with Media Library Widget) โ†’

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    I don't really understand how we've ended up forming an assumption that the default images need to be added to the media library? That doesn't really seem like the expected user experience. Let's say, I'm using Acquia DAM for managing our media assets, and I download a community component which comes with a default image. I certainly wouldn't expect the default image to appear in the DAM. ๐Ÿค”

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    Yeah I don't think an SDC-provided default should end up in the media library; it's not part of the site's media and you would never want to really select it nor should you have the ability to change it directly, it's just a fallback.

    But a site-builder specified default probably *should* end up in the media library as described in #26, if it's configured by editing the Component config entity?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    @lauriii FYI we previously discussed SDCs providing default images in the context of ๐Ÿ“Œ [SPIKE] Comprehensive plan for integrating with SDC Active months ago.

    Note that an SDC currently cannot provide a valid default image, because an image URI must be either:

    • an actually working http:// URI
    • an actually working https:// URI
    • an actually working root-relative http:// URI

    โ€ฆ plus it must NOT be some random image hosting service: it MUST be powered by the current site's domain or a DAM service it integrates with.

    IOW: the sole example of this in the XB codebase itself is in need of an overhaul:

        image:
          title: 'Image'
          $ref: json-schema-definitions://experience_builder.module/image
          type: object
          examples:
            - src: /sites/default/files/600x400.png
              alt: 'Boring placeholder'
              width: 600
              height: 400
    

    ๐Ÿ‘† that is a default value not provided by the SDC (hence ๐Ÿ“Œ [SPIKE] Comprehensive plan for integrating with SDC Active ), nor is it absolute, nor is it guaranteed to be root-relative.

    This default value only works thanks to experience_builder_install(). Except when it doesn't. Which is what is being reported here! ๐Ÿ˜…

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Discussed this with @longwave and @effulgentsia. We didn't get to a resolution.

    Here's the scenario that forces us to import/create the SDC/"code component"-specified default image as a File entity (if using the image field type for the "image" prop shape), a Media entity (if the media library is installed) or a DAM (if such a module is installed):

    • I have a "Author" SDC, with 2 required props: profile_pic + bio.
    • To ensure instantaneous previews of XB components, XB imposes a rule on SDCs to be XB-eligible: all required props MUST provide >=1 example; the first example will be used as the default.
    • When I place a component in XB, powered by the"Author" SDC, I must be able to save it immediately after placing it. This WILL result in an image file/image media/DAM reference being stored.
    • Therefore it is necessary for the SDC's default image to be created as the "kind of thing" any user-provided image would be stored (file/media/DAM).
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Sounds like @larowlan also has been thinking about this:

    Once that is in, we will need default values to cover both. e.g. If we've got an image reference, the default value of the hydrated props might be a URL, alt tag and width and height, but the stored value would be the media or file ID.

    โ€” @larowlan, January 14, 2025, #3493943-7: [PP-1] Split default values into resolved and raw โ†’

    I'm not sure how he imagines making the SDC-provided default image into a resolveable URL though.

    But this is exactly why I wrote #20: it all is connected. And the more small pieces of ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active are solved, the easier it becomes to reason about this. ( ๐Ÿ“Œ Move clientside assumptions about prop form data shape into a series of prop specific transforms Active just landed!)

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Related: โœจ Feature request: Ability to specify minimum image resolution for an image upload Active โ€” many image-consuming SDCs would likely not provide their own default images, but would rather instead have generic images generated that show the maximum resolution?

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    The root of the issue here is that we map the image property to an entity reference field of type media, but set a default value that is a hard-coded URL - suggesting that in fact this could/should be a URI field. But we want to use the media library to edit the values. So we're in a state where the component has a default value that doesn't match the data shape (field type) we've specified. We work around this with ::findTargetForProps but this is a temporary workaround (And commented as such).

    In HEAD we try to reconcile this by at least ensuring that a media entity exists with the given default image URL - but as discussed in this issue, that isn't working in all cases.

    We're trying to resolve this on multiple fronts.

    - ๐Ÿ“Œ Support server side massage and validation of component prop form values Active
    - ๐Ÿ“Œ Split model values into resolved and raw Active

    However those are both very tricky. I'll have a think about what we can do in the meantime

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Came here to post a reference to ๐Ÿ“Œ Update XB's `image` SDC to comply with best practices, and document those best practices Needs review and then realised Wim already had ๐Ÿ˜€

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    I think we also have a disconnect in our components API with what we store in the config entity

    Here's what's in the components api for the default value for the image component - this is taken from the example in the component.yml file.

    But here's whats in the config entity:

    uuid: 0202d62a-193a-4129-ae3a-b7c8f742f743
    langcode: en
    status: true
    dependencies:
      module:
        - media_library
    label: Image
    id: sdc.experience_builder.image
    source: sdc
    category: Other
    settings:
      plugin_id: 'experience_builder:image'
      prop_field_definitions:
        image:
          field_type: entity_reference
          field_storage_settings:
            target_type: media
          field_instance_settings:
            handler: 'default:media'
            handler_settings:
              target_bundles:
                image: image
          field_widget: media_library_widget
          default_value: {  } # ๐Ÿ‘ˆ๏ธ๐Ÿ‘ˆ๏ธ๐Ÿ‘ˆ๏ธ
          expression: 'โ„น๏ธŽentity_referenceโŸ{srcโ†entityโœโœentity:media:imageโfield_media_imageโžโŸentityโœโœentity:fileโuriโžโŸurl,altโ†entityโœโœentity:media:imageโfield_media_imageโžโŸalt,widthโ†entityโœโœentity:media:imageโfield_media_imageโžโŸwidth,heightโ†entityโœโœentity:media:imageโfield_media_imageโžโŸheight}'
    
    

    And this is because \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo special cases the a prop with $ref 'json-schema-definitions://experience_builder.module/image'

    And I also think that reveals a possible solution for us here as follows:

    • We already have \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin which is called from updateConfigEntity and createConfigEntity.
    • Currently in that code we're bailing out if the prop shape maps to an entity-reference field BUT perhaps we shouldn't - perhaps we should add support in there for looking at the default image referenced in the $ref and importing it into a media entity AND then setting a default value that includes the target ID
    • Remove the special casing of 'json-schema-definitions://experience_builder.module/image' in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo
    • This also allows us to manage config dependencies, as we can make a reference from the component config entity to the media entity which will mean \Drupal\Core\Config\ConfigEvents::IMPORT_MISSING_CONTENT gets fired on a config import
    • We then make use of an additional key in the source plugin settings for SDC/Code components to store a base64 encoded version of any default images for deployment sake and add an event listener for \Drupal\Core\Config\ConfigEvents::IMPORT_MISSING_CONTENT to take care of creating the missing media entity in a deployment based on these settings
    • Then our default value would include target_id and this would get us closer to removing findTargetForProps too

    If this seems like a reasonable idea I can do a spike on it.

    Most of this seems to have todo's in the code so it might be time to pay down these debts

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Is there really not a solution that wouldn't involve importing images to media library? As I already pointed out in #27, I don't think adding default images from components to the media library is part of the ideal experience.

    #29 is pointing out that the limitation is that the image URI must be either:

    • an actually working http:// URI
    • an actually working https:// URI
    • an actually working root-relative http:// URI

    Could we not then allow the default value to remain either:

    • an actually working http:// URI
    • an actually working https:// URI
    • an actually working component-relative http:// URI

    Then we would transform the component-relative URI to a root-relative URI before it actually gets rendered.

    โ€ฆ plus it must NOT be some random image hosting service: it MUST be powered by the current site's domain or a DAM service it integrates with.

    Where does this requirement come from? Why can we not allow images coming from image hosting services?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    But a site-builder specified default probably *should* end up in the media library as described in #26, if it's configured by editing the Component config entity?

    I think that's right, at least in the site where the component was built. I would imagine that โœจ Defining props for code components Active would be using media / image upload for uploading default images and therefore they would end up in the media library. I think in the case of a config deployment, it would make sense to create the media or file entity.

    However, in future we may allow exports and imports of a component. In this scenario, it might not make sense anymore to add the image to the media library because the image may not have anything to do with the site where it gets imported.

    It looks like https://wordpress.org/patterns/ is using remote URLs for the images so that the component can be simply copy pasted.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    In #27 @lauriii states that the default image should not end up in the site's media library. It's not even guaranteed that media library will be used for media storage in the Acquia DAM case.

    Instead of trying to store the default image as a media entity I'm wondering if it would be worthwhile having a custom image controller that renders default images for any component. This image can still be stored as base64 in the config entity but it otherwise doesn't need to exist on disk or as a content entity. If the prop value is empty, then we output the URL that points to this custom controller.

    Optional images are still awkward though. If optional images have a default example, what do you use to tell the difference between the user wanting no image at all and the default image to be shown?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Yep, agreed with #33.

    I'd prefer to have @larowlan continue on ๐Ÿ“Œ Support server side massage and validation of component prop form values Active (reviewed in-depth today) and other ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active child issues, because it'll make the correct solution here clearer.

    The code @larowlan is quoting in #35 is being touched upon by ๐Ÿ“Œ Maintain a per-component set of prop expressions/sources Active โ€” which is yet another child issue of #3467954.

    Will follow through in-depth here tomorrow, with hopefully #3467954 committed and #3499550 close. ๐Ÿคž

    P.S.: Quoting \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo() (which was moved multiple times but dates back to the very early XB days):

          // @todo Add support for default images in SDCs: /components/image/image.component.yml. (And entity references in general.)
          // @see \Drupal\experience_builder\Entity\Component::getDefaultsForComponentPlugin
    
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Discussed this at length with @longwave, @wim leers, and @effulgentsia. We essentially identified four scenarios that we need to support and whether we'd expect the default images to appear in the media library:

    In order to accomplish this, we'll add support for scenario 1. by using the JSON Schema default annotation.. This would essentially allow specifying a default image which would not show up in the form, but would be loaded as the default value in the field.

    Scenario 4. is a future consideration because we don't have the option to export code components outside of regular config currently. We can fallback to the behavior from scenario 3. until then.

    In future, we may consider adding support for JSON Schema examples values for images. This would require being able to display a non-media library in the field widget. This depends on finishing designs for the media widget.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    In ๐Ÿ“Œ Support server side massage and validation of component prop form values Active I've managed to do away with ::findTargetForProps once the user selects something from the media library

    However I've had to retain it for the use-case when the user adds a new image component because the default value is invalid

    This reinforces my comment above

    The root of the issue here is that we map the image property to an entity reference field of type media, but set a default value that is a hard-coded URL - suggesting that in fact this could/should be a URI field. But we want to use the media library to edit the values. So we're in a state where the component has a default value that doesn't match the data shape (field type) we've specified

    I think until we have a way to reconcile those two we're at an impasse. Either we're storing a media reference OR we're storing a URL.

    And perhaps that points to us needing a custom field type - a compound field that is an ER with additional columns/properties for src/alt/width/height.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #40:

    1. The use of default vs examples to indicate that the default should be used for generating the default_markup without putting it in the Media Library is nice, but it's not a full solution.

      Note that SDC does itself not support/use default either โ€” see #3462705-19: [META] Missing features in SDC needed for XB โ†’ and #3462705-26: [META] Missing features in SDC needed for XB โ†’ .

      But we in XB can support it anyway, by changing GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo() โ€” easy enough!

      Except โ€ฆ what @larowlan wrote in #41 means we likely can't. Because before we can "just use the SDC-specified default), we MUST apply a transformation, because even in the best case scenario where the SDC includes an image for which a publicly accessible URL can be generated, it still must be generated. So perhaps specifying it as default: https://example.com/path/to/sdc/cat.jpg would be the way to specify in the *.component.yml that the cat.jpg image file included in the single directory should be loaded.

      (And no, a root-relative URL doesn't work either, because Drupal could be installed in a subdirectory, and there's multiple possible file system locations for modules/themes to be installed.)

    #41: I think @lauriii's #40 didn't explain in sufficient detail how he imagined it'd work. See the first sentence of my response to #40.1. To expand on that: @lauriii wrote

    This would essentially allow specifying a default image which would not show up in the form, but would be loaded as the default value in the field.

    But I think he meant "the SDC default would not show up in the Media Library widget" and "would be loaded as the default value in the default_markup" aka component preview.

    IOW: regardless of whether the image is optional or required, the image specified as default (and with the transformations needed as mentioned above) would NOT be stored in the component tree and hence would also never need to live in the Media Library.

    That means

    Either we're storing a media reference OR we're storing a URL.

    does not need to be true โ€ฆ anymore! ๐Ÿ˜…

    Doing that does create a new challenge: we may end up storing an instance of the sdc.experience_builder.image Component in the tree without any corresponding inputs ๐Ÿซฃ ValidComponentTreeConstraintValidator requires each component instance in tree to have a corresponding entry in inputs. But we can relax that a bit:

          // Get the stored explicit input. Only add a violation error if the
          // Component in its current definition requires explicit input. (Silently
          // ignore stored inputs that are no longer required per Postel's law.)
          // @see https://en.wikipedia.org/wiki/Robustness_principle
          try {
            $inputs = $value->get('inputs');
            assert($inputs instanceof ComponentInputs);
            $stored_explicit_input = $inputs->getValues($component_instance_uuid);
          }
          catch (MissingComponentInputsException $e) {
            if ($component_source->requiresExplicitInput()) {
              $this->context->buildViolation('The required properties are missing.')
                ->atPath(sprintf('inputs.%s', $e->componentInstanceUuid))
                ->addViolation();
              continue;
            }
            else {
              // Fall back to empty input.
              $stored_explicit_input = [];
            }
          }
    

    could be modified to also allow the inputs for the component instance to be absent if for every of the required SDC props, a default is specified.

    This may require adjusting both the semantics and implementations of GeneratedFieldExplicitInputUxComponentSourceBase::requiresExplicitInput(), or might require an additional method.

    I do not like any of this though; it makes the entire XB data model harder to reason about.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Thank you @wim leers for expanding #40! I tried to make a quick comment at the eleventh hour to avoid blocking folks.

    could be modified to also allow the inputs for the component instance to be absent if for every of the required SDC props, a default is specified.

    FYI, this would be probably temporary because once we have support for image examples, we'd want to enforce in this scenario that required images have been uploaded. This is actually nice because it allows showing a default image while still requiring the content creator to select an image. Pointing this out because I'm trying to avoid us taking a difficult path for a temporary solution.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    [โ€ฆ] temporary because [โ€ฆ]
    This is actually nice because it allows showing a default image while still requiring the content creator to select an image.

    I was thinking about exactly this and hoping that you'd not say this.

    Because it introduces a major new consequence that everything in XB was carefully designed to avoid.

    • Until now: every component that you can drag onto the canvas to instantiate is immediately visible and saveable, because every required input must have an example/default value. (This is also why we do not consider available SDCS eligible for XB if they have required props without >=1 examples!)
    • What you're saying: every component that you can drag onto the canvas to instantiate is immediately visible and saveable. ๐Ÿšจ Do we force the author to enter a value before being able to move on to another component? Or do we allow moving on, but provide an "error" affordance on the preview, so the person can find the component instance in an error state? Or do we omit the component instance altogether when saving?
      (We faced a similar challenge with https://drupal.org/project/quickedit btw.)
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    You're right, we've designed the UX to avoid exactly that ๐Ÿคฆโ€โ™‚๏ธ We should avoid reversing that because it will have major consequences on the UX.

    Let's simplify this and work for now with the assumption that default cannot be used for required images, that way the field can be left empty in which case it falls back to the default image. I think that's an acceptable state until we've figured out support fo image examples, which would allow marking images as required.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Diving into the deep end of this. ๐Ÿคฟ

    Step 1: where does the current default value come from?

    The default value that appears on the client side for the sdc.experience_builder.image Component is:

    src: /sites/default/files/600x400.png
    alt: Boring placeholder
    width: 600
    height: 400
    

    It is provided by GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo(), which does this:

          $prop_info = ($component_plugin->metadata->schema['properties'] ?? [])[$prop_name];
          // Defaults are guaranteed to exist for required props, may exist for
          // optional props. When an optional prop has no default value, the value
          // stored as the default in the Component config entity is NULL.
          // @see \Drupal\experience_builder\ComponentMetadataRequirementsChecker
          $is_image = isset($prop_info['$ref']) && $prop_info['$ref'] === 'json-schema-definitions://experience_builder.module/image';
          // @todo Add support for default images in SDCs: /components/image/image.component.yml. (And entity references in general.)
          // @see \Drupal\experience_builder\Entity\Component::getDefaultsForComponentPlugin
          $is_datetime = isset($prop_info['format']) && $prop_info['format'] === 'date-time';
          // @todo DateTimeItem stores information in a format that clashes with JSON schema's, and it has no automatic conversion. Figure out a better solution for both this and \Drupal\experience_builder\PropExpressions\StructuredData\Evaluator::evaluate().
          $default_value = NULL;
          if (($is_image || $is_datetime)) {
            if (isset($prop_info['examples']) && is_array($prop_info['examples']) && !empty($prop_info['examples'])) {
              $default_value = $prop_info['examples'][0];
            }
          }
          else {
            $default_value = $this->getDefaultStaticPropSource($prop_name)
              ->evaluate(NULL);
          }
    โ€ฆ
          if ($default_value !== NULL) {
            $field_data_partial[$prop_name]['default_values'] = $default_value;
            $default_props_for_default_markup[$prop_name] = $default_value;
          }
    

    The special casing of type: string, format: date-time is irrelevant here. This issue is solely about $ref: json-schema-definitions://experience_builder.module/image.

    • ๐Ÿ’ก So it is just literally what's in image.component.yml.
    • ๐Ÿค” But why does this even need to be special-cased? Why can't this rely on $default_value = $this->getDefaultStaticPropSource($prop_name)->evaluate(NULL); like everything else? (Except type: string, format: date-time?

    Step 2: why can't we evaluate the Component's StaticPropSource that uses the stored default value?

    The relevant logic was last touched in a significant way in ๐Ÿ“Œ Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed . It was moved to a different class a few times since (first due to adding support for blocks, then for "code components"). It's now at GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin() and contains:

          $skip_prop = FALSE;
          $storable_prop_shape = $prop_shape->getStorage();
          if (is_null($storable_prop_shape)) {
            continue;
          }
    
          if ($storable_prop_shape->fieldTypeProp instanceof FieldTypeObjectPropsExpression) {
            // @todo Add support for default images: /components/image/image.component.yml.
            if ($storable_prop_shape->fieldTypeProp->fieldType === 'entity_reference') {
              $skip_prop = TRUE;
            }
            else {
              foreach ($storable_prop_shape->fieldTypeProp->objectPropsToFieldTypeProps as $field_type_prop) {
                if ($field_type_prop instanceof ReferenceFieldTypePropExpression) {
                  $skip_prop = TRUE;
                }
              }
            }
          }
    

    That

    // @todo Add support for default images: /components/image/image.component.yml.
    

    is why there's no default value!

    And the reason for that is that there's:

    • no File entity for the SDC's example value (when the Media Library module installed and hence using the image field type)
    • nor a Media entity for the SDC's example value (when the Media Library module installed and hence using an entity reference field pointing to image media, see media_library_storage_prop_shape_alter(), but note that ever since ๐Ÿ“Œ Make Media Library a dependency Fixed XB is requiring the Media Library module to be installed for simplicity sake)

    โ€ฆ to use as the default value!

    โš ๏ธ Note that it's impossible for an SDC to specify a working image URL, except to external services. Reliance on an external service is unacceptable.

    • ๐Ÿ’ก @larowlan introduced the current sole hardcoded default image (+ hook_install() to ensure it exists) in https://git.drupalcode.org/project/experience_builder/-/commit/0603b8636...
    • ๐Ÿค” I know he asked me whether I was fine with it for the sake of being able to land that MR (I wrote I was), but I'm not able to remember nor find the exact reason for it ๐Ÿ™ˆ My bad.

    Next step: allow the default used in the preview to be different from the default used in ::getDefaultStaticPropSource().

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Screenshot of where I left things when I passed it on to @longwave:

    • form works ๐Ÿ‘
    • preview does not yet, for the reasons outlined in my previous comment, visualized here.
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Still needs refining, but it's now partially working, after pairing with @longwave on it: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/d92...

    But as you can see, the Media Library does not yet open. To be continued tomorrow.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Keeping issue summary relevant. The current MR should do only one thing, but there's a second part to thisโ€ฆ

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    @Wim you forgot to add the new FallbackPropSource file so I rewrote it from memory :D

    Also fixed up the #disabled code as it wasn't quite right. Time to actually open an MR here!

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    longwave โ†’ changed the visibility of the branch 3501902-replace to hidden.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    Given the MR fixes the issue described in the title, can we push solving the problem of handling/remapping default image URLs to a followup? The image component is no longer completely broken, just it relies on the hack in experience_builder_install() to get the default image file to the right place.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #53 ๐Ÿ˜ฑ๐Ÿ™ˆ๐Ÿคฃ gitnoob And you got it character-by-character identical except for one line :O

    #55: Not opposed to that, but it does mean that the literal originally reported issue does not get fixed. How about we just do 2 MRs for this single issue? And just merge this first MR โ€ฆ first? Avoids issue queue overhead.

    The issue title has been lacking precision since the start, which adds to the confusion. I'll think of a better title with a fresh head in the morning.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I was thinking about this some more last night.

    When and why do we need this FallbackPropSource? We need it for:
    preview purposes, never storage purposes
    never for booleans, integers, or numbers
    never for any string format other than one of the uri + uri-reference + iri + iri-reference ones: https://json-schema.org/understanding-json-schema/reference/type#format
    โ€ฆ and even then, only when they must be resolvable/fetchable, to make certain tags work: <img>, <video> โ€ฆ

    So AFAICT these special needs do not apply to anything else. The most common "resolvable URL" case is links (<a href="<SDC PROP HERE">link text</a>), but there it doesn't apply either, because the URL does not have a visible impact: a dummy URL would do just fine for preview purposes!

    Although, come to think of it: a "call to action" component would want to have a required link, without wanting that link to ever be saved? So, perhaps there is a difference in preview impact (image/video URLs vs link URLs), but the saving impact is the same: nobody wants links pointing to https://example.com/campaign, right? :D

    Given all that, I think that UrlPreviewPropSource would be a better name, that is much narrower and hence better captures both the need and when it is appropriate.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I believe I see a solution for

    1. agree upon the syntax for letting an SDC specify a default image, knowing it CANNOT be a resolvable image URL, and perform the necessary transformation to a resolvable image URL

    in ~100 LoC (nutshell: require example values to start with https://example.com, add new method to ComponentSource to rewrite such example values to resolvable URLs):

    The only thing that's still missing: updating \Drupal\experience_builder\ComponentMetadataRequirementsChecker::check() to require examples values for type: string, format: uri|iri|uri-reference|iri-reference to start with https://example.com.

    See the second branch:

    1. making it work in ~100 LoC: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/3dc...
    2. getting rid of the auto-created Media entity for the 600x400.png sample image: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/bf1...

    P.S.: this also paves the path for examples for images to become optional if min/max dimensions are specified, then we could auto-generate a relevant sample image. See โœจ Feature request: Ability to specify minimum image resolution for an image upload Active .

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    #57 makes sense! Great insight to limit this to URLs - but so that it works for links as well as images - and the https://example.com/ prefix is also a fantastic idea.

    I think we could just work on this in a single MR now? I don't see the point in keeping two separate ones if we are going to solve both problems here.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    WFM! :D Go ahead and merge as far as I'm concerned :) But โ€ฆ I think it'd be better/simpler to first get tests to pass on the current MR? ๐Ÿ˜‡ That keeps the focus on the most important thing?

    Just walked @lauriii through what I did in #58 (at the end of a meeting about something else). He VERY much does not like the https://example.com prefix ๐Ÿ˜… He agrees with the overall approach, but just explicitly doesn't like that prefix.
    I respectfully disagree, but given @lauriii has a much stronger front-end development past than I do, I defer to him ๐Ÿ˜Š Done: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/457...

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    Started going too far with this and almost tried to solve ๐Ÿ“Œ Split model values into resolved and raw Active as well, but that wasn't as easy as I first thought (I should have learned by now) so I rolled back those changes and concentrated on only the image problem here.

    Will merge in the second branch now.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    Reworked as discussed with @wim.leers, @effulgentsia, @f.mazeikis - the leading slash is not even required so any example file is relative to the SDC itself. If the file exists then the full path to it is given (e.g. for an image src), if the file doesn't exist then it is treated as relative to the root of the site. Added test cases for both these, with and without leading slashes.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #62 is actually what @lauriii expressed on the meeting I had with him as his ideal.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    @wim leers asked me to test this with more permutations to get a sense where we are:

    I'm getting this error when I try to place "Container" element from demo design system. It has an optional image without an example.
    AssertionError: assert(\is_array($this->value)) in assert() (line 78 of /var/www/html/web/modules/contrib/experience_builder/src/PropSource/FallbackPropSource.php).

    Placing the "Hero" component which has an optional image with an example value works but the media library doesn't open when I click "Add media". If I change other props than the image, the default image is lost.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    @lauriii Re #65 the problem with Container is that it specifies an image without an example:

        # Don't put examples as this is used with and without images.
        image:
          $ref: json-schema-definitions://experience_builder.module/image
          type: object
          title: Image
    

    but Experience Builder's image schema says the src property is required:

        "image": {
          "title": "image",
          "type":  "object",
          "required": ["src"],
          "properties": {
            "src": {
              "title": "Image URL",
              "$ref": "json-schema-definitions://experience_builder.module/image-uri"
            },
    

    This causes a validation error because when you first add the component the image doesn't have a src. If an example was specified (as in the XB image component) then we use that as the fallback, but there is nothing to fall back to here either.

    Other than this, the tests should be passing now and all of Wim's feedback should be implemented.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    This looks good to me, I manually tested it and confirmed it is working as expected.
    I also tried to remove ::findTargetForProps and confirmed we cannot do that without ๐Ÿ“Œ Split model values into resolved and raw Active

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #66: That problem with the demo container SDC missing an example src in its json-schema-definitions://experience_builder.module/image points to a bug we haven't really seen before, because the only type: object XB supports is that image shape.
    The problem: \Drupal\experience_builder\ComponentMetadataRequirementsChecker::check() only checks for the presence of an examples[0], but it does not verify that it actually complies with the JSON Schema!

    ๐Ÿ‘‰ New issue created, with detailed plan: ๐Ÿ“Œ ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active .

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    wim leers โ†’ changed the visibility of the branch 3501902-sdc-shipped-default-image-syntax to hidden.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Looks great!

    All I did was:

    1. adding docs: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
    2. moving it to a new helper method, to have 2 clearer methods with clear responsibilities: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...

    Will merge after green CI ๐Ÿ‘

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Follow-up to tighten things, after this and a related issue landed: ๐Ÿ“Œ Clarify default 'resolved' vs 'source' value logic in GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo() Active .

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Looks like the bug which I reported in #65 where the default image is lost when changing props is still happening: ๐Ÿ› Default image is lost after changing props Active .

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #75: I explained in #68 that it's out of scope here, and opened ๐Ÿ“Œ ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active for it.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia nagwani
Production build 0.71.5 2024