Also convert metadata (page data) fields in ClientDataToEntityConverter

I think this MR is quite a bit too optimistic?! 😅🙈

1. Easy: AFAICT the special read-only handling is simply because this MR is not yet performing the proper Entity Field API access checks: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...
2. Hard: I have strong doubts about the (undocumented!) assumption this MR makes: that the client sends Entity Field property values. AFAICT it does NOT send those, it just happens to for simple Field Types/Widgets, where the shape of the data in the Field Widget's HTML form happens to match that of the corresponding Field Type's prop. But that's definitely NOT guaranteed. See: all \Drupal\Core\Field\WidgetInterface::massageFormValues() implementations.

   I'm fine with not fully solving this here. But not acknowledging that is not acceptable IMO.

   In addition to acknowledging that, this code should point to a follow-up issue where that does get fixed. And #3446434: Document "Semi-Coupled Theme Engine" and "Redux-integrated field widgets" components landed 2 days ago, and documented precisely this in docs/redux-integrated-field-widgets.md.

@tedbow and I discussed this in light of the 'publish' functionality.
As we understand it, the publish feature will be similar to publishing a workspace. In the first instance it will take all auto-saved items and turn them into entity revisions. This won't make use of workspaces in the original implementation but it is hoped that it will by the end state.

If that's the case, then is there any need for this issue/controller? The only thing that writes to the entity will be the publish operation, i.e. all other controllers only write to the tempstore. In that case should we focus more on the publish operation taking all of the auto-saved items and 'promoting' them to entity-revisions?

And if so, that raises a number of UX questions:

• If there are multiple pages being published, what happens when some of them pass validation but others don't
• How do we convey that to the user
• What happens if we're publishing a large number of entities and we hit resource constraints - we likely don't want to reach for batch API here because the UX isn't on par with what XB is delivering

So thinking out loud, perhaps the frontend needs to keep track of the list of unsaved entities that a publish operation will actually publish? A list controller in Drupal can expose the list of unsaved entities so that this can be syncronized with the frontend. This would likely need to support pagination for the case of a large number of items. This list could deal with the reality that some pages won't have an entity ID until they are published - so the autosave controller will need to deal in UUIDs or similar. This list could be used by the editor to navigate between pages being edited. It would also allow for validation to provide meaningful actions for the user. e.g. they attempt to publish 7 pages, but pages 2 and 3 are invalid. Being able to present the list of unsaved pages to the user will allow them to navigate to the invalid ones. We could also possibly show invalid components in the tree outline in the editor. This also probably means that the controller endpoint for fetching the current layout needs to work with the auto-saved ID rather than entity ID.
In terms of addressing the 'large number of entities' I think if the publish controller is configured to return a 307 while each group is processed that should just work with the fetch API. But that puts us into a state where several entities could save and one could fail. Probably post 0.2 to consider once we have workspaces support.

So tl;dr I think we likely need a list controller that returns the current 'auto-save'/in-progress content - i.e. the things that publishing will publish. This will allow nice UI affordances like `Publish (3)` and also a UI to navigate between pages in progress from with XB.

@larowlan in #7

In that case should we focus more on the publish operation taking all of the auto-saved items and 'promoting' them to entity-revisions?

But if all those auto-saved-to-tempstore items exist only in tempstore, how can you preview them together? I guess that's your point: you won't "in the first instance" — i.e. that's for a later phase/release?

The first 2 UX questions you raise are exactly what not saving to tempstore but to a workspace would address.

The third UX question is a really good point! That requires clarification from Product Manager @lauriii and the design team, because not only is that a performance/scalability matter, it's also a UX concern: what if >100 changes are accumulated prior to publishing them all? 😨

@larowlan on the MR: in almost every of your comments on the MR you refer to the inability of this controller to update the Pattern config entities. But that's not relevant here. The controller being updated exists specifically for updating content entities that have an XB field. The Pattern config entity type already has full CRUD support via an HTTP API, see \XbConfigEntityHttpApiTest::testPattern() 😊

