Create an Image SDC that can be included by other SDCs

@lauriii and I discussed this and decided to make it a beta blocker. Every design system includes components with images, so providing the XB recommended way to do that is important to include in the beta.

This is (slightly) related to ✨ Component restrictions Active per discussion with @lauriii in slack. We want to restrict an SDC on the root canvas instead of a concrete slot, but the mechanism could be similar or even the same API. For now it's fine to hard-code it.

I actually don't think this is exactly the same as what's being worked on there. What we want to do here is prevent exposing the component to the UI at all. Something along the lines of `hide_from_ui: true` property in the SDC schema. The difference is that this is the component indicating it should be hidden from the UI, vs ✨ Component restrictions Active where it would most likely be the canvas indicating what it can accept.

Something along the lines of `hide_from_ui: true` property in the SDC schema

Field types and Views plugins can have a no_ui key in their plugin definition, so we can continue with that pattern here, just in the SDC YAML instead of in a PHP attribute.

Probably worth opening a core issue to propose adding the no_ui key to SDC definitions. We don't need to hold this issue up on that actually landing in core, but it would be good to get some initial feedback on it from SDC system maintainers.

I'll work on the backend infra.

I see two alternatives:

• Add a check for ComponentMetadataRequirementsChecker::check for noUI == TRUE and not create the component config entity. Profit.
• Add no_ui to Component

I'm tempted for 2, even if potentially more complex, because:

• We can track versions when the noUI component changes.
• We can add dependencies/be dependant on.
• I suppose this might be a thing at some point for code components. Where as a site admin I cannot delete a code component because it's in use in some old page, but I want to prevent editors to add it to new pages.

Before moving further would love thoughts on #10 + the MR direction.

re: #10: Another reason for #2. When/if we migrate from LB, we might want people to use/edit pages with layouts, but we might not want people to add layouts to new pages. Those would be no_ui components.

Looks good to me

#10:

I suppose this might be a thing at some point for code components. Where as a site admin I cannot delete a code component because it's in use in some old page, but I want to prevent editors to add it to new pages.

This is already possible: the Component config entity's status needs to change to FALSE.

We can track versions when the noUI component changes.

What is the purpose of this? 🤔 A Component config entity that keeps getting updated but is not available for anybody to instantiate … what value is there in/problem does it solve to then have a log of all changes that were made to that component?

We can add dependencies/be dependant on.

This is definitely true! And that is where the prior point becomes valuable too, I suspect? 🤔

I am still not convinced we need #10.2 aka . For the dependency reason above, the first option in #10 is also not good.

Unless I'm missing something, a hybrid actually makes more sense:

1. Add a check for ComponentMetadataRequirementsChecker::check for no_ui == TRUE and DO create the component config entity. But set status = FALSE.

That means zero new infrastructure, zero config schema changes, but it achieves everything outlined in #10? 🤓🤞

I suppose this might be a thing at some point for code components. Where as a site admin I cannot delete a code component because it's in use in some old page, but I want to prevent editors to add it to new pages.

This is already possible: the Component config entity's status needs to change to FALSE.

Maybe I'm wrong, but that would not render the component on existing pages where it already existed, right? As a content manager, I'd like to be able to not affect existing pages, but still block editors from adding them again.

See also #14 where this might be even more relevant.

Maybe I'm wrong, but that would not render the component on existing pages where it already existed, right?

You're wrong :)

Component's status only controls whether those components are available for content creators to instantiate. Disabling a Component does not and MUST NOT prevent existing instances from working. In fact, that is exactly why Component config entities were introduced: to use (config) dependency management to guarantee that all existing component trees will continue to work (unless of course you're going to start hacking code and/or bypassing validation).

See the original issue that introduced the Component config entity for more detail if you like: 📌 "Developer-created components": mark which SDCs should be exposed in XB Fixed .

#19 Sure, there's no way we could manually disable SDCs as we can now if what you said wasn't true 🤦🏽‍♂️

