🇺🇸United States @tedbow

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

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.

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.

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

I have started work on top of [##3478565] for nwo

tedbow → created an issue.

wim leers → credited tedbow → .

tedbow → created an issue.

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

Hit us again #3485937-10: Drag and drop in the preview not working correctly →

I think if had this 📌 Create an E2E test that starts with an empty canvas Active we would have caught this bug

Test manually and it works now

reviewed the code and also looks good

Postponed on ✨ AutoSave Entity Revision POC Active

I think the e2e failure was random🤞🏻

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

tedbow → created an issue.

see MR comments

Looks like this needs @wim leers or @f.mazeikis approval to merge

Looks good

tedbow → created an issue.

needed to merge 0.x but now test are failing

I think this is ready for review.

This is no longer a POC so changing the Priority

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

ok seems like 0.x is passing again?

one thing I don’t like is that the functional tests were failing on this $assert_session->statusCodeEquals(200);
just on gitlab not locally, so could have been some composer package version that wasn’t pick up temporarily or some problem with gitlabci
but because of that assert we only get
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
in autoupdate, in our build tests, we had some utility method to always display the page content because the 500 error doesn’t tell us anything

If you look at this pipeline for the MR found this on https://git.drupalcode.org/project/experience_builder/-/pipelines/331481
you can see composer jobs passing and then phpunit jobs failing in the same way that 0.x was

Created 🐛 Test failing on 0.x Active

Assigned to myself but if anyone has any ideas please jump in

I am going to start by creating MR to get a better error message from this

3) Drupal\Tests\experience_builder\Functional\TranslationTest::testTranslation with data set "asymmetric" (array('tree', 'props'), false)
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.

tedbow → created an issue.

looks like 0.x is now failing since this commit https://git.drupalcode.org/project/experience_builder/-/pipelines?page=1...

I can't get it to fail locally

So setting to needs work to alert anyone who is following this issue and might have ideas. We should probably fix in another issue

It could have been a drupal core change but I have pull the latest changes and can't get it to fail

balintbrews → credited tedbow → .

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

Looks good! Thanks everyone!

I think this good enough considering the number of follow-ups we have. There are number of places we assume "props" but I think handling that in 📌 Define `props` in the context of Block components Active is good enough to get things moving

Clarifying where we currently call things "props" that might not make sense

1. Just to be clear though, I'm not suggesting to actually alter the query. I'm suggesting instead that before the query is executed, that we invoke the code to save auto-saved states into revisions, and then let the original query proceed as normal.

   '
   Yep I got that. I think we would also have to do this on hook_entity_preload() has that appears to not result in a query uses the `entity_query` Instead it uses this $query->addTag($this->entityTypeId . '_load_multiple');.

2. I think we could do first try at this that implements hook_entity_preload(), hook_views_query_alterand hook_query_TAG_alter we know might miss some edge cases where the system query the tables on a lower level but then work from there.

   We could keep of track of the ids of the entities that have auto-save states in the current workspace and if a query might return those entities then when convert the auto-saves to real revisions. We could see how the performance and then make the system better if need be. For example we could determine which bundles currently have auto-save states and inspect queries to see if they filter by bundle and will not return the bundles that auto-saves and then we know we don't need to do any conversions

3. Since the above ideas all have their shortfalls, instead I recommend an on-demand revision creation approach

   (emphasis mine)
   While I see the benefit of the lazy saving approach I don't see how it can replace saving revisions every once in a while.

   For instance if 1 user is working on page in XB for many hours and nobody else is viewing the workspace then we would never make a revision of that page and then the history would be lost. I guess that won't be true if ✨ Implement time travel (undo/redo/revert) of auto-saved states across devices and users Active is done without relying revisions at all(I am not convinced of that, are the any UI mock-ups for going through hundreds or thousands of auto-save states?)

   So the this lazy-saving has to be an add-on functionality to solve the problem of seeing the workspace with the very latest XB changes, it doesn't help keeping a history, at least it can't be relied on for that

UPDATE: my comment below goes on the assumption that we would using hook_entity_query_alter but since @effulgentsia mentioned hasData() we would need to use something like hook_query_alter so maybe this won't be done on the entity query level but the lower level which probably means there would be less edge cases.

Original comment

By `hook_query_entity_query_alter` do you mean `hook_entity_query_alter`?

?

Does that get triggered for the node x when viewing node/X? I test but didn't seem to for me? Does it get triggered for views? I would have thought so but in debugging it didn't seems to(I did see some hits for that hook).

Could we reliably use just entity query altering? Would we have to implement hook_entity_preload and make sure ours runs after `workspaces_entity_preload`?

Would we have to implement `hook_views_query_alter` also? Since workspaces `\Drupal\workspaces\ViewsQueryAlter::alterQuery` seems to have. Otherwise the filters would not work correctly on a View, right?

