HTTP API for code component config entities

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

💯 — and #3484678: Simplify ComponentSourceInterface::getClientSideInfo(), introduce ClientSideRepresentation value object, and leverage it 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 #3499947: Managing code components in library.

This is blocked on #3499927: Config entity for storing code components.

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 #3479982: HTTP API to read+write PageTemplate and Pattern config entities can be leveraged.

The work that #3479982: HTTP API to read+write PageTemplate and Pattern config entities did and was finalized in #3486203: Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model 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 #3499927: Config entity for storing code components.

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

That also means that a JavascriptComponent config entity must exist BEFORE auto-saving towards it is possible. That means that an empty such config entity must be created first by the UI, with at the very least a name/ID.

I'm told by @laurii and @effulgentsia that that is already taken care of by the UI designs, which is great. 👍 (I was looking at the wrong GIF in #3499919: [Meta] Plan for in-browser code components. 🙈)

We'll also need a new route to actually auto-save, because for the XB field and PageTemplate, that's handled implicitly by the "preview" route. For these in-browser code components, there's no need to talk to the server to preview them, so … no way to piggyback on that! Perhaps we want PATCH to be repurposed to mean "auto-save to ephemeral storage, do not actually save" for this particular config entity?

AFAICT that means we need:

• "list"
• POST (with only an ID being required)
• DELETE
• maybe PATCH, if we deviate from the "correct" semantics and use it for auto-saving
• "publish": AFAICT this would rely on the existing /xb/api/publish-all — per #3500042-5: Auto-save code components

Scope expansion: support this for "global CSS" too (AssetLibrary config entity type ATM)

We need the exact same saving functionality for the config entity being added at #3499931: HTTP API for code component config entities as we do here. (Because we'll assume a single such config entity with the ID global, there won't be the same "create named empty config entity first" challenge as above.)

Left a comment for that realization at #3499933-28: Storage for CSS shared across in-browser code components (and other use cases in the future).

Just discussed both of those 👆 with @effulgentsia, @lauriii, @longwave 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!

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 #3500042: Auto-save code components.

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 😄

#3499927: Config entity for storing code components is in! 🥳

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 😊

🏁 in sight!

3 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...
3. out-of-sync OpenAPI schema: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

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 #3310170: Use UUID as entity ID.
• ❌ #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: #3464042: Add ComponentAuditabilityTest. 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: #3502982: Rename ComponentSource settings' `plugin_id` to `local_source_id` or similar to not bias towards source plugins that don't use plugins under the hood. 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: #3503272: JavaScriptComponent config entities should have mutable machineNames.

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 #3499933: Storage for CSS shared across in-browser code components (and other use cases in the future) not needing to modify ApiConfigControllers at all.

Plus, even if/when we do #3499933: Storage for CSS shared across in-browser code components (and other use cases in the future), 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 #3499933: Storage for CSS shared across in-browser code components (and other use cases in the future) 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:

EDIT: that's for the Layers tab, but the same is true for the Library tab.

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:

• #3504366: ApiConfigControllers::post() should detect a conflicting ID and throw a 409 exception
• #3505174: XbConfigEntityHttpApiTest::testJavaScriptComponent doesn't change the component on the first PATCH request
• point 4 in #3503547-3: Display validation errors in dialogs that create sections
• #3505224: XbConfigEntityHttpApiTest missing test coverage for individual GET requests for JavaScriptComponent + AssetLibrary

#35 is an incomplete list.

• #3509026: JavaScript code components slots' examples and description should be optional
• #3509037: JavaScriptComponent config entities' `examples` and `enum` do not respect the prop's type