- Issue 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 3:33pm 10 September 2024 - Assigned to wim leers
- Status changed to Needs work
3 months ago 12:06pm 11 September 2024 - π§πͺ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 containsrc
,alt
,width
norheight
as a field property.It only contains a
target_id
property (and a computedentity
property).Possible solutions
- 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 actualStaticPropSource
representation, only to pass that back from the client to the server again to update the preview. - 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
- Introduce the outline of the second proposed solution.
- 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()
. - Defer a full solution (and test coverage) to a follow-up issue.
- For every interaction with a field widget, compute the updated
- Issue was unassigned.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
RE next steps in #6:
- The outline of the proposed resolution has been introduced: https://git.drupalcode.org/project/experience_builder/-/merge_requests/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 aFile
or aMedia
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?