I like the idea in theory I like the idea but I wonder if we end playing whack-a-mole to have all the places we need lazy load our entities before.

but maybe looking at all the hooks in workspaces.module all the works has already been done for a lot of these problem because Workspaces needs for instance make sure the right revision is used in workspaces_entity_preload, so in that case we would also need to check if there is an auto-save state.

I think the MR needs @f.mazeikis' approval because of the changes to `/config/schema/`

Yep this looks fine

Added a comment in #3455629-33: [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model → because that issue will need to add back some of what is being remove here related to translation.

I am working on 📌 Tighten validation: only allow StaticPropSource in XB fields + PageTemplate, DynamicPropSource in ContentTypeTemplate Active . Since we are removing dynamic properties we didn't need some of the logic and test coverage we added in 📌 Allow Experience Builder fields to support Asymmetric and Symmetric translations Needs review .

Basically since you can't display a field like the node title in the XB field it you can't get the wrong translation of the title. You can still have different component properties per language, they will just all be static props

But this current issue will need some of the logic and test coverage we removed added back. Particulary this https://git.drupalcode.org/project/experience_builder/-/merge_requests/3.... I assuming the same fix will be needed and work but to be sure we will need to update the test coverage we removed in \Drupal\Tests\experience_builder\Functional\TranslationTest in #3481720

The test/use case won't be exactly the same but basically we will need to test if you use the node title in a component prop in the content type template when view a single node in different languages you see the correct translation of the title

We will also need to determine if we will support config translation of content type templates. If so that will be another area to test

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

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

🎉

@wim leers ok!

Addressed the last bits. If tests pass I will merge

tedbow → created an issue.

@wim leers thanks for the review, back to you

tedbow → created an issue.

@Wim Leers the direction looks good to me!

going to review

I don't think I fully understand Rule 1: Users can view others' work "live" -> Scenario 1.2: Viewing someone else's in-progress work

🔵 And I select user #2's in-progress editing session (How?)
🔵 Then I will see the current state of user #2's in-progress work (How does Experience Builder know to start viewing someone else's work instead of starting a new, distinct editing session?)

Given 🟢 Rule 3: Only one user can edit an XB-enabled entity at a time and discussion we have had

I would assume that if

1. I am a user who has permission to use XB for an entity and see others work in progress in XB
2. Another user is using XB for node 1
3. I open XB for node 1

Then there would be no choice if I wanted to see the other user's progress in XB for node1 or not, I would automatically see their current work and when their auto-save happened my version would be updated.

To me it would make no sense for me start a "a new, distinct editing session" otherwise we have to figure out to merge the 2 sessions.

We have to work out "🟢 Rule 3: Auto-save states can be transferred" but think that would be where I would able take over the XB session for node 1 from the other user. I think after the auto-save state was transferred to me and I wanted a ""a new, distinct editing session"" I could either 1) clear the canvas or 2) revert to the last revision. But after the auto-save state is transferred to me then it would be no different that if I was the sole user editing XB node 1 and want to "start over".

I agree with #58

I think to make safer to not implement the reverting to auto-saves now but make it possible later we should be very clear that our auto-save implementation is entirely internal. We should be free to change it later no have to worry about BC. Especially before 1.0 and if we still haven't done ✨ Implement time travel (undo/redo/revert) of auto-saved states across devices and users Active by 1.0 we should continue to keep it internal so are free to support reverting later.

I agree that #3481006 would be a lot of work and it doesn't seem realistic to do it with all the product requirements we are trying to get done

@lauriii re

We could do something similar to: https://www.youtube.com/watch?v=CsRBypaT_hM&t=27s. We could apply this both for the scenarios where the user is the same and where it's different.

to be clear, since there is a lot going in that video, the only part you are talking about is requesting hand off control, of the XB edit, correct?

wim leers → credited tedbow → .

FieldTypeUninstallValidatorTest was failing it tests we can uninstall the link module after we remove all uses in XB fields. Now that link is a dependency of XB we get an error after remove all the links uses because it tries to also uninstall XB but its fields are in use on bundles.

Fix the test to make sure we don't get "link" at ll in the uninstall error message at after we have removed all the uses of the link field

there is merge error now in one of the .js tests so assigning back to @bnjmnm

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

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

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.

Created the follow-up 🐛 FieldTypeUninstallValidator's error message needs to include the entity type and bundle of the default value field Active

Postoning on 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active since that might change on things are stored. This issue is still minor Priority. I think it got less important since we have covered 2 of the more likely cases in the tests that were already merged

tedbow → created an issue.

Just merged 📌 Remove dead `ApiResponseValidator::performXbValidation()` Active we may have to add this back here if we need it

yep, this find we can add it back exactly in the other issue if we need it

#19 @catch

