HTTP API for code component config entities

See if the work done in 📌 HTTP API to read+write PageTemplate and Pattern config entities Active can be leveraged.

💯 — and 📌 Improve or remove ComponentSourceInterface::getClientSideInfo() Active just made another XB config entity leverage it, and improved the infrastructure! 😄

You'll get this "for free" by implementing \Drupal\experience_builder\Entity\XbHttpApiEligibleConfigEntityInterface and updating the route requirement:

diff --git a/experience_builder.routing.yml b/experience_builder.routing.yml
index 96238d96..d360c9c9 100644
--- a/experience_builder.routing.yml
+++ b/experience_builder.routing.yml
@@ -115,7 +115,7 @@ experience_builder.api.config.list:
     _controller: 'Drupal\experience_builder\Controller\ApiConfigControllers::list'
   requirements:
     _permission: 'access administration pages'
-    xb_config_entity_type_id: '(component|pattern)'
+    xb_config_entity_type_id: '(component|pattern|code_component)'
   methods: [GET]
 experience_builder.api.config.get:
   path: '/xb/api/config/{xb_config_entity_type_id}/{xb_config_entity}'

👆 That will get you "list" and GET support. For POST, PATCH and DELETE, you'll need to (currently) expand \Drupal\experience_builder\Controller\ApiConfigControllers::denormalize().

This blocks ✨ Managing code components in library Active .

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.

Complication: auto-saving

See if the work done in 📌 HTTP API to read+write PageTemplate and Pattern config entities Active can be leveraged.

The work that 📌 HTTP API to read+write PageTemplate and Pattern config entities Active did and was finalized in ✨ [PP-1] Integrate saving sections with the backend Postponed is not applicable/transferable to this issue, unfortunately, because of the auto-save requirement. Pattern config entities ("Sections" in the UI) are very different: they are NOT auto-saved, and hence they have a much simpler kind of communication between client and server.

Create an HTTP API for creating, reading, updating, deleting, listing code component config entities implemented in ✨ Config entity for storing code components Active .

Actually, it's not quite this simple, because we won't be updating this config entity directly: it'll be auto-saved (see ✨ Auto-save code components Active ) and then published from an auto-save state ( ✨ Publishing code components Active ).

Scope expansion: support this for "global CSS" too

We need the exact same saving functionality for the config entity being added at ✨ HTTP API for code component config entities Active as we do here.

Just discussed both of those 👆 with @effulgentsia, @lauriii and @tedbow in a call. @effulgentsia offered to clarify this.

Turns out @effulgentsia essentially repeated in that meeting just now what he already wrote at #3500042-10: Auto-save code components → 😅 Sorry, Alex!

Actually, it's not quite this simple, because we won't be updating this config entity directly[...]

I think we will be when we do ✨ CLI tool to manage code components Active . I also thought we'd be doing POST/PATCH etc. requests in ✨ Managing components sourced as code components Active . I'll ping @effulgentsia to chat about this, so I get a better understanding.

Then I actually read the entire comment. 🙈

So yes, that's quite in line with how I understood the requirements, though I don't yet understand why PATCH is a deviation from correct semantics, but I assume it has something to do with the auto-saved version not being the same entity as the published one the way we store them at the moment?

Also related to #6: @tedbow's write-up WRT auto-saving considerations over at #3500386-6: Code Components should render with their auto-saved state(if any) when rendered in the XB UI → .

In #8, there's a solid reason for keeping PATCH support using the original semantics (the CLI tool): it'd trigger a full (config entity) save including validation. In contrast with an auto-save, which:

• does not validate (hence allowing invalid values)
• must leave the original (live) config entity untouched, and hence store these auto-saved values somewhere else

TBD: how we then do the auto-save: which HTTP method, URL, request body…

Discussed #6 through #9 with @balintbrews.

Our conclusion: take-away: keep everything for "code component" simple at first, ignore auto-saving support; do that in a subsequent sprint. Because getting this off the ground at all is more important than auto-saving. 👍

+1 to implementing PATCH as a config save as the scope for this issue and deferring the changes needed for autosaving to ✨ Auto-save code components Active .

