Problem/Motivation

I want to be able to create a role in Drupal that doesn't have the permission to bypass node access, but can create, edit, and delete content (including unpublished content) and be able to add menu links to them.

At the moment a user MUST have the bypass node access permission to be able to create a menu link as a child of an unpublished node via the node form

Steps to reproduce

Login as an editor without the bypass node access permission but ability to create menu links
Create a parent page leaving in draft
Create a child page and notice in the menu settings you can't select the parent.

Proposed resolution

Only add the query condition on node status from DefaultMenuLinkTreeManipulators::checkNodeAccess if the user doesn't have the 'view any unpublished content' permission or the project has no node_grants implementations. The query still checks access so should still filter the list for nodes that the user doesn't have access to.

This could be considered a bug as I find it odd that a user is able to create, edit, and delete content and menus/menu items but can't set a parent to an unpublished node that even they created.

CommentFileSizeAuthor
#107 2807629-107-d9.patch8.23 KBMirjam Dissel
#105 2807629-105.patch8.2 KBsraLton
#104 2807629-89-d9.patch11.06 KBdouggreen
#96 Screenshot from 2023-02-07 07-58-29.png59.44 KBlarowlan
#96 Screenshot from 2023-02-07 07-58-45.png81.5 KBlarowlan
#89 2807629-89.patch10.98 KBidebr
#89 2807629-89-test-only.patch5.25 KBidebr
#89 interdiff-87-89.txt2 KBidebr
#87 2807629-87.patch12.29 KBidebr
#87 2807629-87-test-only.patch5.31 KBidebr
#87 interdiff-84-87.txt3.77 KBidebr
#84 2807629-84.patch8.52 KBidebr
#84 2807629-84-test-only.patch2.79 KBidebr
#84 interdiff-81-84.txt727 bytesidebr
#82 2807629-82-rerolled-for-9.5.x.patch8.25 KBidebr
#81 2807629-81.patch8.16 KBidebr
#81 2807629-81-test-only.patch4.97 KBidebr
#81 interdiff-74-81.txt12.15 KBidebr
#75 interdiff_45-75.txt1.78 KBszloredan
#75 2807629-75.patch5.8 KBszloredan
#74 interdiff_73-74.txt971 bytespooja saraah
#74 2807629-74.patch4.72 KBpooja saraah
#73 reroll_diff_57-73.txt2.88 KBravi.shankar
#73 2807629-73.patch4.73 KBravi.shankar
#57 interdiff_56-57.txt1.65 KBranjith_kumar_k_u
#57 2807629-57.patch4.92 KBranjith_kumar_k_u
#56 2807629-56.patch4.95 KBthomas.crouch
#45 interdiff-2807629-39-45.txt3.22 KBacbramley
#45 2807629-45.patch6.33 KBacbramley
#39 interdiff_36-39.txt745 bytesnikitagupta
#39 2807629-39.patch3.39 KBnikitagupta
#36 2807629-36.patch3.39 KBKapilV
#35 interdiff_34_35.txt1.23 KBKapilV
#35 2807629-35.patch3.37 KBKapilV
#34 2807629-34.patch2.63 KBseanB
#33 interdiff_18_33.txt1.79 KBkalvis
#33 2807629-33.patch2.7 KBkalvis
#23 interdiff_21-23.txt1.14 KBravi.shankar
#23 2807629-23.patch2.65 KBravi.shankar
#21 drupal-n2807629-21.patch2.63 KBDamienMcKenna
#18 2807629-18.patch2.61 KBidebr
#18 2807629-18-test-only.patch1.81 KBidebr
#18 interdiff-16-18.txt553 bytesidebr
#16 interdiff-7-16.txt1.1 KBidebr
#16 2807629-16.patch2.38 KBidebr
#16 2807629-16-test-only.patch1.81 KBidebr
#7 drupal-core-allow-menu-items-to-link-to-unpublished-2807629-7-8.3.x.patch1.27 KBgalooph
#4 drupal-core-allow-menu-items-to-link-to-unpublished-2807629-4-8.3.x.patch1.26 KBaditya.n
#2 drupal-core-allow-menu-items-to-link-to-unpublished-2807629-2-8.3.x.patch564 bytesaditya.n