If we store XB-entered data in Field Union, then XB will be writing that data to field union field values. This means that the field union data is field data, same as any other field (except the extra stuff it adds on top).

So this is different from what is proposed in the summary of this issue, correct?

In the summary there is static_values with the example

{
  "prop2": "Hello, world!"
}

But in what you wrote in #19 it seems like this would be written to the field_union field values instead. Otherwise you wouldn't get the benefit of using it in manage display or views.

@deepakkm can you make the follow-up issue I asked for here https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

After that re #28 let's mark this issue postponed on 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active because that is might change field storage, and probably make sense to postpone writing the remaining test case till after that is decided.

Since the remaining case is a pretty rare case to be practical lets at least postpone it on 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active because this might stop us from having to adapt the test case after we have made it. Can just make it 1 time, if we determine it is still needed, after things are more settled.

🎉Thanks @abhisekmazumdar and @wim leers

tedbow → created an issue.

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

I talked with @lauriii, @wim leers, @traviscarden and @effulgentsia about the Workspaces requirement.

I brought up my idea from #19 again of making a recipe that would require Workspaces to provide UX we want with full site preview but not making Workspaces a depdency of XB itself.

Pretty much everyone in that list of folks was concerned about ever complexity it would take to support both that case where Workspaces was enable and when it was, which is a valid concern.

@lauriii suggested that we could make Workspaces a dependency of XB now and if it is determined later making Workspace a dependency of XB is big barrier for many sites using it we could change this decision before it is stable.

This seems like a reasonable idea to me. I still have my concerns about making Workspaces a dependency but

1. I have not built sites in a while so probably not the best person to judge how much of barrier it would be. I still think my points above are valid but it probably it is up to others who like @johnwebdev and others who are currently making site to express concern if they have any with having enable Workspaces . I can try to help the discussion with backend implications if they come up.
2. We are just at 0.1.0-alpha1 for XB so we have time to change this decision
3. Workspaces just became stable so it has a lot of time for improvement before we use it in a stable XB release which is many months away. This is bit of gamble, relying a recently stable module that has not been used by a lot of production sites but @lauriii said at Drupalcon Barcelona there was talk of being required for Drupal CMS would require Workspaces to have UX improvements that XB would also benefit from it.

If we do decide to depend on Workspaces I think it would great make it a dependency as soon as possible to make sure it the direction is as clear as possible. If we only make it dependency after we have spent months to make integration really nice that could shorten that time people to raise concerns before XB becomes stable. I assume the people how are following this issue or would be somehow "in the loop" to know are plans will be a very limited number of people.

If we intend that you should only use XB inside a workspaces then could simply:

1. add the dependency
2. add a check on our current demo xb/node/1 to see if they are in a Workspace.
3. If not in workspace and only 1 workspace exists automatically switch them to that workspace, give them a message why they were switched
4. If not in workspace and more workspaces exist redirect them to the workspaces listing page, give them message they have to they have switch to a workspace
5. If they don't have permission to switch to workspace, give them a message they currently demo doesn't support using the XB Demo outside of workspace and they don't have the permission

It doesn't have to be exactly like this but it would good to have follow-up like "Ensure XB can only be used in Workspace" ASAP if we make this decision that will force us to start using it. My guess is providing the optimal UX for Workspaces + XB will take a lot of work in XB, Workspace, and possibly Workspaces Extras. It would great if we could be practical with this part of module like we have with other parts, like the Canvas UI demo that doesn't allow saving, to get working with Workspaces in even rough way first. Though we shouldn't stop saving if you are in workspace.

Another concern I want to raise about relying on Workspaces is that if want to also be able to edit and stage Config changes as @catch mentioned in #31 we need to rely on Workspaces Extras → . While amateescu, the Workspaces core maintainer, is also maintainer of this and it is under active development which are really good signs, it doesn't actually have a published release and as far as I can tell only has 2 tests(functional only). If config support is must have we will probably need to factor in work on WSE or at least get a confirmation on the modules state.

longwave → credited tedbow → .

@longwave thanks for the write up!

We should be able to use Autosave Form to handle automatically saving XB data into tempstore.

Right now the XB ui is not a Drupal Form so we would have to make it Drupal Form to take advantage of AutoSave.

Right now the front is doesn't use the Drupal form except for the side bar but not the complete tree and model values. In fact when in the UI and the sidebar is not open there is no <form> on the page at all.

From the discussions with @wim.leers it sounds like we pretty much have all we need in data currently passed from the client for the preview route to perform a tempstore and/or entity save, at least for the XB field.
I was just wondering if it would make more sense for the JS client to save/auto-save via an API call.
We may get some functionality for free if we use a drupal form but also there would probably be some work to get there instead of js api call from the react app