@tedbow in #8

1. This is my understanding for 0.2 too, but it will need to be supported later, right?
2. This is AFAICT still necessary, to power the "Publish" button?
3. I don't understand this leap. 😅
4. Even if there's an auto-saved version first, that may be invalid, and so we'll still need the client to talk to the server (to the controller that #3478565: Add Entity Update controller introduced) to do a "real" save, including actual validation?!
5. This part I disagree with I think — see the prior point. The client must have all the data anyway. So expecting the client to perform an "actual/real" entity save through an explicit HTTP API request to a controller, to receive validation errors etc. seems A) logical, B) reasonable, C) in line with eventually adding Workspaces support?
6. You say "before" because that way the XB UI would be informed of validation errors sufficiently early?

So I'm not at all following how we conclude #3489743: Create AutoSave service and HTTP API to retrieve all entities with pending changes is the correct next sttep.

#8.3/#13.3

@tedbow just talked me through this. I now understand it! The future Publish button will publish an entire workspace, meaning 1, 2 or possibly 100 content entities (and perhaps even config entities) all at once, with a single click of a button. Given that, a single request to publish everything auto-saved into tempstore is the closest current equivalent. So +1.

Per @tedbow and @larowlan, this should hence be postponed on #3489994: Create an endpoint to publish all auto-saved entities, which will refactor the logic that #3478565: Add Entity Update controller introduced into an explicit ClientToServerConverter service.

After that is done, this issue will then update that ClientToServerConverter service to also support (metadata) fields.

The third UX question is a really good point! That requires clarification from Product Manager @lauriii and the design team, because not only is that a performance/scalability matter, it's also a UX concern: what if >100 changes are accumulated prior to publishing them all? 😨

Needs input from @lauriii and/or design.

Blocking this issue on those clarifications, but it really should be spun off into its own issue.

If there are multiple pages being published, what happens when some of them pass validation but others don't
How do we convey that to the user

This is demonstrated by this user flow: https://www.figma.com/design/Ps3m4APGHIILsBrm0Uj31N/Experience-Builder?n.... Let me know if you want me to record a short video to walk through that since this is one of the more complex flows and it might be helpful.

What happens if we're publishing a large number of entities and we hit resource constraints - we likely don't want to reach for batch API here because the UX isn't on par with what XB is delivering

We can accept the scalability problems for this initial state. We'd probably publish at maximum 10-15 entities with the intermediate solution. We could probably accept even if it was just 5-10 but then we'd need to document it somewhere.

Do these scalability concerns exist with the Workspace based solution too? From design perspective, it seems that the current solution is good enough for now. It would probably be worth opening a spinoff issue to define in a bit more detail how we'd expect the panel to behave in this scenario.

@larowlan on the MR: in almost every of your comments on the MR you refer to the inability of this controller to update the Pattern config entities. But that's not relevant here. The controller being updated exists specifically for updating content entities that have an XB field

Thanks, personally in my head there's an end-state where that should be one and the same and abstracted behind routing/storage layers - much like how LB has little distinction between writing to config entities (defaults), content entities (overrides) or simple config (navigation module). But that's for the future.

Do these scalability concerns exist with the Workspace based solution too? From design perspective, it seems that the current solution is good enough for now. It would probably be worth opening a spinoff issue to define in a bit more detail how we'd expect the panel to behave in this scenario.

In my opinion the issue still exists for Workspaces - it makes use of set_time_limit to continue to increase the execution time on a workspace publish event. This might work for low-end setups but for most HA setups the CDN won't hang around waiting for a response from the backend that long. E.g. Cloudfront, Cloudflare all have time-out limits after which they return a 504. So we will likely need to resolve that too if this is going to be for enterprise setups. I created a small POC yesterday to check if the fetch API respects a 307 header and it indeed does. So there is scope for us to craft a controller for the publish that processes chunks (e.g. we could use the existing settings.php flag for entity_update_batch_size) and returns a 307 with a Location header and JSON body that contains the progress. AKA an API based batch API. In my testing, I created a controller that responded with 4 307s and then a 201, and the FE did indeed make the subsequent requests to the Location header until the 200 was returned. So I think there's a world with nice UX where we put the operations in a queue and process them.

