- Issue created by @mglaman
- 🇺🇸United States mglaman WI, USA
Okay, I think this jumps the shark a bit as we have no way to stage configuration changes.
Approach 1: base field on xb_page
- There is a new "hidden"/internal boolean flag on pages called homepage
- Mark as homepage actually toggles a base field, which goes to auto save
- On preSave we see if homepage === TRUE, change system.site and remove the value of homepageproblem: how to prevent marking multiple as homepage in autosave data?
Approach 2: make ApiPublishAllController aware of frontpage concept
- the concept of what would be the new homepage cannot live in the entity data
- needs to be part of staged config changescreate a one-off snowflake concept in AutoSaveManager to track changes for the homepage (system.site.path.front) only
- 🇺🇸United States effulgentsia
I think approach 1 is fine until we add Workspaces integration.
problem: how to prevent marking multiple as homepage in autosave data?
Do we need to? Why not just let the last save win?
- 🇺🇸United States mglaman WI, USA
Why not just let the last save win?
Perfectly acceptable to me!
- 🇺🇸United States mglaman WI, USA
CI is failing, but I'd rather get reviews on the approach before fixing.
- 🇺🇸United States mglaman WI, USA
I need to update the issue summary with changes on the approach from the MR
- 🇺🇸United States mglaman WI, USA
I pushed to the MR with changes to use AutoSaveManager for handling simple config changes. It's not 100% done but enough to review and validate as an approach.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
See review on MR; AFAICT this is blocked on ✨ Auto-save code components Active . Notified @tedbow over at #3500042-37: Auto-save code components → an hour ago.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
✨ Auto-save code components Active is in, and responded on the MR 👍
- 🇫🇮Finland lauriii Finland
But also now that I am thinking about. What happens if you delete the page you set as the home page?
We should not allow deleting the home page. User would have to assign another page as a home page before they could delete it. I'll let you decide if we should implement that here or in another issue.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#16 seems reasonable to consider part of this issue from a UX POV, but I don't think it makes practical sense.
Because it would need to work generically, not just for the
Page
content entity type. Plus, it'd require no longer hardcoding the link in the UI. Wrote up a detailed plan at 📌 Disallow deleting an XB-enabled content entity if it's currently the homepage Active .The UI doesn't currently convey:
- that the currently viewed page is the current homepage
- that the currently viewed page is about to become the current homepage (thanks to auto-saving)
- which page in the list is the current homepage
I also think that the UI strings are quite confusing, which @tedbow also pointed out in the MR.
This is what it all currently looks like:
@mglaman has indicated these aspects are missing from the design.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Also, can we link the designs in the issue summary? 🙏
- 🇫🇮Finland lauriii Finland
We don't have full designs for this flow but attaching a design that shows how the homepage would look like in the navigator / top bar which addresses most of the concerns. The home icon should appear in the review changes panel too. We could open a follow-up to explore if we need to clarify this action in the review changes panel.
Let's focus on the deletion aspect in 📌 Disallow deleting an XB-enabled content entity if it's currently the homepage Active .
- 🇫🇮Finland lauriii Finland
I don't really understand the confirmation panel. What are we trying to warn the user about? I don't remember seeing designs with a confirmation panel. I understand that we have a confirmation panel for deleting a page since it takes place immediately. But since changing home page goes through the review changes I don't see why we need the warning.
Setting a draft page as home page would be fine since it would be published once we have 📌 Create an endpoint to publish all auto-saved entities Active . We need to think about archived content in relation to setting a home page once we implement support for that.
- Status changed to Needs work
4 months ago 4:56pm 10 April 2025 - 🇺🇸United States mglaman WI, USA
This ended up in my sprint and need to upload. But I think one of the remaining blockers was the missing 🏠 icon if a page/entity/whatever in the navigation has a path which matches system.site.path.front?
- 🇺🇸United States mglaman WI, USA
I've recreated the MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/882
- First commit to issue fork.
- 🇺🇸United States mglaman WI, USA
I consider the MR is 90%. It needs review on the approach.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This is not passing tests, unfortunately, so not ready for review.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
FYI: this issue's MR contains a superset of what 🐛 Define what should happen if you delete a page while you are editing it Active 's initial MR (!804) was also trying to achieve. 😅🙈 Unfortunate to see so much waste.
- Issue was unassigned.
- 🇺🇸United States effulgentsia
Since this was last worked on, 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active landed, so now the autosave manager may only save entities (EntityInterface objects). We could expand that to allow simple config, but I'm curious what you all think of the following idea instead...
What if we created a new entity type (neither a content entity nor a config entity, just an EntityInterface object) that can contain one or more config actions? It would have no storage but could be saved to autosave, and its save() method would instead apply the config actions.
Would that be a nice way to achieve the goal of this issue? It would have the added benefit of not having to deal with conflicts if keys in
system.site
other thanpage.front
got changed outside of XB. - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Yeah that works for me, the alternative is to add a new method to the auto save manager ::saveSimpleConfig::/getSimpleConfig which is also fine if the new entity-path is too long
- 🇺🇸United States mglaman WI, USA
Okay, stepping up for Round 3 on this thing. I like #36 vs a config entity wrapping/representing the simple config. Also possibly easier to display change in pending changes.
- Merge request !1217#3503412 Create entity to allow staging config updates → (Merged) created by mglaman
- 🇺🇸United States effulgentsia
Overall I think this MR is really good. My one question/concern is: are we sure we want to limit it to just being able to run a single action? I think allowing it to be a sequence of config actions would be potentially more useful to us in the future, even if the current use-case is just a single action. To make it a sequence of config actions would require decoupling the ID from the action. Which I think would be okay, because load() doesn't need to instantiate an entity that doesn't already exist, does it? Couldn't we instead make load() load from auto-save without making any assumptions about the nature of the ID?
If we want to punt on ^ to a follow-up, in order to not hold up this MR, I'd be okay with that.
- 🇺🇸United States mglaman WI, USA
Yeah that's fair. So instead of action and input as top properties there'd be a new property that is a sequence of those. I will make that change tomorrow.
I'll probably finish implementing the tests first since that'll be faster and refactor the approach for multiple actions.
Right now the way the ID would be constructed assumed a lot of manual concatenation of target and action via the UI possibly
- 🇺🇸United States mglaman WI, USA
I'm about to push changes which allow StagedConfigUpdate to support multiple actions. After I did that, I just thought of a problem: if someone changes the site homepage and also something else in system.site (site name?) how can we reflect that easily in one auto-save. That was actually an issue with the original approach that I wasn't sure how to resolve.
- 🇺🇸United States effulgentsia
if someone changes the site homepage and also something else in system.site (site name?) how can we reflect that easily in one auto-save
I don't understand what the concern is. Can you elaborate?
I like the multiple actions support in this MR, but I'm confused why we're limiting to a single target. I imagined
actions
following the same format as in a recipe, where its direct children are targets and then children of each target is actions on that target. Is there a compelling reason to deviate from how theactions
key works in a recipe?Related to above, do we need to couple the target and ID? Couldn't the ID be anything? For example, for the use case of XB UI setting the homepage, could it be something like
xb_set_homepage
without being coupled tosystem.site
? - 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
wim leers → credited penyaskito → .
- 🇺🇸United States mglaman WI, USA
Related to above, do we need to couple the target and ID? Couldn't the ID be anything? For example, for the use case of XB UI setting the homepage, could it be something like xb_set_homepage without being coupled to system.site?
You're right, it can be arbitrary by the frontend. My concern was having multiple staged config updates that possibly overlap (like 3 stacked site name changes) but that would be a client error if they didn't use the same ID. I'll double the ID from the config target so that we can do arbitrary IDs.
- 🇺🇸United States mglaman WI, USA
Marking for review of the backend specific MR. There are a few open threads that need discussion
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
larowlan → changed the visibility of the branch 3503412-set-page-as-homepage to hidden.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Updating status - there's some actionable things in the MR comments
- 🇺🇸United States mglaman WI, USA
The current MR keeps `target` as a property so that each staged configuration update is targeted to one configuration object, and all actions apply to that configuration object.
It would be a non-breaking data model change to allow targeting multiple configuration objects with the actions.
* If `target` is set: do not allow `target` in any `action`
* If `target` is not set: all `action` must specify their `target`Staged configuration updates live in auto-save only, and there is a compatibility layer possible here if we were to not allow either option and move to all actions specifying their target.
The biggest implication is access checks now must be performed against each action if we went this route.
- 🇺🇸United States tedbow Ithaca, NY, USA
see my MR comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... I will implement this
- 🇺🇸United States effulgentsia
"Needs work" for test failures, but if you're wanting additional reviews ahead of fixing tests, please feel free to set back to NR.
- 🇺🇸United States mglaman WI, USA
Had a discussion w/ tedbow. Keeping NW for now. Idea is to remove staged config update from config POST/GET and make a new auto-save config POST for only staged config update
- 🇺🇸United States tedbow Ithaca, NY, USA
created 🐛 DxRouteConsistencyTest::ignoreDynamicPathPartNames regex does not preserve the number of path arguments Active . though I am not sure that is correct now. I put a fix in for \Drupal\Tests\experience_builder\Unit\DxRouteConsistencyTest::ignoreDynamicPathPartNames in this MR
- 🇺🇸United States mglaman WI, USA
Moving back to review. Backend implementation is all set.
- 🇺🇸United States hooroomoo
I was asked to add the frontend parts to the MR so feel free to assign to me after but i will just start on it today
- 🇺🇸United States mglaman WI, USA
Assigning to @hooroomoo as we're mostly on the front end work now and they are working on it
- 🇫🇮Finland lauriii Finland
Some UX related feedback which I'm fine to get addressed in a follow-up if we want to land this faster:
-
What do you mean it cannot be undone? I don't think we need the confirmation because the change goes through the review changes workflow. - Icon is not vertically aligned to center
- I don't think we should use the page icon here. Could we use the homepage icon here?
We should use the home icon consistently across all places where the page is being rendered.
-
- 🇺🇸United States tedbow Ithaca, NY, USA
I re-ran the gitlabci job on 0.x and it is not getting the same phpstan file as this MR
see https://git.drupalcode.org/project/experience_builder/-/pipelines/550539I opened 🐛 phpstan ci job is failing Active
- 🇺🇸United States tedbow Ithaca, NY, USA
I think this issue is good. Re #66 We need follow-ups but since they are front-end and I don't exactly what what the problems are I don't think could make even much of placeholder issues
@larowlan left to comments https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... but chatted with him and these just suggestions for things to do if we have other simple config updates, which we don't currently have planned.See #65 about the phpstan failure, I don't think we need to fix that here
global-regions-interact.cy.js is failing but passes for me locally but not consistently. I don't see how that could be related
It would be good to get approval of the front-end code on the MR, assigning to @jessebaker, but others could review, assigned in gitlab
- 🇬🇧United Kingdom jessebaker
Fundamentally I think this is all working and the MR is good but there are a few small bits that should be addressed.
Given the relative urgency of this, I think the remaining threads on the MR could be shifted to a follow up if there isn't time to fix them.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- Thanks @tedbow for creating the follow-up 📌 Create StagedConfigUpdateValidationTest for StagedConfigUpdate config entity type Active 🙏
- Thanks @mglaman for adding the missing docs in
docs/config-management.md
— Succinct, and beautifully explains the rationale 😊 - 📌 Add validation constraints to system.site Active would allow us removing one hack from XB now, so tagged that issue. (Thanks @mglaman for linking!)
- @tedbow's
move to own controller to make it clear it acts differently then other auto-save config entities
commit was a great call 👍 - Thanks both of you for the clarifications to the code, and for addressing @lauriii's & @larowlan's remarks.
- I don't understand why we'd require
StagedConfigUpdate::ADMIN_PERMISSION
to be able to set the homepage at all? Is it simply because of pragmatism, because\Drupal\Core\Entity\EntityAccessControlHandler::checkCreateAccess()
is called with zero context?Turns out … that that "contextless" thing turns out to be the reason @mglaman did it this way, per his MR comment, but turns out that there is a way! 😄 Yay for core's super abstracted nature 🤓
Raised this with @lauriii. He didn't realize that/missed that in his manual testing in #63. He agrees what's in HEAD is very confusing for users.
So: implemented a solution 😊👍
Only one concern, not commit-blocking:
- We currently use this only for updating the
system.site
simple config. But theStagedConfigUpdate
infrastructure supports targeting any config entity.system.site
will always exist, because the System module is one of the modules Drupal cannot run without. But if other simple config is targeted and the providing module is uninstalled, or a target config entity is deleted … then the garbage values inAutoSaveManager
will continue to exist!
So: we need another follow-up to expand what\Drupal\experience_builder\AutoSave\AutoSaveManager::onXbConfigEntitySave()
does (in the case of target config entities getting deleted) and we need to implementhook_modules_uninstalled()
to deleteStagedConfigUpdates
targeting uninstalled modules' simple config.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The bold bits of #71 which required changes from me have passed automated tests, and work fine during manual testing 🥳
Then simplified, and now this is IMHO ready to go from a BE POV. Approved.
Favoring progress over perfection, let's handle the identified follow-ups in follow-up issues:
mine (#71)- @jessebaker's (#70)
- @larowlan's (https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...)
Theirs are optimizations/clean-ups, mine is a data integrity issue that cannot yet occur in HEAD 👍
@jessebaker agreed, so … merging as soon as CI (pipeline #100!!!) is green 😊
- 🇬🇧United Kingdom jessebaker
Approved so this can be merged but would like to see a follow up created to address Lee's comment and my concern about performance before this is closed.
(oh I've just seen #72 which was posted as I was writing this!)
-
wim leers →
committed 7f81c657 on 0.x authored by
mglaman →
Issue #3503412 by mglaman, tedbow, hooroomoo, wim leers, larowlan,...
-
wim leers →
committed 7f81c657 on 0.x authored by
mglaman →
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🚀
Time to create those 3 follow-ups (or do some of them in a follow-up MR on this issue, whichever is easiest, @hooroomoo!) — hence instead of .
Thanks all, and especially @mglaman! 🙏😊
- 🇺🇸United States tedbow Ithaca, NY, USA
@wim leers and @jessebaker, thanks for the reviews!
- 🇺🇸United States mglaman WI, USA
😮 it finally landed! first work started 4 February 2025 and now we're here 🥳
Thanks to everyone who landed this, from frontend work to backend refinement.
- 🇺🇸United States hooroomoo
Assigning to myself to work on the frontend followups. I plan to open a separate MR here.
A 404 error is appearing in the console for the following endpoint:
/xb/api/v0/config/auto-save/staged_config_update/xb_set_homepage
This failed API request for
xb_set_homepage
is also visible in the network tab.@mglaman @wim-leers
Verified below scenarios and some scenarios getting failed, please look into it :
- 🇺🇸United States tedbow Ithaca, NY, USA
@mayur-rose thanks for the detailed testing report.
I can't reproduce the problem in TC8
"Set as homepage" action is hidden. After page reload option is visible again.
what exactly failed? What I am seeing with 2 pages is, if I set Page 1 as home page, "Set as homepage" action is hidden on Page 1. That is expected. I reload and it is still hidden on Page 1. If I set Page 2 as home page then the actions disappears from Page 2 but appears again from Page 2
To reproduce the three issues mentioned above, please follow these steps:
- Create 3-4 XB pages (for example: homepage, contact us, about us, test page 1).
- Set homepage as the homepage and publish the changes.
- Create a duplicate of the homepage by clicking the three dots next to the page.
- A "Homepage (Copy)" page will be generated. Set "Homepage (Copy)" as the homepage and publish the changes.
- Now, set any other page as the homepage. You will notice a 404 error in the console, as described in the testing report above.
- 🇺🇸United States hooroomoo
Yep TC8 is expected behavior. We hide the "Set as homepage" option if the page is already set as the homepage. Although it should not appear again after a reload which I don't see myself.
TC10: If the homepage hasn't been set yet the api returns a 404 and we call it to know which page to render the home icon for. I'm gonna try to find a way to work around this in my FE follow-up MR.
TC11: Hm I'm not able to reproduce this. What do you mean by showing incorrectly?
- 🇺🇸United States hooroomoo
Thanks @mayur-sose. Good catch. It is because the frontend is checking the auto-save endpoint of staged_config_update for the homepage and after we publish the page, the auto-save endpoint doesn't have that anymore.
- Merge request !1331[#3503412] Ensure drupalSettings is updated with current homepage and UI improvements → (Merged) created by hooroomoo
@wim-leers I have verified the previously failing scenarios, and they are now working as expected. There are no errors in the console or network tab, and the "set as homepage" functionality is functioning correctly.
-
hooroomoo →
committed f7d1ecfd on 1.x
Issue #3503412 by mglaman, tedbow, hooroomoo, wim leers, larowlan,...
-
hooroomoo →
committed f7d1ecfd on 1.x
-
balintbrews →
committed 3cf66d34 on 0.x authored by
hooroomoo →
Issue #3503412 by mglaman, tedbow, hooroomoo, wim leers, larowlan,...
-
balintbrews →
committed 3cf66d34 on 0.x authored by
hooroomoo →
- 🇺🇸United States hooroomoo
hooroomoo → changed the visibility of the branch 3503412-fe-followup to hidden.
- 🇺🇸United States hooroomoo
doing followup #2 for the frontend code improvement review feedback that didn't get addressed yet here 📌 Allow CMS Author to set site's homepage from navigation Postponed