Issue fork drupal-2807629

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

aditya.n’s picture

@acbramley, hi, I followed your instructions, and regenerated the issue. Yes I believe, this might be a bug and the user might feel limited with the permissions provided. I went through with your provided resolution, and am adding a patch for the same. After applying this patch, everything seems to work fine, and the user is able to add menu links as a child to the unpublished node.

Still the API and documentation changes are required Here, if this patch passes the tests.

Status: Needs review » Needs work
aditya.n’s picture

Patch with revisions.

acbramley’s picture

@androiditya thanks for that! I'll be interested to see what the response on this is as it does seem odd to set a hard condition on the published status of the node when the query has access checking anyway.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

galooph’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

Issue tags: +Needs tests

Let's add test coverage to show the intended behavior

idebr’s picture

Status: Needs review » Needs work

Setting to 'Needs work' per #10

robpowell’s picture

@idebr I am trying to determine the best way to write a phpunit test to cover the expected method result. Here is the test that was removed.

+++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
@@ -294,9 +294,6 @@ public function testCheckNodeAccess() {
-    $query->expects($this->at(1))
-      ->method('condition')
-      ->with('status', NodeInterface::PUBLISHED);

And the error message:

1) Drupal\Tests\Core\Menu\DefaultMenuLinkTreeManipulatorsTest::testCheckNodeAccess
Expectation failed for method name is equal to when invoked at sequence index 1.
Mocked method does not exist.

This makes sense to me as we removed the code the would make the query add that condition. However, I don't understand what we can add in that test's place. Can you expand what you would expect?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

idebr’s picture

The last submitted patch, 16: 2807629-16-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

