Allow CMS Author to set site's homepage from navigation

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 homepage

problem: 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 changes

create a one-off snowflake concept in AutoSaveManager to track changes for the homepage (system.site.path.front) only

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?

Why not just let the last save win?

Perfectly acceptable to me!

CI is failing, but I'd rather get reviews on the approach before fixing.

🏓 https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...

I need to update the issue summary with changes on the approach from the MR

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.

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.

✨ Auto-save code components Active is in, and responded on the MR 👍

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.

wim leers → credited tedbow → .

#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.

To clarify: I defer to @lauriii where to draw the line, but if not everything in #19 is addressed here, there will definitely need to be one or more follow-ups.

Also, can we link the designs in the issue summary? 🙏

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 .

#22: that doesn't address .

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.

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?

mglaman → changed the visibility of the branch 0.x to hidden.

I've recreated the MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/882

mglaman → changed the visibility of the branch 3503412-allow-cms-author to hidden.

mglaman → closed merge request !614

I consider the MR is 90%. It needs review on the approach.

This is not passing tests, unfortunately, so not ready for review.

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.

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 than page.front got changed outside of XB.

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

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.

mglaman → changed the visibility of the branch 3503412-set-page-as-homepage-simplify to hidden.

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.

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

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.

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 the actions 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 to system.site?

wim leers → credited penyaskito → .

Nice progress here!

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.

Marking for review of the backend specific MR. There are a few open threads that need discussion

larowlan → changed the visibility of the branch 3503412-set-page-as-homepage to hidden.

Updating status - there's some actionable things in the MR comments

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.

see my MR comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... I will implement this

I think this is good for review again!

"Needs work" for test failures, but if you're wanting additional reviews ahead of fixing tests, please feel free to set back to NR.

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

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

Moving back to review. Backend implementation is all set.

See MR comments

MR feedback addressed

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

Assigning to @hooroomoo as we're mostly on the front end work now and they are working on it

Some UX related feedback which I'm fine to get addressed in a follow-up if we want to land this faster:

1. 
   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.
2. Icon is not vertically aligned to center
3. I don't think we should use the page icon here. Could we use the homepage icon here?
4. 
   
   

   We should use the home icon consistently across all places where the page is being rendered.

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/550539

I opened 🐛 phpstan ci job is failing Active

I fixed #63 1 and 3

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

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.

• 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:

1. We currently use this only for updating the system.site simple config. But the StagedConfigUpdate 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 in AutoSaveManager 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 implement hook_modules_uninstalled() to delete StagedConfigUpdates targeting uninstalled modules' simple config.

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 😊

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!)

🚀

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! 🙏😊

@wim leers and @jessebaker, thanks for the reviews!

😮 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.

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 :

@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:

1. Create 3-4 XB pages (for example: homepage, contact us, about us, test page 1).
2. Set homepage as the homepage and publish the changes.
3. Create a duplicate of the homepage by clicking the three dots next to the page.
4. A "Homepage (Copy)" page will be generated. Set "Homepage (Copy)" as the homepage and publish the changes.
5. Now, set any other page as the homepage. You will notice a 404 error in the console, as described in the testing report above.

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?

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.

@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 → changed the visibility of the branch 3503412-fe-followup to hidden.

mglaman → closed merge request !882

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