Just initial thoughts, but my worry with the front-end becoming too “drupal aware” is that we start limiting the people who can work on it. Or you start to need a combination of React Skills and Drupal knowledge(probably not novice either)
You will already need modern JS skills we can’t go back on that(and probably a good thing) but my guess is right now you don’t need a lot of drupal skills to work on the Front-end app.

Also have thoughts on Workspaces but will put that in another comment...

Assigning to Wim because I can't actually to the merge without another gitlab approver from CODEOWNERS.txt

Looks good to me!

Now that 📌 Expand test coverage in FieldUninstallValidatorTest Needs work is merged we need to

1. Take the version of the test from 0.x
2. Remove the duplicate reasons from the array
3. Remove the todo pointing to this issue and add the new assert to ensure we don't get duplicate reasons

This looks good to me. Will merge if tests pass

@deepakkm one more follow-up needs to be made, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

I also made another one you can work on also https://www.drupal.org/project/experience_builder/issues/3476891 🐛 FieldTypeUninstallValidator lists the same content revision 2x if it the default revision Active

I think this looks good. But want to review it with fresh eyes tomorrow

For some reason gitlab gets a 404 when linking to my merge request I just created. This can still be worked on while that gets figured out.

I pushed fix but the test will need to be updated.

tedbow → created an issue.

I would like to suggest that we do something like @effulgentsia's proposal in #17 but replacing Workspaces with Content Moderation.

Copy/paste from #17 with replacements
So here's a possible idea for mitigating the above by combining Content Moderation and Temp store:

• Every auto-save submission from the client we save to the temp store.
• We create a new 'auto-save'. state to workflows used on Content Entity types that XB(this could be tricky for existing sites)
• If while in XB, the editor is working, then when we get an auto-save submission from the client, in addition to saving to the temp store, we also save the entity for real, if:
  • It passes validation.
  • We're not already in the process of saving it in a different request (i.e., we add a lock around a real entity save)

If these are true then unless want to make Content Moderation a hard dependency of XB then

• If the site has Content Moderation installed and configure this entity type, We save to the new auto-save state
• If the site does not have Content Moderation installed or it is not configured for this entity type, but the entity type does have a published status and it is not currently published then we save a revision, still as unpublished.

In both cases we would need to decide whether we always want to create a new revision or want to more careful about keeping many revisions. Beside DB size, one reason to not to keep all auto-save revisions is the user might want to revert back to a previous state later, if we have an auto-save revision for every 30 seconds it could overwhelming to find a point to revert to.

If want to minimize the number of revisions we could save locally the revision id of the last auto-save and not create a new revision if most recent revision matches.

My reasoning is:

1. I understand that Workspaces can solve a real problem as @catch describes in #12 and @lauriii's comments #15
   

   I agree with @catch that Workspaces from UX perspective would likely to be closer to the ideal experience (in the long run).

   I think we should at least consider this as an option since we believe it could provide the best experience.

   I agree if implemented very well, the integration could provide the best experience and that XB should be very opinionated about the best UX.

   My concern is another goal of XB is to unify the page building ecosystem in Drupal. I know creating UX that is better than anything currently in Drupal will be part of how we unify, but I think if we make it too difficult for existing users to use XB or XB requires a too specific way of building sites then we risk the page building ecosystem remaining very fractured.

   @johnwebdev in #20 is an example of someone who hasn't seen the problem Workspaces solves (over Content Moderation) as an actual problem that needs to be solved for their sites. I think on the other end if someone does have the problem of entities leaking into the live site or wanting to publish a bunch of content entities in one batch will they for sure want to solve it in the way that Workspaces provides. There numerous contrib modules that handle content staging between actual Drupal environments. Do we want to try to lure current users who are using these solutions, which they may be heavily invested in, to use XB on their current or next site? Will it be harder if they have to also use Workspaces for XB to have a decent UX(including auto-save)?

   I know @effulgentsia proposal in #17 would not require Workspaces but I think it effectively requires it because a very basically functionality, auto-save, that users would expect to just work requires it.

2. The solution of using Content Moderation in this way would not rule out it working with Workspaces too for sites that need/want the multi-entity staging that it provides. We could adapt the case above where Content Moderation is not installed to always save an entity revision and not care about Publish state when within a Workspace.
3. We could make Drupal CMS offer an XB focused recipe that is even more opinionated than XB itself. In it we could require Workspaces(maybe through a "XB Full site preview" module using something like suggestion in #17). Other sites could also use this recipe. We could even make this promoted/recommended way of using XB.

   This would allow sites to not use the recipe if

   • The don't have the need for the Full Site preview
   • They are already using Workspaces and our prefered way with XB conflicts somehow with how they implemented it(including possible custom code)
   • They are already solving the multi-entity staging problem in another way such as using a dedicated staging environments and a contrib module such as Content Synchronization →

   But again we would recommend the recipe that Drupal CMS would also use as the default way to use XB.