idebr’s picture

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -148,7 +148,6 @@ public function checkNodeAccess(array $tree) {
-        $query->condition('status', NodeInterface::PUBLISHED);

The NodeInterface use statement is now unused, so it can be removed.

Lendude’s picture

Status: Needs review » Needs work

So we now have test coverage that we can add a menu link to an unpublished node we have access to, but I don't see any test coverage for this statement in the IS:

The query still checks access so should still filter the list for nodes that the user doesn't have access to.

Pretty sure that it's true, but looking though \Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMenuNodeFormWidget I don't see any explicit coverage for this. There is some coverage for inaccessible menu's, but not for inaccessible nodes as far as I can see.

And bug vs feature...hmm sounds buggy to me, but since there is a specific line of code that explicitly adds this (but has no functional test coverage ¯\_(ツ)_/¯ ), it's probably a feature to remove it.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

DamienMcKenna’s picture

Rerolled against the latest 8.9.x, and it applies cleanly to 9.0.x and 9.1.x.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
FileSize
2.65 KB
1.14 KB

Here I have fixed failed test of patch #21.

anmolgoyal74’s picture

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Needs work

Still needs additional test coverage per #19

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

thatguy’s picture

Patch in #23 applies correctly in 9.0.7 and fixes the issue

pameeela credited b_sharpe.

pameeela credited larowlan.

pameeela’s picture

amateescu’s picture

Issue tags: +Needs tests
kalvis’s picture

The patch in #18 can't be applied to Drupal 8.9.10. Here's an updated version based on #18 and latest 8.9.x branch.

seanB’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

Rerolled for 9.1.

KapilV’s picture

Fixed Custom Commands Failed.

KapilV’s picture

Status: Needs review » Needs work

The last submitted patch, 36: 2807629-36.patch, failed testing. View results

ravi.shankar’s picture

@KapilV can you please add interdiff?

nikitagupta’s picture

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nicrodgers’s picture

Status: Needs review » Needs work

Patch no longer applies - needs a re-roll

pookmish made their first commit to this issue’s fork.

pookmish’s picture

Status: Needs work » Needs review

I re-rolled and did a PR instead of a patch for easier updating if needed.

I was successful on this update working on the latest 9.2.x and 9.3.x versions.

acbramley’s picture

Adds test coverage as per #19

Bohus Ulrych’s picture

Thank you @acbramley
patch #45 works with Drupal 9.2.1

mpp’s picture

Status: Needs review » Reviewed & tested by the community

#45 works on drupal/core (9.2.4).

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

Spokje’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC per #47 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

Spokje’s picture

Status: Needs work » Reviewed & tested by the community
cpierce42’s picture

I pulled down patch 45 and this fixes the issue for me.

cpierce42’s picture

Status: Reviewed & tested by the community » Needs work

After further testing this patch does fix the issue for me but creates another.
Draft content links do appear in menu for authenticated users who have permissions to see draft content.
Problem: Draft content links now show in menus for unauthenticated users.

thomas.crouch’s picture

Status: Needs work » Needs review
FileSize
4.95 KB

I've rerolled the patch and made it so users that are not logged in cannot view unpublished menu items, but logged in users with permissions can.

ranjith_kumar_k_u’s picture

Fixed CS errors.

Status: Needs review » Needs work

The last submitted patch, 57: 2807629-57.patch, failed testing. View results

acbramley’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC and please use patch #45. The permission check in #56 is not needed. If users can see unpublished nodes it's because they have access to them. This is proven by the tests added i #45 as the query is running through node access hooks.

If there is a bug, please post a failing test patch or provide steps to reproduce from a clean install.

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

The last submitted patch, 45: 2807629-45.patch, failed testing. View results

larowlan’s picture

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -149,7 +148,6 @@ public function checkNodeAccess(array $tree) {
         $access_result->addCacheContexts(['user.node_grants:view']);
-        $query->condition('status', NodeInterface::PUBLISHED);

I feel like this will leak into the anonymous user if you don't have a module that implements hook_node_grants

larowlan’s picture

Category: Feature request » Bug report
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work

I think we almost need a lesser permission here, something like 'Reference unpublished nodes' which could also be used in the Node selection handler

There needs to be something between bypass node access and no access to unpublished content in ER fields, menu UI etc.

Also, I think this is a major bug, because it causes data loss as per #3059790: If pages in the menu are unpublished, changes made in the menu UI can be lost when node is saved which was closed in favour of this.

larowlan’s picture

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alienzed’s picture

Any news on this? My site's editorial workflow is severely disrupted here :S

GaëlG’s picture

Issue tags: +Needs reroll
ravi.shankar’s picture

Added reroll of patch #57 on Drupal 10.1.x., keeping the status needs work for comment #68.

pooja saraah’s picture

Fixed failed commands on #73
Attached interdiff patch
The comment #68 needs to be addressed

szloredan’s picture

Reroll for 9.5x

pameeela’s picture

_utsavsharma’s picture

Status: Needs work » Needs review
smustgrave’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Updated IS with steps to reproduce.

Tested manually following those steps and can confirm I can now add child nodes to unpublished parents without the bypass node access permission.

The patch contains valid tests.

So +1 for me.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

This introduces a security issue, we're now not limiting access to menu items that point to unpublished nodes for anonymous users

And the test is adding a hook to check specifically for the actual item.

My comment/suggestion at #68 hasn't been addressed either.

Please don't reroll or use patches #73 through #75 otherwise you're introducing a security risk on your site.

larowlan’s picture

Sorry, the issue is with patch #75 only, #73 and #74 have an extra check.

idebr’s picture

#68

I think we almost need a lesser permission here, something like 'Reference unpublished nodes' which could also be used in the Node selection handler

Introducing a new permission does not seem very sensible at this point, since this issue already requires checking for specific permissions for generic use cases, for example #2845144: User can't reference unpublished content even when they have access to it
As a result, the access check can easily result in incorrect access cache metadata: #3278759: Access cacheability is not correct when "view own unpublished content" is in use

As a more general solution, we can rely on either node access or the broad 'view any unpublished content' permission. Note that the 'view own unpublished nodes' permission was explicitly ignored to not require cache entries per user in #2250315: Regression: Bring back node access optimization to menu trees. This requirement is untouched.

#79.1 Added explicit coverage to test unpublished content is not available in the 'Parent link' select list

#79.2 Removed the hook from the test that checked for an actual item.

The patch implements the following changes:

  1. The 'Parent link' select list now relies on node access for projects with node_grants implementations.
  2. The 'Parent link' select list now lists unpublished nodes for projects for projects without node_grants implementations where the user has 'view any unpublished content' permission.
  3. DefaultMenuLinkTreeManipulators::__construct() now requires the ModuleHandler, added a change record at https://www.drupal.org/node/3336973.
  4. Added a deprecation test for the new DefaultMenuLinkTreeManipulators::__construct() $module_handler argument
idebr’s picture

#81 rerolled for 9.5.x

smustgrave’s picture

Status: Needs review » Needs work

Seem to have CI errors in #81.

idebr’s picture

Added a property for $moduleHandler, fixing the PHPstan error.

The last submitted patch, 84: 2807629-84-test-only.patch, failed testing. View results

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update
  1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
    @@ -149,7 +164,9 @@ public function checkNodeAccess(array $tree) {
    +        if (!$this->moduleHandler->hasImplementations('node_grants') && !$this->account->hasPermission('view any unpublished content')) {
    

    So we now have the check for node_grants being used, but we don't have test coverage for this. There should be some test modules around that we could install in the test to cover this I think

  2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiContentModerationTest.php
    @@ -188,4 +189,53 @@ public function testMenuUiWithPendingRevisions() {
    +    $this->assertSession()->optionNotExists('edit-menu-menu-parent', 'main:' . $link->getPluginId());
    

    First I was worried that we only end on negative assertions, but optionNotExists does check if the specified select element exists and fails otherwise, so that is great.

Also the current proposed fix no longer lines up with the proposed fix in the IS, so the IS could use an update

idebr’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
3.77 KB
5.31 KB
12.29 KB

#86.1 Added a test with the node_grants implementation from node_access_test

#86.2 Updated the issue summary proposed solution in line with the patch

Lendude’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiNodeTest.php
    @@ -377,4 +377,57 @@ public function testMultilingualMenuNodeFormWidget() {
    +    ]);
    +    $this->drupalLogin($admin_user_without_content_access);
    +    $this->drupalLogin($admin_user_without_content_access);
    +    $this->assertFalse($node->access('view', $admin_user_without_content_access));
    

    Nitpick: Double login

  2. +++ b/core/modules/node/tests/modules/node_access_test/node_access_test.module
    @@ -15,8 +15,8 @@
    - * @see \Drupal\node\Tests\NodeQueryAlterTest
    - * @see \Drupal\node\Tests\NodeAccessBaseTableTest
    + * @see \Drupal\Tests\node\Functional\NodeQueryAlterTest
    + * @see \Drupal\Tests\node\Functional\NodeAccessBaseTableTest
    
    @@ -44,8 +44,8 @@
    - * @see \Drupal\node\Tests\NodeQueryAlterTest::testNodeQueryAlterOverride()
    - * @see \Drupal\node\Tests\NodeAccessPagerTest
    + * @see \Drupal\Tests\node\Functional\NodeQueryAlterTest::testNodeQueryAlterOverride()
    + * @see \Drupal\Tests\node\Functional\NodeAccessPagerTest
    

    Unrelated changes, if these were the only bad testing namespaces left in core I'd be ok with sneaking it in here, but they are not, so let's not fix them here, but do a new issue to fix this everywhere.
    If I search core with regex "\\Drupal\\(.*)\\Tests\\" I find a whole bunch of these all over core.
    Did a quick look but couldn't find an existing issue for it.

Edit: better regex "\\Drupal\\(\w+)\\Tests\\"

idebr’s picture

#88.1 Removed the duplicate login

#88.2 Removed the unrelated changes and filed a follow-up issue at #3337653: Update outdated class references from Drupal\*\Tests to Drupal\Tests\*

The last submitted patch, 87: 2807629-87-test-only.patch, failed testing. View results

The last submitted patch, 89: 2807629-89-test-only.patch, failed testing. View results

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to me, thanks!

Fernly’s picture

Only patch #82 worked for me (D 9.5.3).
Patches #84, #87 and #89 did not apply.

Edit: Didn't notice that those patches are for D10. All is good.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
@@ -81,12 +89,14 @@ protected function setUp(): void {
+    $container->set('module_handler', $this->moduleHandler);

Can we confirm that this test has existing coverage for the unpublished status filter.

It looks like node 3 in that instance covers the unpublished case.

But I can't see a functional test in core to ensure that an unpublished node link isn't output to the page.

If we confirm we have existing coverage for that I'll be happy we're not accidentally creating an access bypass here.

The fact tests didn't fail on #75 makes me think that we only have this unit test, and it is too tightly bound to the current implementation to fail when we make this behaviour change.

So to be clear, we need to confirm we have a test that renders a menu with a link in it that points to an unpublished node, that confirms the link isn't output to users without access.

Sorry for being pedantic here, but security is something we've always prided ourselves on, and I don't want to be the one to commit something that harms that.

idebr’s picture

Status: Needs review » Reviewed & tested by the community

Existing functional test coverage is available \Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMenuNodeFormWidget(). This test implements a scenario where an editor is able to add/edit an unpublished node, but not view it:

    $admin_user = $this->drupalCreateUser([
      'access administration pages',
      'administer content types',
      'administer nodes',
      'administer menu',
      'create page content',
      'edit any page content',
    ]);
    $this->drupalLogin($admin_user);
    // Assert that the link does not exist if unpublished.
    $edit = [
      'menu[enabled]' => 1,
      'menu[title]' => $node_title,
      'status[value]' => FALSE,
    ];
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->drupalGet('test-page');
    $this->assertSession()->linkNotExists($node_title, 'Found no menu link with the node unpublished');
    // Assert that the link exists if published.
    $edit['status[value]'] = TRUE;
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->drupalGet('test-page');
    $this->assertSession()->linkExists($node_title, 0, 'Found a menu link with the node published');
larowlan’s picture

Queued a 9.5 test

Just to be super cautious, I did some manual testing of this and it works well.

Steps I took:

* Install umami with patch applied
* Modify permissions for Editor role to allow editing menu
* As admin create some pages, mix of published and unpublished and place them in the main menu
* As editor edit a page (cannot create pages in Umami as Editor which feels odd, but is not related), verify you can see the unpublished items as menu parent
* View page as editor, unpublished items show up in the menu
* View page as anonymous, unpublished items do not show up
* Confirm that both pages are still cacheable (for editor with DPC, for anon with page cache and DPC)

In my screenshots, rubbish and recycling is the unpublished item

Screenshots are for anon, and for editor
My screenshot tool won't capture the open select field, but the unpublished item is in the list

Saving issue credits

  • larowlan committed 08177ee3 on 10.1.x
    Issue #2807629 by idebr, pookmish, ravi.shankar, KapilV, aditya.n,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Whilst this is a bug report, we're introducing a new deprecation and changing the parameters to the menu tree manipulators service - so that means it can only be in a new minor.

I've committed this to 10.1.x and published the change notice.

Thanks folks

larowlan’s picture

Oh, and I meant to say thanks to @idebr for doing the work to find the test coverage I was asking about, much appreciated.

idebr’s picture

Nice to have this committed, thanks!

karenann’s picture

Recently updated one of my instances to 9.5.2 and PHP 8.1.14.

I removed patch #42 from my codebase and the problem I was having reappeared. The problem I have is that, on the edit screen, the "Menu Settings > Parent link" dropdown will omit any unpublished items and all of it's children.

I applied #82 and my problem is resolved.

acbramley’s picture

@karenann the fix has not been backported to 9.5, it is only in 10.1.x. You will need the patch until upgrading to 10.1

Status: Fixed » Closed (fixed)

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

douggreen’s picture

FileSize
11.06 KB

Attached is the backport for 9.x.

sraLton’s picture

FileSize
8.2 KB

Here is a patch for Drupal 9.5.10

mahde’s picture

Mirjam Dissel’s picture

FileSize
8.23 KB

I've looked into why the 2807629-105.patch wouldn't apply on D9.5.10 and it was just a small fix in de core.services.yml part.
Here it is again!