Overview

For #3455753: Milestone 0.2.0: Early preview we will not be using Workspaces.

All work in XB will be auto-saved and then the user will click a "Publish all" button that will convert all the auto-saves into real entity saves.

We have already done #3478299: Implement basic auto-save and #3489743: Create AutoSave service and HTTP API to retrieve all entities with pending changes is in progress to implement the rest of the functionality we will need

This issue will provide an endpoint to the client that will convert all the auto-save states into actual entity revisions.

Proposed resolution

  1. Extract most of the logic in ApiContentUpdateController into a new ClientToEntityConverter service
  2. Temporarily add a getAllAutoSaves to NotTheGoodAutoSaveTrait. We may not end up needing this if #3489743: Create AutoSave service and HTTP API to retrieve all entities with pending changes is committed first
  3. Create a new PublishAllController. This controller will:
    • call getAllAutoSaves, then for each auto-save data it will....
    • convert to an entity using the new ClientToEntityConverter service
    • If the conversion doesn't not have errors.....
    • validate the entity
    • save the entity

As mentioned in #3488368-8: Also convert metadata (page data) fields in ClientDataToEntityConverter once we are using Workspaces, after 0.2, the logic inside the PublishAllController may change. This controller may not be dealing with auto-save to entity conversion to entity saves. It may just moving entities to from 1 workspace to another like regular workspaces

Therefore I don't think we should worry too much about what would happen in 0.2 if you had 100 entities being saved in 1 request.
Since 0.2 is truley a preview I think it would be reasonable to put some limit on the number entity changes we support in XB at once. Because otherwise we likely will be solving performance problems that might go away when using Workspaces.

Follow-ups

Once the above is done we can re-open #3488368: Also convert metadata (page data) fields in ClientDataToEntityConverter but instead add the logic to the ClientToEntityConverter

User interface changes

None the "Publish all" button on the UI will be added in another issue.

Issue fork experience_builder-3489994

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

tedbow created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs followup

@tedbow just discovered most delicious can of worms 🤣

What if one content entity was modified by Ted, another by Wim, and they have different permissions granted? What if Ted is unable to edit what Wim was able to edit and vice versa? Who is then actually authorized to publish those accumulated changes in tempstore?

How does the workspaces module handle this?

Whatever XB does here MUST match however Workspaces handles it.

Let's not derail this issue with that problem and create a follow-up issue to investigate how Workspaces handles it, to determine if keeping things simple here creates more work for when XB adopts Workspaces entirely.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker

This now blocks #3488368 per #3488368-14: Also convert metadata (page data) fields in ClientDataToEntityConverter.

tedbow opened merge request !430

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
Related issues: +#3490588: EntityConstraintViolationList::addAll doesn't reset groupings

I've expanded the tests here and also improved the error response to include meta about the entity type and ID that the issue exists in, which I think (based on the designs) the FE will need.

I will see if we can easily include the component ID in the meta too, although I suspect that will need something that extends ConstraintViolation to add the additional data - so that's likely into follow-up territory

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I will see if we can easily include the component ID in the meta too, although I suspect that will need something that extends ConstraintViolation to add the additional data - so that's likely into follow-up territory

🤦 this is already in the source.pointer output 😎

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

The CI fail is coming from \Drupal\Core\Template\ComponentsTwigExtension::doValidateProps which indicates we have an issue with XBPreviewRenderer when there's an invalid entry in the auto-save storage

We will want these items to render without validation I suspect, or rather without taking out the whole page.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Added an extension to ComponentValidator with a method to opt-out of validation during preview

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Related issues: +#3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log)

I'm surprised by the changes in ApiPreviewController: based on the issue title + summary I didn't expect those in this MR.

@larowlan articulated why this is necessary in #8 + #9: because CI was failing without those changes. Okay! 👍

But … I'm concerned about disabling SDC's validator, because AFAICT it can result in SDCs failing to render, triggering PHP errors. That would make #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) critical based on @lauriii's statements in that issue.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: Unassigned » wim leers
Status: Needs work » Needs review

Assigning back to @wim leers , re #10 I think this is unrelated issue and pushed up a change that removes a need for the changes to `XBPreviewRenderer` see https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

I think the rest of @wim leers comments on the MR are about changes to the files that are no longer needed

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

  • lauriii committed bdb6bae9 on 0.x authored by tedbow 
    Issue #3489994 by tedbow, larowlan, wim leers: Create an endpoint to...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Fixed

Got a +1 from @larowlan on Slack that this is ready to merge.

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@larowlan and @lauriii thanks for finishing this off!

One thing, I noticed is that we have todo to remove ApiContentUpdateForDemoController which also means in that issue we will probably also remove ApiContentUpdateForDemoControllerTest.

The problem is that ApiContentUpdateForDemoControllerTest covers a lot of the logic that is now in ClientDataToEntityConverter. We have the new ApiPublishAllControllerTest but that doesn't cover all the cases we cover in <code>ApiContentUpdateForDemoControllerTest

for instance the logic around creating 'pointer' => "layout.children[1]", but I am pretty sure there is more.

so we can't just remove ApiContentUpdateForDemoControllerTest when we remove ApiContentUpdateForDemoController or we lose our indirect test coverage for ClientDataToEntityConverter.

So we should probably make a follow-up to create ClientDataToEntityConverterTest and move most of the error test cases from ApiContentUpdateForDemoControllerTest into that leaving ApiContentUpdateForDemoControllerTest to be just a small test to cover the logic in the controller that can be removed when we remove the controller.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3491201: Add missing response to `/xb/api/publish-all` in `openapi.yml`

@traviscarden also made a follow-up #3491201: Add missing response to `/xb/api/publish-all` in `openapi.yml`

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

That follow up could perhaps be https://www.drupal.org/project/experience_builder/issues/3489772

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@larowlan

🧐 We can be explicit here (PNX coding standard has banned the use of empty and isset for some time so this is jarring for me, but feel free to ignore).

Feel free to open an issue to enforce this on this project too, using PHPStan presumably? 🤓

@tedbow: Superb simplification of this issue, along with a thorough explanation — well done! 👏🤩

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Issue tags: -Needs followup

Per

After 0.2.0, we'll work on Workspaces integration. The requirements and approach for that are being discussed in #3475672: Research: Possible backend implementations of auto-save, drafts, and publishing

— point 1 in #3455753: Milestone 0.2.0: Early preview.

Workspaces support is for post-0.2, and the issue for it is #3475672.

So, untagging.

Status: Fixed » Closed (fixed)

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