AutoSave Entity Revision POC

The current merge requests uses the preview API call same as 📌 Autosave PoC Active

it currently does not work with components with a "name" property and it does not for images.

Now I have images working. It is just a POC so just assumes we are using media entities and the src always matches a file which then matches and a media entity

Changing this issue to do that actual save via "Publish" as that is the first functionality we need to add.

📌 Autosave PoC Active was committed for auto-save to the tempstore

Until 📌 The UI only handles resolved props values, hence complex widgets do not show the current value in props form (e.g. image with Media Library Widget) Needs work we will not know the actual target id for media. I have search based on src but this is not the best solution

I think this is ready for review.

This is no longer a POC so changing the Priority

wim leers → made their first commit to this issue’s fork.

wim leers → credited longwave → .

wim leers → credited traviscarden → .

Very much on the right track, but I think in the current state it'll be hard to evolve.

I left a bunch of suggestions that IMHO make this more approachable, and make it easier to iterate on in the future.

WDYT, @tedbow?

see MR comments

I think the e2e failure was random🤞🏻

Created follow-up 📌 Create exception class for invalid data from client Active

tedbow → credited f.mazeikis → .

A thorough review round 🏓

I think there's some crucial bits that are too rough to get merged here:

1. supports only POST but does not restrict the route to that and appears to support updating an entity. Suggested fixes on MR: rename route + controller to be about saving entity revisions, not entities, then it all makes sense.
2. the validation error response is way too unstructured, even after having incorporated @longwave's feedback. This will make surfacing validation errors on the client side unnecessarily cumbersome. All the right pieces exist already. I pointed to them in my MR comments.
3. The helper methods were constructing various kinds of undocumented arrays of doom. This is implied by your comment at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3.... I pushed a handful of commits that improve that.

There are now phpunit (next major) tests failing. But the fail the same way for me on 0.x locally after I pulled the latest 11.x changes. That last 0.x ci passed so maybe it is something in Core 11.x.. Will rerun 0.x

Sorry for frustrating you, @tedbow 😬 🙈

Given this is introducing a HTTP API route that accepts arbitrary inputs, we IMHO should not defer precise validation error messages for the UI to follow-up issues: this issue IMHO SHOULD provide the necessary infrastructure to enable folks working on the XB UI to work on validation errors at the same time as they work on saving — because they go hand-in-hand. Validation was already handled … but it was lacking the precision needed for the XB UI to provide a good UX.

@tedbow is right that this resulted in some scope creep. But that's only because I tried to accelerate getting this MR to the point where I am comfortable with it landing (thanks to it being sufficiently complete/not having many follow-ups). Ideally, the changes to ValidComponentTreeConstraintValidator would have happened in a blocking issue/MR (commits https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...), but for the sake of speed/minimizing overhead, I am personally fine with the scope creep 😇

In particular these two MR threads:

• https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...
• https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

I think I need to review the changes @wim leers made then I will put back to RTBC

Needs work for the 2 failing tests cases I added, @wim leers maybe you think we shouldn't cover the new cases because we can somewhat trust the client but I thought I would at least point them out.

Setting to needs work to either make the tests pass or remove the test cases.
@wim leers I assigned to the issue to you to review those not necessarily fix those.

I guess it is also needs review for my long comment here https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... 😬
but like I said there maybe I just need to sleep on it to see the wisdom of the changes @wim leers made, I definitely been convinced before 😉. So i guess I am not asking for any changes right now unless @wim leers is blow away by arguments there(which I am not expecting). I just wrote out what I was thinking and will see if I feel the same tomorrow.

@wim leers ultimately you are the tech lead so if are already confident on the changes you made and don't buy my argument that it is more complicated and harder to understand then I have reviewed them I am fine with them technically, so I won't block or hold up commit on those. I would rather get this issue and move on to other issues.

Other than that I think the only outstanding problem is the failing test cases and if those are fixed or removed I think this issue is ready to be committed.

renaming because no longer have the button.

It was in the MR previously but i remove because I kept having merge conflicts under `/ui`

If we want to could add that back.

Oops, wrote my comment in the IS field 🤣🙈

wim leers → credited larowlan → .

Crediting @larowlan for his review on the MR, which I responded to — I agree.

I think this is ready.

but I like the changes we use more of the validation logic we already have.

— @tedbow at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

Whew! 😮‍💨

Per #30, merging! But apparently this needs to pull in origin/0.x first, so did that, eagerly awaiting test results 🤓

Merged!

Next step: reroll #3487773, because this MR moved some code that that MR was going to remove — see #3487773-7: Get component name from components list not from the component's model → . Un-RTBC'd that prioritize this (more complex) MR.

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