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

  1. 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').)
  2. Expand XbContentEntityHttpApiTest::testDelete setting 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 that ApiContentControllers::list() (/xb/api/content/…) doesn't include the delete-form operation 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 for Page entities 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.
Command icon 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

wim leers created an issue. See original summary.

lauriii’s picture

Assigned: lauriii » Unassigned
Issue tags: -Needs product manager review

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

lauriii’s picture

Issue tags: +Needs followup

Would be good to create the issues for the XB UI work as well.

anjali rathod’s picture

Assigned: Unassigned » anjali rathod

anjali rathod’s picture

Status: Active » Needs review

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

wim leers’s picture

Status: Needs review » Needs work

Thanks for your work on this, @anjali rathod! 🙏

#6: just add

    'list_builder' => EntityListBuilder::class,

to the Page content entity type 😊

Also reviewed the MR! 🏓

anjali rathod’s picture

Status: Needs work » Needs review
anjali rathod’s picture

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

anjali rathod’s picture

Assigned: anjali rathod » Unassigned

roshni upadhyay made their first commit to this issue’s fork.

anjali rathod’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work
roshni upadhyay’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

Significant parts of feedback have not yet been addressed, and it's still unnecessarily making private APIs public.

anjali rathod’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work
Parent issue: » #3452581: [META] XB Permissions
Related issues: -#3452581: [META] XB Permissions

Failing CI

This was marked Needs review but CI is not passing.

The sole failure is
Drupal\Tests\experience_builder\Functional\XbContentEntityHttpApiTest::testList():

 Array (
-    0 => 'experience_builder__auto_save'
-    1 => 'http_response'
-    2 => 'xb_page_list'
+    0 => 'config:system.site'
+    1 => 'experience_builder__auto_save'
+    2 => 'http_response'
+    3 => 'xb_page:1'
+    4 => 'xb_page:2'
+    5 => 'xb_page:3'
+    6 => 'xb_page_list'
 )

This is a legitimate failure. It's missing something like this in \Drupal\experience_builder\Controller\ApiConfigControllers::list():

    // Ignore the cache tags for individual XB config entities, because this
    // response lists them, so the list cache tag is sufficient and the rest is
    // pointless noise.
    // @see \Drupal\Core\Entity\EntityTypeInterface::getListCacheTags()
    $normalizations_cacheability->setCacheTags(array_filter(
      $normalizations_cacheability->getCacheTags(),
      fn (string $tag): bool => !str_starts_with($tag, 'config:experience_builder.' . $xb_config_entity_type_id),
    ));

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.

penyaskito’s picture

amangrover90’s picture

Assigned: Unassigned » amangrover90

wim leers’s picture

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

wim leers’s picture

Issue tags: +stable blocker

This does not seem like a beta blocker to me?

amangrover90’s picture

Assigned: amangrover90 » Unassigned
Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

Thanks! Much simpler now! 🥳

At minimum, cacheability is missing from the access results (and hence the test coverage).

amangrover90’s picture

@wim-leers I've responded to your comments, can you plz check once?

mglaman’s picture

Status: Needs work » Needs review

Marking needs review so we can discuss if cacheability is needed for a non-cacheable HTTP method

nagwani’s picture

Assigned: Unassigned » penyaskito
penyaskito’s picture

Assigned: penyaskito » amangrover90
Status: Needs review » Needs work
amangrover90’s picture

Assigned: amangrover90 » penyaskito
Status: Needs work » Needs review
penyaskito’s picture

penyaskito’s picture

Assigned: penyaskito » mglaman

I contributed significantly to this, so @mglaman will do another review pass before passing this to Wim.

penyaskito’s picture

Assigned: mglaman » wim leers
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

RTBCing.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: -stable blocker +beta blocker, +Needs reroll
penyaskito’s picture

Assigned: Unassigned » wim leers
Status: Needs work » Needs review
Issue tags: -Needs reroll
wim leers’s picture

Assigned: wim leers » penyaskito
Status: Needs review » Needs work

Was 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 😇

penyaskito’s picture

Assigned: penyaskito » wim leers
Status: Needs work » Reviewed & tested by the community

+1. Added a test.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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