For when we do autosaving, I think the right semantics would still be a PATCH, but on a different resource. Instead of the canonical URL of the config entity, it would need to be the URL for the working-copy of that resource. On the back-end the working-copy is the autosave record until we have Workspaces, and when we work on integrating Workspaces we'll need to figure out when something should be an autosave record vs when it should be a config save in a Draft workspace.

Even with the CLI, we'll want the CLI patching the working-copy, not making a change to the live site. Changes to the live site should still be gated behind the "Publish All" button.

I think the right semantics would still be a PATCH, but on a different resource.

👍 That's where my thinking was leading too. +1 for working-copy.

Even with the CLI, we'll want the CLI patching the working-copy, not making a change to the live site. Changes to the live site should still be gated behind the "Publish All" button.

I'm not sure about that, because that might make the CLI tool rather confusing. I imagine we'll actually want the CLI tool to support both "immediate publishing" (PATCH the canonical resource) and "make available for review" (PATCH on the working-copy of the resource).

Thanks, @balintbrews and @effulgentsia, now this issue is indeed as simple again as we thought at first 😄

✨ Config entity for storing code components Active is in! 🥳

f.mazeikis → made their first commit to this issue’s fork.

Unblocked @f.mazeikis: made the test pass (https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...) that he was stuck on — this was an oversight in #3499927.

Also reviewed this MR while on it anyway — this looks well on track 👍 Let's get this wrapped up today? 😊

Addressed the feedback, there's still a couple of questions in MR, but it's ready for another review pass.

Very close!

