#stable blocker

@wim leers Re: #31 I have update the summary. I have had a chance to update 📌 Document the reasons for not using Workspaces for saving in 0.2.0 Active but I think this issue can be reviewed. Most of the back-end changes are in \Drupal\experience_builder\Controller\ApiLayoutController::validateAutoSaves. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testOutdatedAutoSave is the most explicit test of this. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatchTest::test() also catches ConflictHttpException which is thrown from validateAutoSaves()

I need to self-review this yet, but feels close. Bálint changes look good to me, thanks!

Added test coverage for regions to \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testOutdatedAutoSave. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatchTest::test already had a test for a conflict with a region

Change the MR so that \Drupal\experience_builder\Controller\ApiLayoutController::patch only validates the region that contains the component being updated.

I need to add test to demonstrate how this works

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

updated more.... but still more to update

leaving assigned to myself

re-rolled, added some more code comments and updating the summary but not finished. Leaving assigned to myself as I am working on the summary

How do I begin reviewing this MR? There's lots of comments here, and the issue summary doesn't talk about the client_id and clientInstanceId this MR is adding.

Can we get an up-to-date issue summary to facilitate a review? And ideally also sprinkle ℹ️ … comments over the MR to make it clearer still 🙏

P.S.: if 📌 Document the reasons for not using Workspaces for saving in 0.2.0 Active had landed, then I'd have asked to update that ADR. In fact … any reason not to do that now, @tedbow? 😇🙏 That'd be even better than an issue summary update!

📌 For selective reverting add DELETE auto-save endpoint Active is in.

Any SDC that has enum without the necessary meta:enum will no longer be available to XB users, and will now appear in the list of disabled XB components at /admin/appearance/component/status, with an explanation of why.

I'm not entirely convinced that we should not allow SDCs without meta:enum to be used in XB because it is possible to achieve an acceptable UX without providing meta:enum, albeit not in all scenarios. For example, for margin-top, you could choose to provide options '4px', '6px', and '8px'. In this case the same value could apply both as the human readable and machine readable value.

We could still allow translating the enum options but keep the machine readable names consistent across translations or we could simply prevent translating these enums.

I don't think this needs to block this issue from being committed. It seems that we could loosen this requirement in future if this would require a non-trivial amount of work to change.

I think 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active will resolve this, postponing

https://www.drupal.org/project/drupal/releases/11.1.8 →

@tedbow: can you please confirm ✨ Publish all changes within a database transaction to allow rollbacks Active is still okay as a rather than a ?

Closed 🐛 Trying to remove code component from page immediately after attempting to edit it gives 404 error Active as a duplicate of this.

Thanks for pushing this forward!

Unsure how component versions are stored in the config entities - need guidance on how to extract/compare versions!

See \Drupal\experience_builder\Entity\VersionedConfigEntityInterface::getVersions() and \Drupal\experience_builder\Entity\VersionedConfigEntityInterface::getVersionSpecificDependencies().

All tests passing 🎉

Something that doesn't have e2e tests is the code editor, but we need to fix it to at least send the same values in enum as meta:enum.
Bálint volunteered to write this.

@balintbrews For now I'd expect to send the same added values as labels. The only thing to take into account is replacing dots with underscores. Something like:

const getMetaEnumValue = (x: string) => x.replace('.', '_');

const enumValues = [
      '3.14',
      'b',
      'c',
    ];
const metaEnums = Object.fromEntries(new Map(enumValues.map(value => [getMetaEnumValue(value), value])))
console.log(metaEnums);


//{
//  "3_14": "3.14",
//  "b": "b",
//  "c": "c"
//} 

This needs work for:

1. multiple remarks on the MR
2. but most importantly because it's not yet resolving an author-provided URL to the corresponding oEmbed URL, which is why remote videos refuse to render

Note: I'd be totally fine with splitting this across multiple MRs, so you could already land the "local video" MR immediately.

Finally: this is not yet handling document links.

Making @f.mazeikis' observation easier to find: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....

Found the culprit. oEmbed videos are passed as-is:

<iframe src="https://www.youtube.com/watch?v=v747p7xEcgg" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>

… which is incorrect.

This should actually render something like

<iframe src="https://example.com/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>

Why? Because that's how core renders remote videos successfully. See for yourself by doing:

1. Go to /admin/config/media/media-settings and enable stand-alone media URLs
2. Create a video media entity for some YouTube video
3. Navigate to /media/N
4. Video appears!
5. Put a breakpoint in OEmbedFormatter::viewElements(), observe the different steps, specifically how $url and $resource_url exist. This results in:
   

   array (
     '#type' => 'html_tag',
     '#tag' => 'iframe',
     '#attributes' => 
     array (
       'src' => 'http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g',
       'scrolling' => false,
       'width' => 200,
       'height' => 113,
       'class' => 
       array (
         0 => 'media-oembed-content',
       ),
       'loading' => 'lazy',
     ),
     '#attached' => 
     array (
       'library' => 
       array (
         0 => 'media/oembed.formatter',
       ),
     ),
   )

I think this MR needs to perform that
https://www.youtube.com/watch?v=v747p7xEcgg
👇

http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g

transformation, by adding a new computed property on the field definitions for the oEmbed-using MediaType bundles of the Media entity type. That can be done using hook_entity_bundle_field_info_alter(). The MR already needs that for something else too: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

(The hash is coming from \Drupal\media\IFrameUrlHelper::getHash(), the rest from \Drupal\media\OEmbed\UrlResolver::getResourceUrl().)

This would also result in \hook_oembed_resource_url_alter() etc firing (see the example implementation: it rewrites all YouTube URLs to the cookieless variant!)

YAYAYAY!

First local video from Drupal's Media Library rendered via a component:

First remote video (about XB of course — Lee's talk on YouTube!) rendered via a component:

… unfortunately some kind of issue with YouTube refusing things, probably CSP-related, we can figure that out next 👍

D'oh — no, actually — I misinterpreted!

This is actually similar to 🐛 Enabling XB for managing page template breaks Olivero header Active , and there's nothing we can do about this.

The reason is essentially the same as #3505629-2: [upstream] Olivero's header works only correctly when using the "block" page variant → :

• Drupal core's BlockPageVariant renders the contents of block plugins with an extra wrapper: that's what block.html.twig is about, and that's where <div class="block__content"> is coming from
• XB is rendering all block plugins as-is, without adding more wrappers.

We could avoid this by doing something like core's \Drupal\block\BlockViewBuilder::preRender(), which is the "view builder" for the Block config entity type (which is IRRELEVANT/NON-EXISTENT when using XB!) that is responsible for this, see:

      // Place the $content returned by the block plugin into a 'content' child
      // element, as a way to allow the plugin to have complete control of its
      // properties and rendering (for instance, its own #theme) without
      // conflicting with the properties used above, or alternate ones used by
      // alternate block rendering approaches in contrib (for instance, Panels).
      // However, the use of a child element is an implementation detail of this
      // particular block rendering approach. Semantically, the content returned
      // by the plugin "is the" block, and in particular, #attributes and
      // #contextual_links is information about the *entire* block. Therefore,
      // we must move these properties from $content and merge them into the
      // top-level element.

I'm not sure I understand why/how yet, because
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::renderComponent() uses '#theme' => 'block'

Related: 🐛 Placeholders/#lazy_builder is not supported for block component rendering Active + 🐛 Preview is not accurate for the site branding block Active .

Needs debugging to determine the root cause. Thanks for the detailed bug report!

Implemented feedback from #10 ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active , updated simple/video SDC component and added new simple/ombed_video SDC component. This was useful for testing via UI, but not quite sure if we want to keep it.
Added comments to MR, moving to needs review.

• Added the array $version_ids = [] parameter to the method signature as requested
• Implemented the filtering structure that loads each config entity when $version_ids is provided
• Maintained backward compatibility (empty array works as before)

What's left:

Need to implement the actual version checking logic inside the filter function (currently has placeholder return true;)
Unsure how component versions are stored in the config entities - need guidance on how to extract/compare versions!

Did you push all commits?

I have implemented the suggested changes. If there are any additional updates or modifications required, please let me know.

I moved the "beta blocker" tag to just 📌 Create UI for selective publishing of changes Active and tagged the other children as "stable blocker".

tedbow → closed merge request !1080

changing title to make clear we are only handling content entities, node and pages with layouts

We will need this too 📌 For config entities add client support conflict detection via the `autoSaves' key Active

In the interests of keeping things as simple as possible on the FE, I was hoping to get something like the following

JSON:

{
  "drupalSettings": {
    "xb": {
      [...]
      "permissions": [
        "read global regions", "update global regions", "delete global regions", "read sections", "update sections", "delete sections", ...
      ],
    }
  }
}

Because then in the UI checking if we should display a button or not is as simple as, for example:

JSX:

<>
    {drupalSettings.xb.permissions.includes('delete sections') && (
        <button>Delete section</button>
    )}
</>

@jessebaker thanks for testing this. I was testing this with nodes and it was working.

Basically entity have a seperate revision id but if we create a new revision on every save this won't increment. I guess Page is not incrementing which I guess makes sense because we don't have a way to revert to old revisions or view old revisions AFIACT

So I used the "changed" time also in the revision for the client

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

re #24 ok might know what is happening.....

There are probably unrelated reasons why the client should always call /xb/api/v0/layout/xb_page/1 after publishing.

#20

As of my latest commit the FE now re-requests the page after publishing changes IF the current page is amongst the list of pending changes.

In my testing there is one remaining bug that I can see that is quite tricky to reproduce.

1. In a browser tab start from a blank page, no autosaves (everything published)
2. Copy the URL and open a new tab so you have two tabs with the same blank page state.
3. In Tab 1, add a Heading component and immediately publish all changes
4. Switch to Tab 2.
5. Preview is now out of date (showing blank still, but should have a Heading on it)
6. Add a Hero component
7. There should be a conflict warning but there isn't.

From what I can see, the issue is that the autosave hash has autoSaveRevision but it's always "1". Nothing I do ever seems to increment that number (that is actually a string).

 {
  "autoSaveRevision": "1",
  "hash": "efa8dd3ce4f5f9e1"
}

just formatting

Added the different scenarios of permissions that need to be exposed to the client UI.

h3 → h2

Thanks!

As of #19…  I can confirm THE BETA PARTS ARE DONE! 🥳🥳🥳

Signaling this is "done for now", i.e. for 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active but NOT for 🌱 Milestone 1.0.0: Production Sites Postponed , by marking this .

Following up on #15 and #16 (finally).

#15's /xb/api/v0/config/component case was indeed a bug, and was fixed since then in 🐛 The component fails to render the library and props forms due to an unhandled runtime exception. Active , as described in #17.

#3518201 and #3518203 are indeed "nice-to-haves" for 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active .

So … adopting the same structure as we have for 🌱 [META] Production-ready ComponentSource plugins Active and 🌱 [META] Production-ready data storage Active to signal beta vs stable vs post-stable.

@isholgueras is now (hopefully) AFK :)

The MR for 📌 Decouple image shape matching from the `image` MediaType Active has landed — that provides all the examples necessary for developing, testing and debugging this issue's scope.

Tests are green! Follow-up for next steps exists.

Self-RTBC'ing and merging because nobody else knows the area well enough to review it.

wim leers → credited bnjmnm → .

Tests are passing, and are proving that it is now possible to target different fields (i.e. bundle-specific fields) of the same field type across multiple bundles of the same entity type.

This effectively makes this behave similarly to how 📌 Pages have images usable for SEO purposes Active handled the picking of the the image to use in Metatags. (And this will automatically start happening on existing sites with this change.)

And … it actually works thanks to @bnjmnm's work on the Media Library Widget:

Future work

Note: the "same field type" requirement I referred to is simply because the props being targeted in each FieldPropExpression cannot (yet) vary by bundle; only the field name can. This could be expanded in a follow-up issue, which would allow for e.g. different media sources that all provide similar media: the Image MediaSource (powered by an image/file field type for local images), but a "remote image" MediaType — for example powered by core's OEmbed MediaSource, using the (disabled by default) Flickr oEmbed provider.

Created 📌 Support image shape matching from the `image` MediaSource: support both the `image` + `oembed` MediaSources simultaneously Active for that.

Conflicts with 0.x fixed

The initial hard technical blocker:

new FieldPropExpression(EntityDataDefinition::create('media', 'image'), …)

needs to be able to be

new FieldPropExpression(EntityDataDefinition::create('media', ['image', 'acquia_dam_image']), …)

… but
\Drupal\Core\Entity\TypedData\EntityDataDefinition::create() does not allow that:

public static function create($entity_type_id = NULL, $bundle = NULL) {

Investigation led to discovering an upstream core issue for this: #2169813: Support deriving fields from entity definitions with multiple bundles → . Which … I happen to have contributed to 8 years ago for JSON:API. 😅👴

Related but different: 📌 Decouple image shape matching from the `image` MediaType Active .

I agree that #8 🐛 If user with less/different permissions edits a page after a user with more/different permissions data lost could occur Active would be an acceptable way to get us to stable. This could be then improved post-1.0.0.

@tedbow re: #18 #20

1) I think perhaps we should create/address this in a follow up
2) +1 to using 'changed' or even perhaps 'lastModified' and using a timestamp - I could see that information actually being useful elsewhere in the UI at some point.
3) After publishing I think we already refetch some data so I don't see it being a problem to update the page.