Repurposed the MR with that info. Sadly the SDC metadata noUi attribute depends on the core change, we cannot really leap ahead unless we decided to re-parse the metadata (which we don't want).

This should be ready. We cannot leap ahead of core though. So they will be visible before ✨ Allow SDCs to be marked to be excluded from UI Active is released though.

As I wasn't explicit and I see now we didn't discuss this: I'd expect we land !1307, and a different MR for the actual component in this very same issue.

I asked @larowlan if there's any way the core MR could be backported to 11.2.x, and he clarified that as a new feature it's a no-go.
Per previous conversations, we should just hardcode the component name for now until we have a release and can require 11.3.x.
I didn't do this yet as I don't know the name of the component (image? responsive_image?)

justafish → closed merge request !1339

justafish → changed the visibility of the branch 1.x to hidden.

TIL ✨ Allow SDCs to be marked to be excluded from UI Active happened!

RTBC'd the "pure infra" MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1307 — not the other MR.

Needs follow-up issue to remove the work-around once XB requires >=11.3.

Back to and to Sally for the other MR on this issue :) Thanks, @penyaskito!

Created 📌 [11.3.0-and-up] Remove hardcoded Image SDC from `\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::componentMeetsRequirements` Postponed

Did an initial review of the other MR, and raised 2 major concerns 😅

Also, the issue summary is very outdated, because it says

See https://www.drupal.org/project/experience_builder/issues/3532718 📌 Improve the front-end DX of Active for relevant upcoming changes to how srcset is generated, however some of the work here can be started before that's completed e.g. creating a "hidden" component, the card component, and a skeleton for the new image component

and I believe that the thing that concerns me most is something worth calling out in the issue summary.

📌 Improve the front-end DX of Active landed 9 days ago.

@justafish and I met about this MR — we both agrees that https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... is problematic.

The reason that code is in there, is because @justafish understood from talking to @lauriii and from this example in the issue summary:

{{ include('image', {
  src: './gracie.jpg',
  alt: 'Gracie is awesome',
  class: 'max-content',
  fill: true,
  sizes: '(max-width: 768px) 100vw, (max-width: 1200px) 50vw, 33vw'
})}}

… that:

• (the examples.0 in the metadata) must ALSO be able to generate a srcset
• and on top of that, must ALSO be able to generate a srcset

Those 2 bits are why this (in-progress) MR is copying local files (both inside the SDC and outside) to some location in public://: because otherwise no derivative for it can be generated, since \Drupal\image\Controller\ImageStyleDownloadController::deliver() does NOT support "shipped files" (aka files shipped as part of a module or theme), even though the logic in \Drupal\image\Entity\ImageStyle::buildUri() suggests it does (see the else branch).

