Unable to save an optional image with its default value (because it does not correspond to a Media entity): allow saving DefaultRelativeUrlPropSource

🤯 How did you get to that point?

That's coming from \Drupal\experience_builder\PropExpressions\StructuredData\Evaluator::doEvaluate() and means we're trying to evaluate a stored static prop without passing in the field data. That should not be possible for StaticPropSource (only for DynamicPropSources, but we don't use those yet anywhere in the current UI, that's for #3455629: [PP-1] [META] 7. Content Templates — aka "default layouts" — affects the tree+props data model).

Thanks!

I'm seeing this with latest XB+SDDS.

Added 4 columns with cards in each slot and tried to create a section from that and it gives:

Failed to save section

Error 500: No data provided to evaluate expression ℹ︎␜entity:media:image␝field_media_image␞␟alt

but the alt text exists on each image.

— @kristen pol at #3503547-8: Display validation errors in dialogs that create sections

Okay, this does make sense when the image is required, i.e. when using XB's own components/image/image.component.yml SDC.

In … xb_test_sdc gained tests/modules/xb_test_sdc/components/image-optional-with-example/image-optional-with-example.component.yml and friends, to be able to test all 4 permutations of "with vs without example, required vs optional".

Reproduced with image-optional-with-example. 👍

From an SDC author POV, image-optional-with-example should indeed work WITHOUT specifying an image.

But … from an XB data model POV, that does not quite make sense, because we cannot map the default of https://example.com/cat.jpg to a media entity.

In other words: #3501902: Adding the Image component results in a state considered invalid made it possible to generate a preview without having to store

• either the hardcoded absolute URL (such as in xb_test_sdc:image-optional-with-example's https://example.com/cat.jpg)
• or the SDC-included default image (such as in experience_builder:image's 600x400.png)

in a Media entity.

(@lauriii stated that was unacceptable, and we understand — see #3501902 for details).

What this issue is about, is to expand from using the unstorable-as-a-media-entity value to generate a preview, to also to save it.

The problem is that kind of would violate the fundamental XB data model — it would mean that this component instance does not have explicit inputs stored as fields.

IOW, this would require being allowed to store the fallback \Drupal\experience_builder\PropSource\UrlPreviewPropSource we introduced for preview purposes (hence the name), and accept essentially arbitrary values in the stored data, by doing:

diff --git a/config/schema/experience_builder.schema.yml b/config/schema/experience_builder.schema.yml
index 06dd07fe2..3b496af58 100644
--- a/config/schema/experience_builder.schema.yml
+++ b/config/schema/experience_builder.schema.yml
@@ -197,7 +197,6 @@ experience_builder.page_region.*:
             absence:
               - dynamic
               - adapter
-              - url-preview
             presence: ~
           tree:
             absence: ~
@@ -383,7 +382,6 @@ field.value.component_tree:
         absence:
           - dynamic
           - adapter
-          - url-preview
         presence: ~
       tree:
         absence:
@@ -418,7 +416,6 @@ experience_builder.pattern.*:
             absence:
               - dynamic
               - adapter
-              - url-preview
             presence: ~
           tree:
             absence:

I'd like @longwave's thoughts about this, because he and I worked the most on #3501902: Adding the Image component results in a state considered invalid.

⚠️ IOW: this is literally about everything I warned against in #3501902-42: Adding the Image component results in a state considered invalid.

AFAICT #9 explains/predicts why @longwave in #3508077-15: Default image for SDC with optional image is lost after changing value for another SDC prop wrote that it feels wrong. 😅

I'm not sure why this is in "Needs review" without an MR or apparent conclusion in the comments. I'll assume it's supposed to be "Needs work". Also, I may as well affirm, as long as I'm here, that the problem does, indeed, still exist in 0.x.

heyyo and I have run into this as noted here:

#3511805: Fix or workaround XB image alt bug that prevents saving SDDS components

which makes it difficult for the Driesnote as you'd have to replace each image in the SDDS components with something from the media library to be able to save the page which we want to be able to demo.

I bet #3493943: SDC+JS Component Sources: split default values into `resolved` and `source` will help with this.

#14: see the comment in which I marked it Needs review — the proposal needed review.

I'm not sure if this was addressed or mentioned before, I was testing out the Experience Builder (Drupal 11.1.4 - DDEV PHP 8.3) today, looking at the components and stuff, then used the Undo button (circled arrow <- ) and when I tried to save the button I try to click the button a few times and did nothing, when I looked at the console I saw this error.

I got the same response as mentioned above:

{
    "message": "No data provided to evaluate expression \u2139\ufe0e\u241centity:media:image\u241dfield_media_image\u241e\u241falt",
    "file": "\/var\/www\/html\/web\/modules\/contrib\/experience_builder\/src\/PropExpressions\/StructuredData\/Evaluator.php",
    "line": 44,
    "trace": [
        {

This is still an issue. Drupal stores the following error message:
OutOfRangeException: No data provided to evaluate expression ℹ︎␜entity:media:image␝field_media_image␞␟alt in Drupal\experience_builder\PropExpressions\StructuredData\Evaluator::doEvaluate() (line 44 of /Users/felix.mazeikis/work/pbx/web/modules/contrib/experience_builder/src/PropExpressions/StructuredData/Evaluator.php).

Ran into this again yesterday when trying to import config for a XB section.

In that case, the XB config had a media target_id that didn’t exist

For the XB-demo, we may just try to catch these and ignore them so at least there’s no error in the UI presented to the user

@f.mazeikis in #20: thanks for confirming.

@kristen pol in #21:

Ran into this again yesterday when trying to import config for a XB section.

In that case, the XB config had a media target_id that didn’t exist

That is expected; it's a known bug. See #3460232, and specifically #3460232-14: [PP-1] Support XB config entities with component trees that reference File/Media, use target_uuid and embed — aka: self-contained content entity dependencies. That's also what you reported in #7, I realize now — sorry for not making the connection sooner!

This issue's steps to reproduce are different: they don't involve any config entity at all. See #9: this issue is specifically about saving a component instance in an XB field (i.e. in a content entity).

Quoting myself from #9:

The problem is that kind of would violate the fundamental XB data model — it would mean that this component instance does not have explicit inputs stored as fields.

→ for that, I opened #3514672: [Needs design] DX & authoring experience: support `default` in addition to `examples[0]`, enables updating a component input across all existing instances the other day, because it's a general problem. This issue is simply the one where that general problem is most easily encountered, and in fact, unavoidable, for the reasons cited in #9.

In #3514672: [Needs design] DX & authoring experience: support `default` in addition to `examples[0]`, enables updating a component input across all existing instances, @f.mazeikis, @mglaman and I argue delve into this general problem space. The general problem should be solved there.

The root cause here though is that even though there is a StaticPropSource with a reference to a Media entity for the image … the entity reference field is empty at the time of saving!

So, rescoping this issue to make that mysterious error 10x more precise, and make it an actually meaningful validation error. 👍

We had at one point code that auto created a media entity based on example URIs but remove it because it relied on the magic in findTargetForProps finding a file with the exact uri of the example, which wasn't very precise with things like FileExists::rename.

I feel we're going to keep coming up against this issue.

I'm trying to think of creative ways to get around this.

As I see it the issue is stemming from when an SDC component documents an object as the $ref: json-schema-definitions://experience_builder.module/image we automatically apply a certain storable prop shape that assumes a media entity

But we need flexibility to be able also handle the default value.

We have #3493943: SDC+JS Component Sources: split default values into `resolved` and `source` for some of this but I don't know if its going to solve this issue.

I'm spending today on both this and that to try and propose a solution.

#3493943: SDC+JS Component Sources: split default values into `resolved` and `source` is in. @larowlan, would you be able to take another look at this?

This is still an important issue to get done, but we started a shorter-than-usual sprint following DrupalCon, and are choosing to prioritize some other issues ahead of this one. @larowlan: if you come up with any ideas in the meantime, fantastic! Otherwise, we'll likely pick it up again in an upcoming sprint.

This might be solved already, I'll test tomorrow

(I wrote #22 while traveling to DrupalCon, but never found the time to finish it.)

There's 2 ways to trigger the reported error (No data provided to evaluate expression …).

1. Perhaps the first one (required image) should be moved to a separate issue to keep the scope tight.
2. The second one (optional image, with example) is what most of the people commenting on this issue seem to be focused on. This is the one that is deeply connected to #3514672: [Needs design] DX & authoring experience: support `default` in addition to `examples[0]`, enables updating a component input across all existing instances.

I added a proposed resolution for both to the issue summary.

While getting this going, I discovered a regression that makes it kinda impossible to work on this issue: #3517102: Regression caused by #3493943: existing optional image SDCS with example/default no longer render, adding SDCs with optional image to canvas fails.

FYI: this no longer triggers an error in HEAD for the image-optional-with-example-and-additional-prop SDC when saving it without selecting an image from the media library, but then when rendering the end result, it does not use that same default value. That is what the second point in the proposed resolution aims to solve, and which #3514672: [Needs design] DX & authoring experience: support `default` in addition to `examples[0]`, enables updating a component input across all existing instances should refine.

In 0.x following the steps from the IS now results in

{
  "detail": "The 'url-preview' prop source type must be absent.",
  "source": {
    "pointer": "field_xb_demo.0"
  },
  "meta": {
    "entity_type": "node",
    "entity_id": "1",
    "label": "Test",
    "api_auto_save_key": "node:1:en"
  }
}

I think approach 1 from the IS makes most sense because then I believe we can drop UrlPreviewPropSource entirely.

Spent a while on trying to implement approach 1 but I'm going round in circles. In ::renderComponent() we can find the example value relatively easily but we still need to translate the example value into a usable URL, but we don't have the expanded schema available in order to detect which props (or child props in arrays) are URIs. Now I've got this far into it, doing this on the fly at render time feels quite late and quite expensive.

Putting this down again here because I'm not sure this is the right approach after all.

Postponed on #3517102: Regression caused by #3493943: existing optional image SDCS with example/default no longer render, adding SDCs with optional image to canvas fails

But pushed a branch that builds on that with a test the change that makes it work.

Basically the 'url-preview' prop source type needs to be stored for this to work

Comment above was unsaved from Friday. The blocker is in, I will reroll.

Over to Wim to review. Allowing storage of url-previews might be undesired - but I think in the case of an optional default image with no value it makes sense as the component creator has indicated it as a default value. This gives the option of updating to a different default image at a later point and it will still work.

Just to clarify, we're not storing the default image (as in the file) but rather a prop source of type url_preview which allows a just-in-time resolution of the URI to the default image relative to the SDC component. I think that is pretty low risk but I will defer to Wim and Dave.

Over to Wim to review. Allowing storage of url-previews might be undesired - but I think in the case of an optional default image with no value it makes sense as the component creator has indicated it as a default value. This gives the option of updating to a different default image at a later point and it will still work.

This is what my thinking was tending towards, too. And I'd swear I'd done exactly what you did here in some MR, but can't find it 🫣 Must've dreamt it 😜 (Or maybe this is because — GASP! — a month ago I wrote #9 in which I cited this as a potential solution, but back then I was less positive about it.)

This MR then significantly changes #3514672: [Needs design] DX & authoring experience: support `default` in addition to `examples[0]`, enables updating a component input across all existing instances: we do end up treating examples[0] as the default value. But that's actually already the case in HEAD — we just never did so for SDC props for which exampleValueRequiresEntity() returns true.

However, if we do this for XB fields (i.e. component trees on content entities), then we must also do this for everything else: PageRegions can run into the same exact challenge, and so can Patterns.

So, I'm +1 to this MR, as long as it is expanded to include

diff --git a/config/schema/experience_builder.schema.yml b/config/schema/experience_builder.schema.yml
index 2cbae2c0b..2e2a504b4 100644
--- a/config/schema/experience_builder.schema.yml
+++ b/config/schema/experience_builder.schema.yml
@@ -197,7 +197,6 @@ experience_builder.page_region.*:
             absence:
               - dynamic
               - adapter
-              - url-preview
             presence: ~
           tree:
             absence: ~
@@ -383,7 +382,6 @@ field.value.component_tree:
         absence:
           - dynamic
           - adapter
-          - url-preview
         presence: ~
       tree:
         absence:
@@ -418,7 +416,6 @@ experience_builder.pattern.*:
             absence:
               - dynamic
               - adapter
-              - url-preview
             presence: ~
           tree:
             absence:

Dave's out, so little point assigning to him 😅

Oh, and if we go and do this, we should:

1. rename \Drupal\experience_builder\PropSource\UrlPreviewPropSource to DefaultRelativeUrlPropSource or something along those lines
2. rename the source type prefix from url-preview to default-relative-url or something along those lines
3. update all comments to stop saying it's only for previews
4. because it now is ending up getting stored, more guarantees should exist; so it should get explicit test coverage in \Drupal\Tests\experience_builder\Kernel\PropSourceTest

Made those changes

Self-addressed all my feedback: fixed many docs, minimized storage space, added tests.

I changed too much for me to comfortably commit this. I'd like a +1 from @longwave or @larowlan.

+1 to the changes - great stuff!

re #47 I am going to merge since @larowlan gave a +1

Would still like @longwave's review even though this has already been merged 😅

So this looks good as a solution and follows the conclusion I came to in #33. There are two todos about clunkiness with no linked issue, commented on these on the MR.

I also tested manually with the xb_test_sdc SDCs and found the following:

1. "XB test SDC with optional image and heading" works correctly
2. "XB test SDC with optional image, with example" works correctly
3. "XB test SDC with required image, with example" works correctly
4. "XB test SDC with optional image, without example" does not work:
   • The component does not show in the preview and is unselectable (but can still be removed via the component tree)
   • The Settings tab shows "An unexpected error has occurred while rendering the component's form."
   • The PATCH request to /xb/api/form/component-instance/node/1 returns 400 Bad Request with an unhelpful body: {"errors":[""]}

However I think that should be dealt with in a followup - we should either consider that component incompatible (optional images must have examples) or we will have to provide some kind of default example for this case.

Assigning to Wim to read this and close :)

"XB test SDC with optional image, without example" does not work

Oh hm, I thought it was due to the optionality not being respected, but that's not the case:

{% if image and image.src %}
  <img src="{{ image.src }}" alt="{{ image.alt }}" />
{% endif %}

… it's simply because this renders to the empty string 🤪

This is not unique to images — the same could happen for an SDC with a single boolean input named enabled and whose Twig looks like this:

{% if enabled %}
  <p>Hi</p>
{% endif %}

… that's simply a nonsensical SDC.

IOW: I disagree with provide some kind of default example for this case, because it's not unique to images.

AFAICT we have two choices:

1. Make everything in XB gracefully handle the case of ::renderComponent() resulting in '' (i.e. no markup)
2. Make \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::checkRequirements() parse the SDC's Twig and detect whether it'd render to the empty string, and if so, say this doesn't meet the requirements.

(Although the second choice still doesn't solve this problem for code components — but it really is an absurd edge case…)

That'd handle the first bullet under #53.4, but not the second and third. Can you elaborate on how those can happen? :O

I think we should do the first choice because the second option possibly involves solving the halting problem; we can't know for sure just by parsing Twig whether it will actually output anything - we could guess but we might also be wrong in some cases.

I think the sensible thing to do is output a wrapper in the case that any component doesn't contain any tags - an SDC or other component could just output text with no tags as well - but let's discuss this in a followup.

I think the second and third bullets are a bug, and that needs investigating separately, will open another issue for that.

Perfect, thanks!

Opened:

#3518247: [PP-1] Clean up JSON schema normalization and reference resolving in PropShape
#3518250: [PP-1] Handle components instances rendering nothing: wrap them in a container
#3518253: SDCs with optional images without examples cannot be placed

Otherwise, we are done here.

Thanks for filing those — responded to all three! 😄🏓