Overview

First we should fix #3469421: `{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs!, then this becomes the next thing to fix.

The expected behavior is currently this broken image:

Root cause: we're passing the file URL (e.g. public://foo.png) to the image component, not the file URL (http://example.com/sites/default/files/2024-08/foo.png)

Proposed resolution

Make that not so terribly broken 😄 Crucial for #3454094: Milestone 0.1.0: Experience Builder Demo.

  • Modify the default config to correctly point to the url property of FileUriItem, instead of to its value.
  • (This will likely result in the same "actually, spaces in a URL are not valid" problem that #3469421: `{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs! already solved for Uri, which this would need to solve for ComputedFileUrl). Copy that solution if needed.)
  • 👆 This part is a "manual fix", and solves the immediate problem. But we should solve it generically 👇
  • For matching existing field instances: Update SdcPropToFieldTypePropMatcher + SdcPropJsonSchemaType::toDataTypeShapeRequirements() + \Drupal\image\Plugin\Field\FieldType\ImageItem::propertyDefinitions() to ensure that this match is automatically found, and that tests prove this.
  • For creating new field instances: update \Drupal\experience_builder\SdcPropJsonSchemaType::computeStorablePropShape() + test coverage.

User interface changes

Working image component:

CommentFileSizeAuthor
#22 images now properly render.gif811.98 KBwim leers
Screenshot 2024-08-21 at 1.53.27 PM.png16.45 KBwim leers

Issue fork experience_builder-3469436

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #3450586: [META] Back-end Kanban issue tracker
Related issues: +#3454094: Milestone 0.1.0: Experience Builder Demo
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property » Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property
Status: Postponed » Active

#3469421: `{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs! is in.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: Unassigned » tedbow

working on this

tedbow opened merge request !211

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@Wim Leers regarding our discussion early. I did retest and to get the image to show up correctly for manual testing all I have to do to is https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

  1. Change the default value for the field to use the URL property of the file item
  2. change image-uri in the schema.json

    3. file to "format": "url",

    If I don't do that I get the error

    Drupal\Core\Render\Component\Exception\InvalidComponentException: [image.src] Invalid URL format in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).
    Drupal\Core\Template\ComponentsTwigExtension->doValidateProps(Array, 'experience_builder:image') (Line: 109)
    Drupal\Core\Template\ComponentsTwigExtension->validateProps(Array, 'experience_builder:image') (Line: 42)
    __TwigTemplate_1ea3af69cc18cdfe7d9bc5f335f33eeb->doDisplay(Array, Array) (Line: 360)
    Twig\Template->yield(Array, Array) (Line: 123)
    __TwigTemplate_9ddc501ed58cc12c5336551d9efaef06___413594941->doDisplay(Array, Array) (Line: 360)
    Twig\Template->yield(Array) (Line: 40)
    __TwigTemplate_9ddc501ed58cc12c5336551d9efaef06->doDisplay(Array, Array) (Line: 360)
    Twig\Template->yield(Array) (Line: 335)
    Twig\Template->render(Array) (Line: 38)
    Twig\TemplateWrapper->render(Array) (Line: 234)
    Drupal\Core\Template\TwigEnvironment->renderInline('{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
    {% embed 'experience_builder:image' %}
    {% endembed %}
    ', Array) (Line: 54)

The renaming of image-uri to image-url is not necessary.

Obviously this in not correct fix but just wanted to mention because thought "format": "url", shouldn't make it work and I thought maybe I had changed something else so I double checked and can get it to work with just the 2 changes

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs work

#7.2: you really shouldn't need to change format: uri to format: url. That's not a real format, per https://json-schema.org/understanding-json-schema/reference/string#built.... I bet it's only superficially working because \JsonSchema\Constraints\FormatConstraint::check() handles non-official formats by … just accepting any value as valid. 😅

What happens if you don't change format? Do you get a validation error from the SDC subsystem? If so, then that would match my above hypothesis. If not: what exactly is not working in that case?

What's definitely still missing is the update of

␞␟{src↝entity␜␜entity:file␝uri␞␟value,alt↠alt,width↠width,height↠height}

to

␞␟{src↝entity␜␜entity:file␝uri␞␟url,alt↠alt,width↠width,height↠height}

in all test expectations.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I fixed 3 of the 8 failures for you — these are the ones under the https://git.drupalcode.org/project/experience_builder/-/blob/6efdbece061... umbrella.

The remaining failures fall under the https://git.drupalcode.org/project/experience_builder/-/blob/6efdbece061... umbrella.

tedbow opened merge request !223

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

So I don't think we can just change the default value

ACK — unfortunate indeed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3471171: Allow components to use textarea in favor of input

@tedbow FYI the work I did on #3471171: Allow components to use textarea in favor of input might help you grok the different areas at play better. The commits tell the story/sequence :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Great catch/call about uri vs uri-reference, @tedbow!

P.S.: #3454173: Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) landed earlier today and introduces a second place where this fix is sorely needed 😄

tedbow closed merge request !223

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: tedbow » wim leers

Assigning to @Wim Leers for review since everything except the E2E test are passing.

I think the previous test interactions might have only worked because the images were not showing up
for instance tests/src/Cypress/cypress/e2e/add-section.cy.js

 cy.getIframeBody()
        .find('[data-component-id="experience_builder:two_column"] .column-one')
        .first()
        .trigger('click');
      cy.findByLabelText('Column Width').should('exist');

What I am seeing locally in 0.x is that clicking on the .column-one element would open up the Column form. but in the MR version the image takes up the more of the column with so it then opens up the image element form.
Not sure how to change the test to click on .column-one but not the image or other element.

So leaving needs work for that. It would probably be better if someone with more Cypress experience worked on that because I only have a little experience with this. I would be interested to work on this problem but since this is the #1 issue for #3454094: Milestone 0.1.0: Experience Builder Demo now we should probably get it down as quickly as possible.

jessebaker made their first commit to this issue’s fork.

jessebaker’s picture

jessebaker commented

@tedbow I have fixed 2/3 of the failing tests. The 3rd test seems to be failing on something specific to this branch so I believe it’s a legitimate bug.
After choosing an image from the Media Library there is an error when making a request to /api/preview/node/1
as follows:

The website encountered an unexpected error. Try again later.<br>
<br>
<em class="placeholder">Drupal\Core\Render\Component\Exception\InvalidComponentException</em>
: [image.src] Does not match the regex pattern ^(/|http).*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ in <em class="placeholder">Drupal\Core\Theme\Component\ComponentValidator-&gt;validateProps()</em>
(line <em class="placeholder">203</em>
of <em class="placeholder">core/lib/Drupal/Core/Theme/Component/ComponentValidator.php</em>
). <pre class="backtrace">Drupal\Core\Template\ComponentsTwigExtension-&gt;doValidateProps(Array, &#039;experience_builder:image &#039;) (Line: 103)
Drupal\Core\Template\ComponentsTwigExtension-&gt;validateProps(Array, &#039;experience_builder:image &#039;) (Line: 42)
__TwigTemplate_331ff7e5a20fda144110fb9d0e1512e8-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array, Array) (Line: 123)
__TwigTemplate_a529a4e6ed6302d18950f54e7bf80640___162390326-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 40)
__TwigTemplate_a529a4e6ed6302d18950f54e7bf80640-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 335)
Twig\Template-&gt;render(Array) (Line: 38)
Twig\TemplateWrapper-&gt;render(Array) (Line: 234)
Drupal\Core\Template\TwigEnvironment-&gt;renderInline(&#039;{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
{% embed &#039;experience_builder:image &#039;%}
{% endembed %}
&#039;, Array) (Line: 54)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func_array(Array, Array) (Line: 107)
Drupal\Core\Render\Renderer-&gt;doTrustedCallback(Array, Array, &#039;Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725 &#039;, &#039;exception &#039;, &#039;Drupal\Core\Render\Element\RenderCallbackInterface &#039;) (Line: 825)
Drupal\Core\Render\Renderer-&gt;doCallback(&#039;#pre_render &#039;, Array, Array) (Line: 387)
Drupal\Core\Render\Renderer-&gt;doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer-&gt;doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer-&gt;doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer-&gt;render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension-&gt;escapeFilter(Object, Array, &#039;html &#039;, NULL, 1) (Line: 147)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b___1318261257-&gt;block_column_two(Array, Array) (Line: 430)
Twig\Template-&gt;yieldBlock(&#039;column_two &#039;, Array, Array, 1) (Line: 193)
Twig\Template-&gt;renderBlock(&#039;column_two &#039;, Array, Array) (Line: 66)
__TwigTemplate_99ea5cc98d60103a120c7f4dc604a8ad-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array, Array) (Line: 125)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b___1318261257-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 40)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 335)
Twig\Template-&gt;render(Array) (Line: 38)
Twig\TemplateWrapper-&gt;render(Array) (Line: 234)
Drupal\Core\Template\TwigEnvironment-&gt;renderInline(&#039;{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
{% embed &#039;experience_builder:two_column &#039;%}
  {% block column_one %}
    {{ column_one }}
  {% endblock %}
  {% block column_two %}
    {{ column_two }}
  {% endblock %}
{% endembed %}
&#039;, Array) (Line: 54)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func_array(Array, Array) (Line: 107)
Drupal\Core\Render\Renderer-&gt;doTrustedCallback(Array, Array, &#039;Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725 &#039;, &#039;exception &#039;, &#039;Drupal\Core\Render\Element\RenderCallbackInterface &#039;) (Line: 825)
Drupal\Core\Render\Renderer-&gt;doCallback(&#039;#pre_render &#039;, Array, Array) (Line: 387)
Drupal\Core\Render\Renderer-&gt;doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer-&gt;doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer-&gt;doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer-&gt;doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer-&gt;render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension-&gt;escapeFilter(Object, Array, &#039;html &#039;, NULL, 1) (Line: 80)
__TwigTemplate_77fd3c900ebe4612a401cff4784a3799-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 335)
Twig\Template-&gt;render(Array) (Line: 38)
Twig\TemplateWrapper-&gt;render(Array) (Line: 33)
twig_render_template(&#039;core/modules/system/templates/page.html.twig &#039;, Array) (Line: 348)
Drupal\Core\Theme\ThemeManager-&gt;render(&#039;page &#039;, Array) (Line: 446)
Drupal\Core\Render\Renderer-&gt;doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer-&gt;render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension-&gt;escapeFilter(Object, Array, &#039;html &#039;, NULL, 1) (Line: 81)
__TwigTemplate_3a9bda3284efa427aa1dce14c22e6c78-&gt;doDisplay(Array, Array) (Line: 360)
Twig\Template-&gt;yield(Array) (Line: 335)
Twig\Template-&gt;render(Array) (Line: 38)
Twig\TemplateWrapper-&gt;render(Array) (Line: 33)
twig_render_template(&#039;core/modules/system/templates/html.html.twig &#039;, Array) (Line: 348)
Drupal\Core\Theme\ThemeManager-&gt;render(&#039;html &#039;, Array) (Line: 446)
Drupal\Core\Render\Renderer-&gt;doRender(Array, 1) (Line: 203)
Drupal\Core\Render\Renderer-&gt;render(Array, 1) (Line: 108)
Drupal\Core\Render\Renderer-&gt;Drupal\Core\Render\{closure}() (Line: 593)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 107)
Drupal\Core\Render\Renderer-&gt;renderRoot(Array) (Line: 66)
Drupal\Core\Render\BareHtmlPageRenderer-&gt;renderBarePage(Array, &#039;&#039;, &#039;page &#039;, Array) (Line: 76)
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer-&gt;renderBarePage(Array, &#039;&#039;, &#039;page &#039;) (Line: 388)
Drupal\experience_builder\Controller\SdcController-&gt;preview(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware-&gt;handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength-&gt;handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState-&gt;handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
require(&#039;/Users/jesse.baker/Workspace/drupal11/web/index.php &#039;) (Line: 71)
</pre>
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Reproduced the last failure:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [image.src] Does not match the regex pattern ^(/|http).*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

Investigating 🕵️

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs documentation updates, +Needs screenshots

So close! 🤩

Great work here 😊

Posted a ~dozen review threads/suggestions on the MR, and an overall assessment: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » tedbow
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: tedbow » wim leers
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs documentation updates, -Needs screenshots
StatusFileSize
new images now properly render.gif811.98 KB

Docs updated. Nits addressed. GIF added to issue summary.

Let's ship this! 🚀

Thanks, @tedbow!

  • wim leers committed 96848d39 on 0.x authored by tedbow 
    Issue #3469436 by tedbow, wim leers, jessebaker: Fix the visually broken...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks! Confirming this is working for me with fresh install and I can swap out images with no problems... this will help a lot!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: Page builder » Shape matching

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.