(For debugging: install XB, then navigate to /sites/default/files/styles/xb_parametrized_width--640//core/tests/fixtures/files/image-1.png.webp to generate a 640px-wide derivative of core/tests/fixtures/files/image-1.png. Put a breakpoint in the aforementioned methods and observe: it'll try to use core as the scheme.)

This is why @justafish resorted to copying shipped files into public:// — because core's ImageStyleDownloadController does not support generating derivatives for shipped files.

But precisely that is the very worrying piece here: a Twig extension writing to the file system as a side effect. 😱

This opens a can of worms:

1. What if the default image is updated (its contents are changed but its filename is not): that should then be reflected too in the public:// copy. How do we efficiently detect such a change?
2. What about multiple SDCs with the same filename? Multiple extensions with the same SDC name? In core, the File entity does the deduplicating of identically named files; here we would become responsible. A naming scheme is conceivable (public://sdc-default-images/<extension name>/<SDC name>/<filename>) to avoid that, but it's not great.
3. What about the extra disk space consumption? Wouldn't it be better to just use the actual shipped files instead?

Conclusion

1. IMHO the value of generating a srcset for a shipped default image in an SDC is questionable — it is unlikely to be enormous anyway. @justafish understood @lauriii to want this, but based on the issue summary, I'm not confident that is the case. Doubly so because we don't currently do that in HEAD either and this was jointly agreed in ✨ Add automated image optimization to image component Active as a reasonable trade-off.
   EDIT: I found the original comment covering this → .
2. If there is sufficient value in that in Lauri's eyes, then the correct (and maintainable) solution would be:
   1. Rewrite the relative-to-the-SDC URLs to relative-to-Drupal-root URLs, aka exactly what \Drupal\experience_builder\PropSource\DefaultRelativeUrlPropSource + \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::rewriteExampleUrl() do in HEAD.
   2. Subclass core's ImageStyleDownloadController and add support for shipped files.
3. 99% of the value is NOT in making the "shipped images in an SDC have srcset derivatives" bit work, but in what the issue title says: . So: if @lauriii thinks this should happen despite all of the above concerns, then it'll need to be in a non-beta blocking follow-up.

Assigning to @lauriii to get feedback on this; meanwhile @justafish will push ahead with everything else.

It's fine to open a follow-up for generating a srcset for shipped images. Sorry, I should have made it clear that it's critical that shipped images work when using the image component, but we don't have to add the full support for that here. Once we have the follow-up, we can figure out how to prioritize that. In the meanwhile, we should just passthrough those images.

I think we need two follow-up issues where we should cover this for both SDC and code components:

1. Generate srcset for shipped images
2. Generate srcset for remote images from allowed 3rd parties

#34: perfect, that sounds completely sensible 😊 — thanks!

I've left some comments in the MR

Can we get some screenshots of uses of the new noUi SDC experience_builder:image (both preview + its component instance form) in the issue summary? 🙏

While reviewing, found some concerns, most importantly tests/modules/xb_test_sdc/xb_test_sdc.install.

Feedback on the MR + there's still 3 "Needs …" tags. 😅

I reviewed and fixed my findings, but would need clarification on the Playwright changes. Otherwise this looks ready to me.

I'm not sure if the follow-up from #34 is still needed, re-tagging just to be cautious. The infra follow-up for core 11.3 exists already at 📌 [11.3.0-and-up] Remove hardcoded Image SDC from `\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent::componentMeetsRequirements` Postponed

Thanks for the improvements here, this MR looks much more solid now! 🤩 🙏

In addition to @penyaskito's questions, I also posted a few to the MR — again about documenting the rationale to ensure maintainability (the commits of the past 24 hours made that a lot better — these are the last bits!).

But there's also one bit of missing test coverage (the Twig extension is updated by this MR but tests/src/Unit/Twig/XbTwigExtensionFiltersTest.php did not gain new test cases)

I'm not sure if the follow-up from #34 is still needed,

Yes, I think we still need 2 follow-up issues for these 2 things Lauri requested:

I think we need two follow-up issues where we should cover this for both SDC and code components:

1. Generate srcset for shipped images
2. Generate srcset for remote images from allowed 3rd parties

https://www.drupal.org/project/experience_builder/issues/3538858 📌 Generate srcset for remote images from allowed 3rd parties Active created for generating srcsets for remote images

I haven't created a follow up for shipped images as we can already do that 😊

Thanks for creating 📌 Generate srcset for remote images from allowed 3rd parties Active ! Posted a few questions there. Doing (final! 🤞) review pass here :)

I haven't created a follow up for shipped images as we can already do that 😊

No, we don't support that. We're copying a shipped image *to* a stream wrapper to test stream wrappers — quoting xb_test_sdc_install():

  $source = $module_path . '/components/card/balloons.png';
  $destination = 'public://balloons.png';
  $directory = 'public://';
  $file_system->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY);
  $copied_file = $file_system->copy($source, $destination, FileExists::Replace);

See https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... they literally show that no srcset is generated for sdc.xb_test_sdc.card (shipped image inside SDC) nor sdc.xb_test_sdc.card-with-local-image (shipped image outside an SDC) — the test expectation make this very clear. 😊

Finished review.

Issue created for something surfaced here but not caused here: 📌 DX: update `PropShapeToFieldInstanceTest` to not test all SDCs, but only those that meet XB's requirements Active .

Only fixed some nits, but also found some missing test coverage, and that revealed a bug. @justafish, please confirm that you agree with the changes I made for that — see the 2 commits referenced here: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

And thanks for teaching me https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... 😃

Wim is correct, we discussed this and though we both remember it working we can't actually reproduce it so have decided it was a Folie à deux 😆

Follow up created here https://www.drupal.org/project/experience_builder/issues/3539062 📌 `ParametrizedImageStyle`: Generate srcset for local unmanaged files Active

🤣

Thanks!