Overview

In order to enable #3454173: Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) before we have #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings, we could hard code the image (json-schema-definitions://experience_builder.module/image) props to use the media widget temporarily.

Proposed resolution

  1. new configuration to allow declaring a preference for image vs media (which oddly would not be possible through a new experience_builder.settings.yml config, only through a new config entity, because simple config cannot have module nor config dependencies)
  2. updating \Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage() to respect that configuration

— #3461499-18: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings

… but there was also:
@todo Support a `hook_storable_prop_shape_alter()`, which does NOT allow modifying the schema, but does allow modifying the field type, widget and storage settings. There is a risk here wrt deps, but the fact that after https://www.drupal.org/project/experience_builder/issues/3463999, dependency checking will be handled by config dependencies for us. 👍

… implementing means we solve the original intent, but without any hardcoding 👍 — the act of installing the optional Media Library module would change the default.

That means this will also help with testing #3464042: Add ComponentAuditabilityTest 👍

Changes

  1. New: hook_storage_prop_shape_alter(), with explicit test coverage (HookStoragePropAlterTest) and API docs
  2. New: CandidateStorablePropShape, the mutable variant of StorablePropShape, used only for that alter hook. Has convenience to/from methods.
  3. New: media_library_storage_prop_shape_alter(), implemented by XB on behalf of Media Library, with explicit test coverage (MediaLibraryHookStoragePropAlterTest)
  4. Improved: StaticPropSource::randomizeValue() now has special handling for entity references, for reasons documented in detail
  5. 🐛 Changed: previously, all logic assumed using the default widget for a field type was fine. This has served XB well so far, but it falls apart for the Media Library use case: that uses the standard EntityReferenceItem field type, configures it to allow only media entities, but Drupal's field system is not smart enough to have storage settings-dependent default widgets. So we need to update StaticPropSource's 3 widget-related methods to require a Field Widget plugin ID to be passed in: changes + updated tests — this part could be split off to a separate issue.
  6. 👆 that was the blocker to write logic to automatically use the widget defined in media_library_storage_prop_shape_alter() — this superficially touches upon something @lauriii and I already identified earlier, but won't affect us in the short term: #3463996: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade
  7. ✅ … this means two crucial @todos added by #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings are now resolved

User interface changes

Without Media Library installed

With Media Library installed

CommentFileSizeAuthor
#12 Screenshot 2024-08-02 at 4.32.50 PM.png62.53 KBwim leers
#12 Screenshot 2024-08-02 at 4.32.18 PM.png70.22 KBwim leers
#8 Screenshot 2024-08-01 at 3.56.54 PM.png54.01 KBwim leers

Issue fork experience_builder-3462709

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

lauriii created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

"could", but why? There seems to be plenty of issues with front-end work to be done?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I filed this issue during our weekly planning with you, based on instructions I received from you. 🤔 I agree that the issue summary could use some more context but I don't have any more information than you to expand on this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Hardcode image props to use the media widget temporarily » [if no actionable front-end issues remain] Hardcode image props to use the media widget temporarily
Assigned: Unassigned » wim leers
Category: Feature request » Task
Status: Active » Postponed
Parent issue: » #3450586: [META] Back-end Kanban issue tracker
Related issues: +#3454173: Media Library integration (includes introducing a new main content renderer/`_wrapper_format`), +#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings

Yes, I said we could do that if needed. It'd unblock front-end discovery work on #3454173: Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) if there were not enough actionable work in #3450592: [META] Front-end Kanban issue tracker. But there's ~half a dozen of critical issues ready to be worked on there.

I'll self-assign to revisit this during this sprint if those half dozen front-end issues have landed. But I'd prefer to not do this, and instead just do #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings. But we first need to get on the same page on that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [if no actionable front-end issues remain] Hardcode image props to use the media widget temporarily » [PP-1] Hardcode image props to use the media widget temporarily
Issue summary: View changes

Discussed with @lauriii. Now that #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings is virtually done, it makes sense to keep this blocked on #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings.

Now there's a much more precise plan of action! 👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Hardcode image props to use the media widget temporarily » Hardcode image props to use the media widget temporarily
Status: Postponed » Active

#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings is in.

Wim Leers opened merge request !135

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new Screenshot 2024-08-01 at 3.56.54 PM.png54.01 KB

Still cleaning this up (hacked my way to success!), but I've gotten to the point where the \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget does load 😊

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

This will 100% need a dependency on the media module since we're assuming it is there, and even though it's not necessary to function the media library module should probably also be there since that's the widget we indend to use.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

💯

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Hardcode image props to use the media widget temporarily » Use Media Library widget for "image" PropShape if Media Library is installed
Issue summary: View changes
Related issues: +#3464042: Add ComponentAuditabilityTest

I just introduced hook_storage_prop_shape_alter() + docs + infra — that fixes a @todo and minimizes the hardcoded nature.

It is also an essential step towards contrib extensibility/overridability, which we already knew in #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings that we'd have to add.

Retitling accordingly.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Use Media Library widget for "image" PropShape if Media Library is installed » Introduce, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed
Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3463996: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade
StatusFileSize
new Screenshot 2024-08-02 at 4.32.18 PM.png70.22 KB
new Screenshot 2024-08-02 at 4.32.50 PM.png62.53 KB

Review suggestion: see the Changes section in the issue summary for the big picture. That should help you review this.

Alternatively: the commit history walks you through it via the commit messages.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Introduce, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed » Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed

🤣🙈

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This conflicted with #3462247: Start passing entity type/entity ID context to SdcController from the FE on the back-end side, and then tests failed here, due to a regression on the front-end side introduced by that same issue/MR: #3462247-17: Start passing entity type/entity ID context to SdcController from the FE. Trivial fix though :)

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks good. I made 2 very minor changes

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » bnjmnm

@bnjmnm in #9: I know I agreed with you in #10, and then I didn't do it!

The reason is simple though: media_library_storage_prop_shape_alter() literally only is executed if the Media Library module is installed — even though that hook is implemented in experience_builder.module. It's simply implemented by XB on behalf of Media Library. It's a technique we've often used in experimental modules in Drupal core :) Hence also the without/with screenshots in the issue summary!

And as a bonus: that alter hook implementation checks even something more granular, to be on the safe side:

    // @todo Allow configuring which media type, for now this assumes the
    //   Standard 'image' media type.
    // @see core/profiles/standard/config/optional/media.type.image.yml
    if (!MediaType::load('image')) {
      return;
    }

  • Wim Leers committed 09b5d998 on 0.x 
    Issue #3462709 by Wim Leers, tedbow, bnjmnm: Introduce `...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: bnjmnm » Unassigned
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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