Click image with ML enabled should open that image in props form

Created on 10 September 2024, 3 months ago

Overview

Proposed resolution

User interface changes

πŸ“Œ Task
Status

Active

Component

Page builder

Created by

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

Merge Requests

Comments & Activities

  • Issue created by @bnjmnm
  • Merge request !290Resolve #3473336 "Click image get media" β†’ (Open) created by bnjmnm
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    Theres some code formatting needed but it works & the e2e test expanded to include this functionality

  • Status changed to Needs review 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
  • Assigned to wim leers
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Status changed to Needs work 3 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Root cause analysis

    For the steps to reproduce in this issue, the small commit I pushed shows that this is a broader problem:

    LogicException: Unexpected static prop value: {"src":"\/sites\/default\/files\/2024-09\/before_1.png","alt":"asdf","width":284,"height":334} should be {"target_id":null} β€” properties are missing in Drupal\experience_builder\PropSource\StaticPropSource::Drupal\experience_builder\PropSource\{closure}() (line 218 of modules/contrib/experience_builder/src/PropSource/StaticPropSource.php).
    

    … and if you'd manually fix that, you'd now get:

    LogicException: Unexpected static prop value: {"src":"\/sites\/default\/files\/2024-09\/before_1.png","alt":"asdf","width":284,"height":334,"target_id":null} contains garbage `src`, `alt`, `width`, `height` properties in Drupal\experience_builder\PropSource\StaticPropSource::Drupal\experience_builder\PropSource\{closure}() (line 223 of modules/contrib/experience_builder/src/PropSource/StaticPropSource.php).
    

    The entity_reference field type does not contain src, alt, width nor height as a field property.

    It only contains a target_id property (and a computed entity property).

    Possible solutions

    1. For every interaction with a field widget, compute the updated StaticPropSource representation. Given the need for real-time updates of the preview (without relying on communication with the server whenever possible), it's not viable to constantly be submitting the field widget to retrieve the actual StaticPropSource representation, only to pass that back from the client to the server again to update the preview.
    2. The server side knows the prop expression, so … it should be able to unevaluate/reverse the evaluated/resolved props values that the client does know (and needs for real-time preview updates) using that prop expression.

    That second part is something I have known for a while we'll need to do/resolve. But everything has been "good enough" so far. This is the first issue where that explicitly is being surfaced, so I used the opportunity to dig into this.

    Why dig into it now? Because while the fix that @bnjmnm wrote is a fine work-around, it essentially hardcodes the "unevaluating/reversing" of evaluated/resolved props values using the expression for one particular hard-coded expression: the one defined in media_library_storage_prop_shape_alter().

    Proposed next steps

    1. Introduce the outline of the second proposed solution.
    2. Verify it works for the use case reported here: tests should be made to pass again, without @bnjmnm's hardcoded addition to \Drupal\experience_builder\PropSource\StaticPropSource::formTemporaryRemoveThisExclamationExclamationExclamation().
    3. Defer a full solution (and test coverage) to a follow-up issue.
  • Issue was unassigned.
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    RE next steps in #6:

    1. The outline of the proposed resolution has been introduced: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
    2. Ben's hardcoded addition was removed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... … but then the general solution was narrowed to only that particular case because it's unknown whether it'll work for everything: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

    Next: create follow-up issue, link to it, and await review.

    Just talked to @tedbow, @longwave and @effulgentsia about this problem space in a meeting, so at least now they're aware that this is something we'll need to figure out πŸ‘ (And it likely will require the data model on the client side to become more complex.)

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    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.

  • First commit to issue fork.
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    @tedbow indicated this as a possible issue to work on for me, so I started with a rebase, looking further into the remaining work next

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Is this still needed in light of discussion in https://docs.google.com/document/d/1sMpbWxnOZM-yq4IbrabUBh7-RGYj_2-aYkBR... and πŸ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active where it looks like we might keep the stored and evaluated state in the FE state?

Production build 0.71.5 2024