Overview
Currently XB is only enabled for the Page content entity type and the article bundle of the Node content entity type. #3498525: [META] Allow Canvas to be used on any content entity type (bundle), as long as it has certain entity type characteristics will generalize that.
Hence we need a generic solution to what @lauriii described at #3503412-16: Allow Content Author to set site's homepage from navigator:
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.
Proposed resolution
- Implement
hook_entity_access()to disallow deletion of the given entity if it's currently the homepage (either as its canonical entity route or as its alias).
(Note that this automatically causes the Drupal admin UI to respect this everywhere, thanks to\Drupal\Core\Entity\EntityListBuilder::getDefaultOperations()checking$entity->access('delete').) - Expand
XbContentEntityHttpApiTest::testDeletesetting up a new page, marking it as homepage, and verifying that the user don't see the delete-form operation and cannot delete it. After granting the 'bypass node access' permission, the operation should appear and it could be deleted.
IOW: verify thatApiContentControllers::list()(/xb/api/content/…) doesn't include thedelete-formoperation for the homepage, this allows the client to see the result of the "entity operations" hook, aka the result of\Drupal\experience_builder\Controller\ApiContentControllers::getEntityOperations()and\Drupal\Core\Entity\EntityListBuilderInterface::getOperations()..
⚠️ Users with elevated permissions would still be able to bypass this for some content entity types, such as Node:
// Only bypass if not a revision operation, to retain compatibility.
if ($account->hasPermission('bypass node access') && !isset(static::REVISION_OPERATION_MAP[$operation])) {
$result = AccessResult::allowed()->cachePerPermissions();
return $return_as_object ? $result : $result->isAllowed();
}
Out of scope: updating the React UI to only show available operations.
User interface changes
- The
Delete page
operation in the XB UI forPageentities will not appear if the current page is the homepage - The
Delete
operation will also not appear in Drupal admin UIs for relevant content entities.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | Screenshot 2025-03-21 at 12.47.08 PM.png | 527.02 KB | anjali rathod |
| #9 | Screenshot 2025-03-21 at 12.47.00 PM.png | 529.78 KB | anjali rathod |
Issue fork experience_builder-3506434
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:
Comments
Comment #2
lauriiiThis looks like a solid plan. We can do this in XB first but this is probably something core should handle for us. Ideally, even the "bypass node access" wouldn't be able to bypass this but we can leave that to the core issue.
Comment #3
lauriiiWould be good to create the issues for the XB UI work as well.
Comment #4
anjali rathodComment #6
anjali rathodSince there is no entitylistbuilder implementation for xb page content type, I went ahead with the approach I could think of. @wim leers please let me know if this is okay or should I create a entitylistbuilder and make the changes accordingly?
Thank you!
Comment #7
wim leersThanks for your work on this, @anjali rathod! 🙏
#6: just add
to the
Pagecontent entity type 😊Also reviewed the MR! 🏓
Comment #8
anjali rathodComment #9
anjali rathodAdding screenshots for how the navigation menu for a page set as homepage looks and how the navigation menu for a page not set as homepage looks like..
I have removed the set as homepage link along with the delete page link from navigation menu for the page set as homepage. For other pages there is no changes.
Comment #10
anjali rathodComment #12
penyaskitoReviewed. To note, some parts of this were expected to happen in #3516641: Make the XB UI show only UI elements/operations available to the current user and #3516657: Update `ApiContentControllers::list()` to expose available content entity operations in `meta` Tempted to postpone on those.
Comment #13
anjali rathodComment #14
wim leersComment #15
roshni upadhyay commentedComment #16
wim leersSignificant parts of feedback have not yet been addressed, and it's still unnecessarily making private APIs public.
Comment #17
anjali rathodComment #18
wim leersFailing CI
This was marked but CI is not passing.
The sole failure is
Drupal\Tests\experience_builder\Functional\XbContentEntityHttpApiTest::testList():This is a legitimate failure. It's missing something like this in
\Drupal\experience_builder\Controller\ApiConfigControllers::list():Review
This is still using the approach I pushed back against >1 months ago. My feedback from April 10 has not been addressed.
Independently of me, @penyaskito posted similar observations in #12.
Comment #19
penyaskitoUpdated proposed resolution now that #3516657: Update `ApiContentControllers::list()` to expose available content entity operations in `meta` landed.
Comment #20
amangrover90 commentedComment #22
wim leers#19++ — thanks, @penyaskito! 🙏
Closed the existing MR 768 because per #19: a lot is different now :)
(Plus, keeping commit history since March around is going to make things unnecessarily painful. Just copy over whatever bits are relevant. Credited @anjali rathod for their many commits to that MR.)
Comment #23
wim leersThis does not seem like a beta blocker to me?
Comment #25
amangrover90 commentedComment #26
wim leersThanks! Much simpler now! 🥳
At minimum, cacheability is missing from the access results (and hence the test coverage).
Comment #27
amangrover90 commented@wim-leers I've responded to your comments, can you plz check once?
Comment #28
mglamanMarking needs review so we can discuss if cacheability is needed for a non-cacheable HTTP method
Comment #29
nagwani commentedComment #30
penyaskitoComment #31
amangrover90 commentedComment #32
penyaskitoRemoving needs follow-up, #3516641: Make the XB UI show only UI elements/operations available to the current user exists already.
Comment #33
penyaskitoI contributed significantly to this, so @mglaman will do another review pass before passing this to Wim.
Comment #34
penyaskitoComment #35
penyaskitoRTBCing.
Comment #36
wim leers#3532490: Add tests to ensure duplicated pages don't retain path alias of original entity landed and conflicts with this. 🫣
Comment #37
penyaskitoComment #38
wim leersWas going to merge, then spotted one last bug: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... 😅
I’m okay with not adding test coverage, if you apply the suggestion and agree with that proposal 😇
Comment #39
penyaskito+1. Added a test.
Comment #41
wim leers