This is demonstrated by this user flow: https://www.figma.com/design/1sj0mnVdLFdYihrkAhR43u/Experience-Builder-%.... Let me know if you want me to record a short video to walk through that since this is one of the more complex flows and it might be helpful.

I don't have access to that design @lauriii - so yes please to a short video

much like how LB has little distinction between writing to config entities (defaults), content entities (overrides) or simple config (navigation module). But that's for the future.

LB writes to config entities and simple config?! 🤯

Where? (Do you have a URL? 🙏)

How? (Zero config was validatable at the time Layout Builder shipped!)

set_time_limit()

Woah! 😱 Indeed: \Drupal\workspaces\WorkspacePublisher::publish() uses that…

In my testing, I created a controller that responded with 4 307s and then a 201, and the FE did indeed make the subsequent requests to the Location header until the 200 was returned. So I think there's a world with nice UX where we put the operations in a queue and process them.

🤩👏

I don't have access to that design @lauriii - so yes please to a short video

😬 And no response yet ~24 hours later, escalated internally. Sorry about this 😞

@larowlan Here's the correct Figma link: https://www.figma.com/design/Ps3m4APGHIILsBrm0Uj31N/Experience-Builder?n.... Updated the original link too.

But if all those auto-saved-to-tempstore items exist only in tempstore, how can you preview them together? I guess that's your point: you won't "in the first instance" — i.e. that's for a later phase/release?

For 0.2.0, you won't be able to preview them outside of XB. However, you'll be able to preview them inside of XB. For example, if there's auto-saved changes to both the Page content entity (#3482259: Landing page integration: new content entity type for unstructured content) that you're currently viewing in XB, and the PageTemplate config entity, and one or more JS component config entities (once #3483267: [exploratory] PoC of Preact+Tailwind components editable via CodeMirror is done), then the XB canvas will reflect all of that. There's also been some ideation (not sure if a d.o. issue has been opened yet for the implementation) for a button in XB to remove most of XB's UI and just show its canvas full-screen to make it even closer of a "show me what this will look like on the real site" preview.

LB writes to config entities and simple config?! 🤯

Where? (Do you have a URL? 🙏)

How? (Zero config was validatable at the time Layout Builder shipped!)

• Simple config in core's navigation module
• Config entities in core entity view display and layout library module

Thanks for access @lauriii

@lauriii thanks, it looks like the design matches my mental model of this and your refinement questions match mine in #7

I think it would be good if in the list of 'unpublished changes' there was some sort of icon to indicate a page had an error, and if these could be links to lead the user to loading that page in the editor. Or even better if the errors could link to both the page and open the component in the side panel

The designers suggested that the errors should have a have a link to the page where the error appears, and it should indeed focus the error that the user clicked. I think it's a good point that it might be beneficial to additionally highlight the pages that have errors in the list of pages. 👍

@lauriii one thing the design is missing is translations, it is feasible that there will be multiple translations of a page in the auto-save store, so we might need to convey that in the review changes list somehow. Or should the list only return items in the current language?

We should display translations there but we need to determine how, because the design doesn't account for how translations are displayed in there (yet). I can think of at least two ways to display this:

1. Each translation becomes it's own row
2. Each row indicates which translations of the page has been changed

This is ready for another pass from Wim

The issue summary (and title) still (see #14) don't convey the scope. A screenshot/screencast might help, but AFAICT this is not modify the UI, only under-the-hood things?

Shouldn't this have Cypress E2E tests? If not, or while awaiting those, how should I manually test this?

Detailed review + manual test screencast in https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

Per the issue summary, this is clearly in the critical path for many things. ⇒ Bumped to Critical priority + tagged blocker.