1. DELETE was missing from /openapi.yml — https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
2. </code> should not be a one-off method, but should be formally required on <code> — see https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... (every XB MR should move us closer to a more coherent codebase, especially when new patterns arise that call for expanded abstractions — especially if it'd be a low-effort refactor as is the case here)
3. Optionally: there's a lot of back-and-forth about status/inLibrary and about what the minimum request body should be. I asked @balintbrews for one more round of feedback. EDIT: he already responded: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... — let's take care of that to maximize XB's FE developers next sprint 😊

wim leers → credited larowlan → .

2 bits remain:

1. inLibrary → status, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... and https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
2. moving ::denormalizeFromClientSide onto the interface, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

I would like to propose that we don't use the machineName property as the id entity key in the JavaScriptComponent entity. We would like to allow users to update the machine name of a code component, which is not possible if that property is the id.

We could use the UUID as the id instead, which would be ideal for the HTTP API.

I already had a conversation about this with @larowlan and @effulgentsia, and got 👍👍. If this significantly delays landing this issue, a follow-up is fine, I already have the code written in ✨ Managing code components in library Active with machineName as the id.

Related issue

#23 I understand where the 3 of you are coming from at a high level. But:

1. config entities are specifically designed to have both a UUID and an ID. The ID cannot be the UUID. So: details to be figured out, that go against the grain of how config entities have been designed work at their core.
2. When making a JavaScriptComponent available for content creators, a corresponding Component config entity must be created. And once that Component config entity exists, it must continue to point at the same underlying "code component". Doing this by "machine name aka ID" results in a very clear situation, where the config dependencies are also crystal clear, and hence requirement 14. Configuration management is easily met. But in the new world you describe, this becomes very opaque and hard to audit: a Component config entity would be pointing to the corresponding "code component" config entity by UUID.
3. Perhaps most importantly: the end of the sprint is today, and doing all that in a follow-up is much preferable, because both of the above leave a lot of things to be figured out.

However … it appears @larowlan made it all work overnight anyway. 😅

Although it did result in much bigger MR. Reviewing…

While @larowlan got it all working, I still do worry about the impact this has on 14. Configuration management.

Turns out:

• ✅ #25.3 is moot because @larowlan got it all working 😄
• ✅ #25.1 is a non-issue, thanks to core having added a test config entity that does this (\Drupal\entity_test\Entity\EntityTestUuidId) … only ~2.5 months ago! 🤯 See 🐛 Cannot use UUID as entity ID Needs work .
• ❌ #25.2 still concerns me: the machine name is now allowed to change at any time. That just feels … wrong? 😅 It can lead to a lot of confusing situations — both for "code component" creators and the persona dealing with syncing config. Related: 📌 [PP-2] Add ComponentAuditabilityTest Active . And it's definitely not in scope for this issue.

So, not throwing away what @larowlan did, just extracting that into a follow-up MR. I think all of those changes could happen as planned, with a few changes: disallow changing the machine name once a corresponding Component exists (which requires modifying the new constraint @larowlan added), and continue to use the machine name to refer to it.

I think we could even use the \Drupal\Core\Entity\EntityTypeInterface::getKey() infrastructure for this, and use a new xb_component_local_source_id to identify that actually the machine name should be used, not the JavaScriptComponent config entity ID — related: 📌 Allow component source plugins that don't have a plugin_id setting Active . That would result in the stored component trees again having meaningful names for every component instance (much better for debuggability/DX!). The only place where you'd then continue to see UUIDs showing up instead of the machine name is in the config dependencies. That'd be more balanced IMHO.

So, I'm sorry, but reigning in the scope here so this can land without additional consequences that require a lot of additional discussion. New issue for #23: 📌 JavaScriptComponent config entities should have mutable machineNames Active .

The revert commit also shows how much of a scope increase this was: 17 files changed, 55 insertions(+), 224 deletions(-).

🙈 I also respectfully disagree with both @f.mazeikis and @larowlan WRT claiming that expanding XbHttpApiEligibleConfigEntityInterface is infeasible. 😇 See my response and the corresponding commit.

This IMHO leaves us in a much clearer place than before, and paves the path for ✨ Storage for global CSS Active not needing to modify ApiConfigControllers at all.

Plus, even if/when we do ✨ Storage for global CSS Active , this makes that simpler. Given that that is not a high priority issue, I'm hopeful that the increased clarity that commit brings will improve the XB DX for weeks/months to come.

Finally, @balintbrews pointed out an oversight: the config entity HTTP API listing omitted status=false config entities. Which has been fine so far, but not for this one. 😅 Fixed in https://git.drupalcode.org/project/experience_builder/-/merge_requests/5....

Awaiting final test run, getting breakfast while that happens 😊

drag-and-dropy.cy.js seems to be failing consistently, but … locally it passes:

So went ahead and merged, because ✨ Storage for global CSS Active needs to land next.

BTW, I bet the root cause is the cy.findByLabelText('Image') in

    cy.log('Drag image component out of the slot and to the root level.');
    cy.get('@layersTree').within(() => {
      cy.findByLabelText('Image').realDnd('[data-xb-slot-id="content"]', {
        position: 'top',
      });
    });

(which is the failing bit).

Because components initially all have the label "component", and it then is updated to the actual label:

cy.findByLabelText should continuously retry (for up to 5 seconds I think is the default) until it finds an element with the text it is looking for. I think the error would be something like "could not find element with the text Image" if that was the issue.

In the log it says uncaught exception TypeError: Cannot read properties of undefined (reading 'removeAttribute') which appears to be an error coming from JS (cypress fails immediately on JS errors). I wonder if Cypress is being too hasty and trying to interact with an element that is being re-rendered - but the click is happening on an element that got removed from the dom (because of the re-render).

A potential solution would be to split the check and the click action. But honestly it's a strange one and I don't know if this would actually make a difference.

cy.findByLabelText('Image').should('exist');
cy.findByLabelText('Image').realDnd('[data-xb-slot-id="content"]', {
    position: 'top',
});

"missed defects" this issue should've caught prior to landing:

• 🐛 ApiConfigControllers::post() should detect a conflicting ID and throw a 409 exception Active
• 🐛 XbConfigEntityHttpApiTest::testJavaScriptComponent doesn't change the component on the first PATCH request Active
• point 4 in #3503547-3: Display validation errors in dialogs that create XB config entities (code component, section) →

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

#35 is an incomplete list.

• 🐛 Javascript code components props, examples and description should be optional Active