Steps to reproduce:

Create an unpublished node.

Go to menu admin

Create a menu link pointing to the node.

Try to find the menu link you just created, good luck.

CommentFileSizeAuthor
#220 administer_menu_items_and_book_outlines_for_unpublished_nodes_like_177-460408-220.patch4.25 KBbyronveale
#219 administer_outlines_for_unpublished_book_nodes-460408-219.patch1.4 KBbyronveale
#204 460408-204.patch7.08 KBDavid_Rothstein
#204 460408-204-TESTS-ONLY.patch2.45 KBDavid_Rothstein
#204 interdiff.txt2.52 KBDavid_Rothstein
#203 460408-203.patch6.9 KBDavid_Rothstein
#203 interdiff.txt5.54 KBDavid_Rothstein
#187 460408-D6-187.patch1.21 KBjoelpittet
#186 460408-D6-185.patch1.2 KBjoelpittet
#179 test-only-fail-460408-177.patch2.26 KBjoelpittet
#177 460408-177.patch2.84 KBaerozeppelin
#177 interdiff-460408-157-177.txt6.54 KBaerozeppelin
#177 test-only-fail-460408-177.patch6.54 KBaerozeppelin
#163 460408-163-unpublished-verbose.png46.2 KBstar-szr
#158 fix.png24.49 KBumar-ahmad
#157 menu-access_unpublished-nodes_460408-157.patch5.28 KBmarkie
#155 menu-access_unpublished_nodes-460408-155.patch1.71 KBmarkie
#153 menu-access_unpublished_nodes-460408-153.patch1.92 KBmarkie
#144 460408-144.patch1.39 KBstar-szr
#141 460408-141.patch2.17 KBmarthinal
#141 interdiff-460408-136-141.txt1.34 KBmarthinal
#136 460408-136.patch2.08 KBmarthinal
#131 460408-131.patch2.05 KBmarthinal
#131 460408-131-only-test.patch1.39 KBmarthinal
#128 460408-128-only-test.patch1.39 KBmarthinal
#128 Screen shot 2014-01-24 at 7.14.56 PM.png291.5 KBmarthinal
#126 460408-126.patch2.06 KBmarthinal
#126 460408-126-only-test.patch1.39 KBmarthinal
#118 460408-118.menu_admin_unpublished_nodes.D7-do-not-test.patch594 bytesdww
#118 460408-118.menu_admin_unpublished_nodes.D8.patch683 bytesdww
#111 menu_admin_unpublished_nodes-460408-111.patch597 bytesdotton
#107 drupal-unpublished-menu-items-d7-460408-107.patch1.45 KBjojonaloha
#106 menu.inc_.patch1.74 KBdavid_garcia
#96 460408-96.menu_admin_unpublished_nodes.patch681 bytesdww
#91 460408-91.menu_admin_unpublished_nodes.patch647 bytesdww
#86 menu_admin_unpublished_nodes-460408.patch610 bytesnerdcore
#64 460408-unpublished-menu-items-d7.patch591 bytesacbramley
#42 menu_admin_unpublished_nodes-460408-D6.patch1015 bytesgpk
#35 menu_admin_unpublished_nodes-460408.patch668 bytesgpk
#33 menu_admin_unpublished_nodes-460408.patch650 bytesgpk
#28 menu-administer-unpublished-nodes.patch1.03 KBgnindl
#22 menu.inc-access-unpublished-nodes-460408-22.patch.txt1011 bytesgirishmuraly
#21 menu.inc_.patch.txt940 bytesgirishmuraly
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

Title: Menu doesnt add / on end of path » Menu doesnt give feedback on unpublished node

If you create a menu item that is unpublished it doesn't show up, this gives no feedback

gpk’s picture

Title: Menu doesnt give feedback on unpublished node » Cannot administer menu item/link if it points to an unpublished node

Changing title - on a 6.x site I had a menu item pointing to a node that subsequently was unpublished. In order to remove the menu item I had first to re-publish the node, since menu items that point to unpublished nodes don't appear in the "List items" option/tab for the menu.

This behavior is even more bizarre if you create a new menu item and for the Path enter an unpublished node. The menu item is created but of course it is not visible when you are returned to the list of items for the menu.

Also a menu item whose only children are unpublished nodes will show as collapsed rather than leaf, even though you can't expand it (same thing happens if a menu item's whose only children are nodes you don't have permission to view). That's pretty minor though in comparison with the main problem.

gpk’s picture

Issue tags: +invisible menu items, +missing menu items

Tags.

gpk’s picture

Just hit this again. But it seems maybe no one else has the problem ..??!! Guess I should look into it some more anyways.

petrelharp’s picture

I agree. This is undesireable behavior.

andrewmacpherson’s picture

Issue tags: -Usability

I discovered a forum topic where a use-case/workflow is described in detail.

Menu admin doesn't show links to unpublished nodes

The desired behaviour described is that menu-items which link to unpublished nodes...
* Do not appear when the menu is displayed (menu block or primary/secondary links)
* DO appear on the menu settings page at admin/build/menu-customize/*

So, to clarify, is that the behaviour desired here?

andrewmacpherson’s picture

Further reading material...

It seems that the present behaviour is related to some earlier issues in with Drupal 5.x

I haven't confirmed this (I no longer have any Drupal 5 sites), but it seems that in D5 menu-items linking to unpublished nodes were displayed. I'll install a D5 sandbox to see what happens.

Forum topics:
i want to remove menu item when i unpublish node
Remove link from menu when node is deleted/unpublished

There is a D5 module which ensures menu items linking to nodes are only displayed to users with access to view that node: Remove Non-viewable Menu Items

In D6, menu-items linking to unpublished nodes are not displayed, even where the user has permission to view unpublished nodes.

Perhaps in fixing the undesirable menu display, the menu settings experience got broke.

Frustrating!

andrewmacpherson’s picture

Tagging issue

+Usability
+Needs usability review

Bojhan’s picture

Issue tags: -Needs usability review

.

gpk’s picture

@7, I think the behaviour described in the forum topic is different from what you state. The point being that on admin/build/menu-customize/menu-name, menu items for unpublished nodes don't get listed.

@8, in D5 and previously the menu system was more "dumb" - it frequently didn't know (or evaluate) whether a user was permitted to access the target pointed to by a menu and so you could easily get access denied errors e.g. by clicking on a menu link to a node you didn't have permission to view.

In D6 each menu link is evaluated in a more sophisticated way before being shown, with the result that typically only menu links to which you really truly have access get shown. But the converse doesn't exactly hold for as you say unpublished nodes *don't* get shown in the menu even if you do have access to them.

There is a related issue which deals with the use case where you might want to be able to create menu items pointing to nodes you don't have access to, or to non-existent paths. I'll post a link if I come across it.

RichieRich’s picture

Issue tags: +Usability

The menu system with respect to unpublished nodes is one of my biggest gripes with Drupal 6 (I joined Drupal at 6). I hope that the behaviour has been updated for Drupal 7. If you're the site administrator you should be able to administer menu entries without the need for the associated nodes to be published.

There have been times when I've wanted to publish a group of new pages simultaneously on a live site after having assigned the relevant menu slots, but I can't, at least not via the menu administration screen. This is just plain silly and a right pain in the ass at times. Sorry, but it's caused me annoyance on more than a few occasions.

Perhaps the fact that the nodes are unpublished could be indicated in the menu designer. This would represent a nice balance in my opinion and would prevent administrators from accidentally forgetting to publish nodes after having assigned menu entries

- Rich

gpk’s picture

@12: this will be fixed when if and when someone comes up with a patch to fix the bug.. :) Not sure if the behavior is the same in D7 or not.

John Bryan’s picture

Clumsy fix below, which I currently recreate after each Drupal update:-

/includes/menu.inc

/**
 * Check access and perform other dynamic operations for each link in the tree.
 */
function menu_tree_check_access(&$tree, $node_links = array()) {
+  global $user;

  if ($node_links) {
    // Use db_rewrite_sql to evaluate view access without loading each full node.
    $nids = array_keys($node_links);
    $placeholders = '%d'. str_repeat(', %d', count($nids) - 1);
+ /** include un-published content for authenticated users **/
+    if ($user->uid>0) {
+      $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.nid IN (". $placeholders .")"), $nids);
+    } else {
      $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.status = 1 AND n.nid IN (". $placeholders .")"), $nids);
+    }
    while ($node = db_fetch_array($result)) {
      $nid = $node['nid'];
      foreach ($node_links[$nid] as $mlid => $link) {
        $node_links[$nid][$mlid]['access'] = TRUE;
      }
    }
  }
  _menu_tree_check_access($tree);
  return;
}

Add the lines prefixed "+"

This will allow logged in users to see un-published nodes in the menus.

This is fine for sites where only site maintainers have logins. For more granular control at least it shows where code can be added to get the effect required.

nzcodarnoc’s picture

Version: 7.x-dev » 6.15

Thanks John,

I've just hacked my core with you patch, in case it might help someone the lines I've replaced only show the menu if the user has the "administer menu" option:

    if (user_access("administer menu")) {
     $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.nid IN (". $placeholders .")"), $nids);
    } else {
      $result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.status = 1 AND n.nid IN (". $placeholders .")"), $nids);
    }
Bojhan’s picture

Version: 6.15 » 7.x-dev

Ehh? This is D7 bug

John Bryan’s picture

May have been reported by someone trying D7 but it has existed in all versions of D6 ;¬)

My patch was certainly created on D6 quite some time ago.

ash_choice23’s picture

how to make a parent item point to nothing???
i want the child items to be pointing to nodes....
not the parent item...
its just not possible....lz!! help

andrewmacpherson’s picture

reply @19, ash_choice23,

The Special menu items module provides the behaviour you need.

Menu Firstchild and DHTML Menu may also be of interest to you.

girishmuraly’s picture

Version: 7.x-dev » 6.16
Status: Active » Patch (to be ported)
FileSize
940 bytes

Subscribing.

I like the patches suggested by replies #14 and #15. Although I believe 'administer nodes' is a more generic permission to use, since multiple contrib modules make use of menu_tree_check_access().

For e.g. this issue is also affecting the functionality of Trees module http://drupal.org/project/trees.

I am guessing this bug is not yet resolved since an actual patch has not been submitted. I am therefore attaching a patch. And also setting version to Drupal 6, since I believe this should be backported there first due to the existing huge D6 user base.

Would anyone know when the right patch would make it to the next release?

Thanks,
Girish

girishmuraly’s picture

Version: 6.16 » 7.x-dev
FileSize
1011 bytes

Since I have attached the D6 backported version above, I thought I'd supply the patch for D7 as well.

bricef’s picture

Patch in #21 works for me in drupal 6.16! Please includes it in Drupal 6.17, this behavior disturb my users

gpk’s picture

Status: Patch (to be ported) » Needs work

I wonder if displaying the unpublished nodes' menu items in menus as well as on the menu admin page is the best solution, and also I don't think 'administer nodes' is the correct permission here.

http://api.drupal.org/api/function/menu_overview_form/7 (which calls http://api.drupal.org/api/function/menu_tree_check_access/7, where the proposed patch bites) sets global variable $menu_admin which is used elsewhere in core (just in http://api.drupal.org/api/function/user_is_anonymous/7 in fact) to allow a menu administrator to administer links they otherwise wouldn't be able to see. Seems to me that this is the best approach here too, i.e. check global $menu_admin instead of the 'administer nodes' (or even 'administer menu') permission.

> Would anyone know when the right patch would make it to the next [6.x] release?
The following (non-sequential) steps need to be gone through, starting with 7.x:
- community discusses the way ahead (in progress)
- patches are posted (in progress)
- community agrees the way ahead
- patch is posted and passes automated testing
- patch is reviewed and tested by the community (and status updated accordingly)
- a core committer likes it and commits
- patch ported back to 6.x
- 6.x core committer likes it and commits

So inclusion in even the next 6.x release is far from automatic and depends on this issue getting far more attention than it has done recently :)

tcarnell’s picture

Actually, this is a pretty major issue:

I am editing a Drupal installation for a client who is preparing for a new product launch so it is absolutely essential (with serious legal consequences) that the content is not made public prior to the launch.

I am tasked with creating a new menu hierarchy with a set of new pages. I need to be able to create pages (that are unpublished), with menu items (that are not enabled) and be able to edit all of these things 'offline' without the risk of any of the content being publically accessible.

Or perhaps I'm not working in the 'Drupal' way?

tcarnell’s picture

(sorry, I'm using 6.x)

gpk’s picture

@26: I would suggest you either apply the patch at #21 to provide a temporary fix (http://drupal.org/patch/apply) or implement a different workflow for creating new content. IIRC there are various "workflow" contrib modules that can help with this. If you have any further questions on your own requirements (as distinct from how this should ultimately be fixed in core) I'd suggest opening a (new) support issue.

gnindl’s picture

Version: 7.x-dev » 6.17
Status: Needs work » Needs review
FileSize
1.03 KB

I think gpk in #24 is absolute right as we should use the global variable $menu_admin determining if we should display a menu item. In the function menu_overview_form (menu.admin.inc) this variable is just set before the menu_tree_check_access() which filters out menu links which point to unpublished nodes:

  $menu_admin = TRUE;
  menu_tree_check_access($tree, $node_links);
  $menu_admin = FALSE;

So we just have to interpret this variable in the SQL statement - not filtering unpublished nodes - see patch for details:

Status: Needs review » Needs work

The last submitted patch, menu-administer-unpublished-nodes.patch, failed testing.

Damien Tournoud’s picture

Version: 6.17 » 7.x-dev

Please do not change the version tag.

donquixote’s picture

subscribe.

I guess the correct thing would be to use node_access() for each item that is a node.
Or, more generic, for each item:

<?php
$item = menu_get_item($path);
$access = $item['access'];
?>

Or is this operation too expensive?

gpk’s picture

$item['access'] is checked in http://api.drupal.org/api/function/_menu_tree_check_access/7.

>I guess the correct thing would be to use node_access() for each item that is a node.
The bit of code in http://api.drupal.org/api/function/menu_tree_check_access/7 is I think sorting out the menu items of nodes in the tree --- which share the same menu router entry node/%node/view. We want to avoid doing a node_load() on each of them, as the comments in that function say.

The $select->addTag sorts out the node access conditions for the current user (in D6 this is done via db_rewrite_sql()), but the condition $select->condition('n.status', 1) prevents unpublished nodes getting into the $tree. The suggestion is that global $menu_admin, which will specifically have been set when viewing the menu administration screen, needs to be checked before adding this condition.

gpk’s picture

Status: Needs work » Needs review
FileSize
650 bytes

Status: Needs review » Needs work

The last submitted patch, menu_admin_unpublished_nodes-460408.patch, failed testing.

gpk’s picture

Status: Needs work » Needs review
FileSize
668 bytes
Webrotta’s picture

I tried the trick mentioned in #14 and it seems to be working quite fine. Though there is one problem. Let's say I have a parent node having for example 4 children and I want to publish 2 of those. The problem is that I need to flush alla caches for the effect take place. And vice versa -> If I unpublish those they still show on menu but clicking them result in access denied. I mean If I logout and check whether the menu is working for unauthenticated users.

So the question is -> How do I make the effect take place immediately without flushing caches?

gpk’s picture

gpk’s picture

@36: do you have the block cache enabled?

Webrotta’s picture

Had. But disabling and clearing caches wont help =(

Webrotta’s picture

I have recenty updated my drupal 5 site to drupal 6. In drupal 5 I had this kind of trick made in menu.inc:

function theme_menu_item($mid, $children = '', $leaf = TRUE) {
  $item = db_fetch_array(db_query("SELECT * FROM {menu} WHERE mid = '%d'", $mid));
  $item_nid = ltrim( $item['path'], "node/" );
  $itemstatus = db_fetch_array(db_query("SELECT status FROM {node} WHERE nid = '%d'", $item_nid));

// stab: hide menu item if not published
  global $user;
  if ($itemstatus['status'] == 1 ) {
    return '<li class="'. ($leaf ? 'leaf' : ($children ? 'expanded' : 'collapsed')) .'">'. menu_item_link($mid) . $children ."</li>\n";
  } 
// if authenticated user then show all
  if ($user->uid > 0 ) {
	return '<li class="'. ($leaf ? 'leaf' : ($children ? 'expanded' : 'collapsed')) .'">'. menu_item_link($mid) . $children ."</li>\n";
  } else {
// if contact form and not user 
    if ($mid == '1905' OR $mid == '1810') {
      return '<li class="'. ($leaf ? 'leaf' : ($children ? 'expanded' : 'collapsed')) .'">'. menu_item_link($mid) . $children ."</li>\n";
    }
  }
}

This Worked fine in Drupal 5 but drupal 6 seems to be a bit different so I tried that trick mentioned in #14.

gpk’s picture

The menu system was completely re-written in D6. Will shortly post a version of #35 for D6.

gpk’s picture

As promised.
@Webrotta: this doesn't I think do exactly what you want. Unpublished pages don't appear in the menus. But with this patch they do appear on the admin page for the relevant menu, which was the issue was originally about.

Since the unpublished pages never feature in the menu, there is no opportunity for the menus to show the wrong stuff for e.g. anon user.

Webrotta’s picture

Can that be easily altered so that unpublished nodes would show up to all authenticated users. Not only admins ?

gpk’s picture

Yes, change if (empty($GLOBALS['menu_admin'])) { to if (!$GLOBALS['user']->uid) {

But you may then hit the problem you had previously.

Update: have tried this out and the menus seem to be showing correctly, i.e. the unpublished pages show up in the menu just for authenticated users, not quite sure why you originally hit the problem with unpublished pages showing in the menu for anon users.

Webrotta’s picture

Hmmmm... after removing my menu.inc and replacing it with 6.19 original menu.inc and running that patch I can't see unpublished nodes. Can you see if there is something wrong with the patch ?

Update: I don't have problem that unpublished nodes show up to anon. My problem was that refresh problem. I mean that if i publish child nodes I need to flush caches to get them show up to anons. And vice versa -> If I unpublish them, they still show up on menus though anons get then access denied. And again, after flushing caches they disappear from the menu. So my problem is to get this work realtime without having to flush cache all the time.

gpk’s picture

I can't reproduce the refresh problem. With #42 applied and modified by #44, as I publish/unpublish, the menu item comes and goes for anon. It is always there for auth user. This is with page cache *and* block cache enabled. What happens if you use original menu.inc - do you have the refresh problem then? I wonder if something else is causing the problem.

[update: having looked at http://api.drupal.org/api/function/menu_block/6 it is clear that the block cache is not implicated in any way since menu blocks don't cache.]

Webrotta’s picture

Sorry, my bad. Stupid PEBMAC =). It seems to be working fine now for authenticated users. Something mystical still is with that refresh. I test this a bit and report later. Possibly tomorrow. Thanks what you've done so far.

Lukas von Blarer’s picture

this issue is really bad. who possibly wants to publish a complete section of a website without setting up the menu for it?

please post a fix for this!

thanks

lukas

wizonesolutions’s picture

Yeah, we just ran into this issue as well. It's pretty critical. The content admin thought that they weren't adding at all. Then, when I published the node out of curiosity...BAM, four new menu items!

Does anybody know if a stopgap solution exists as a contributed module right now? If not...I may wind up creating one. It's perfectly doable. One just needs to duplicate and tweak some core code and use hook_menu_alter to make admin/build/menu go there instead.

I might well wind up doing this soon...but if anyone knows if it's already been done let me know.

wizonesolutions’s picture

Also, is there a 6.x analogue of this issue? Or a backport issue? I'm talking about 6.x in my case. Still exists in 6.20.

wizonesolutions’s picture

Tried the patch in comment 33 (and modified it a bit), but I need to get menu items showing up only for users with the right to see them.

For now, I achieved this by changing

<?php 
if (!$menu_admin) { 
?>

to

<?php 
if (!$menu_admin && $user->uid) {
?>

However, I do intend to write and release a simple module to work around this until it is fixed in core. I searched and didn't find any that already did the job. I'll link to it here when it's done.

geek-merlin’s picture

+1 and sub!

wizonesolutions’s picture

Just an update that I tried to write a module for this, and it would actually take significant effort and pose re-configuration challenges that are perhaps not practical. But, as a note to self and if anyone wants to try it, I think you need to:

1. Implement hook_menu_alter() to make the 'admin/build/menu' links use custom code (based heavily on menu.inc/menu.module) that makes more use of global $menu_admin. In other words, you have to duplicate a lot of what's behind admin/build/menu for this one thing (can you see why I didn't want to do it right away? :))
2. Implement hook_block() and copy the block-per-menu code from menu.module, except use the custom code to respect $menu_admin and show admins all nodes - perhaps with a bit of colorization behind unpublished links, like there is currently on nodes.
3. Write a script to migrate people's block settings (menu names are the same, so this is somewhat feasible), or tell them that they need to use your blocks instead now and re-configure everything. If it's a small site, this wouldn't be too bad. For a big one...things get trickier.

So there you have it. If I run into this issue on another site, I'll give this a stab. But for now, kittens are dying on each page refresh on the site I hacked core for :(

Somebody save them.

Peter Törnstrand’s picture

This could easily have been accomplished if only there where more tagging the queries in core.

function menu_tree_check_access(&$tree, $node_links = array()) {
  if ($node_links) {
    $nids = array_keys($node_links);
    $select = db_select('node', 'n');
    $select->addField('n', 'nid');
    $select->condition('n.status', 1);
    $select->condition('n.nid', $nids, 'IN');
    $select->addTag('menu_tree_check_access'); /* customization */
    $select->addTag('node_access');
    $nids = $select->execute()->fetchCol();
    foreach ($nids as $nid) {
      foreach ($node_links[$nid] as $mlid => $link) {
        $node_links[$nid][$mlid]['access'] = TRUE;
      }
    }
  }
  _menu_tree_check_access($tree);
}

and then used:

/**
 * Implements hook_query_TAG_alter().
 */
function CUSTOMMODULE_query_menu_tree_check_access_alter($query) {
  if (user_access('administer menu')) {
    $where =& $query->conditions();
    unset($where[0]);
  }
}

This way everybody would be happy. The ones who hates the idea could just not use the custom module and the ones, like me, who really needs this functionality would not be forced to hack core.

gpk’s picture

jelo’s picture

Just wanted to add my 2 cents that this is a major problem. We use workflow with multiple editors and frequently end up with duplicate nodes because editor 1 misses unpublished content from editor 2 (as it does not get shown anywhere in menus).

Example: editor starts content, but leaves it in drafted state (unpublished), that editor gets sick for a week, two days later supervisor asks why the content has not been published, editor 2 gets on it and creates a completely new page rather than picking up on the drafted one. I am aware about the issue and train people again and again to use a view that shows unpublished content prior to creating new content. But this is a cumbersome process and hence it does not happen. If the editor would see unpublished items in the menu, they would straight away be able to continue work on the unpublished content.

I do not know all the details about the various permissions that have been discussed here, but why should we not just create a custom permission such as "View unpublished content in menu" and then apply the core modification from #14 and #15?

John Pitcairn’s picture

Yeah. Sub (for D6, mostly).

thomasmurphy’s picture

Yeah, just ran up against this wall too. massive problem for out 6.x site, will probablyl have to hack the core (thanks for submitted patches). Is there any possibility of getting this fix into 7.x?

agerard’s picture

Thanks to various helpful souls for patches - I now have the ability to view/edit menu items in the admin view for unpublished nodes in my D7 site. As mentioned above, there's still the problem that unpublished parents can't be selected in node pulldowns, but it since appears that a hierarchy created/selected thru the menu admin screen is successful, guess that's the way to go. I'm relatively new to Drupal, esp 7, and wonder if anyone can point me to a way to actually display the menu (as distinct from administering it) for admins/editors that includes unpublished nodes. For checking navigation "in place" and seeing the site structure this would be very helpful.

rbishop’s picture

im using d7 w/ workbench, similar to #56. i love the tag solution from Peter #54 . are there any other ways to not hack core to do this ?

neilnz’s picture

I have a related problem - the node edit form has the same access check on the menu parent selection. If you create a page and parent it to another page, then unpublish the parent, the next time you edit the child page it will try to reparent itself to the top of primary links. With vertical tabs it's easy to miss that it's about to do this.

#42 fixes half the problem for me, but not the node edit form. I'm using this additional patch to menu.module to allow it to apply to the node edit form too:

diff --git modules/menu/menu.module modules/menu/menu.module
index cfe17b1..f446e52 100644
--- modules/menu/menu.module
+++ modules/menu/menu.module
@@ -206,6 +206,8 @@ function menu_load($menu_name) {
  *   and mlid. The list excludes the given item and its children.
  */
 function menu_parent_options($menus, $item) {
+  global $menu_admin;
+
   // The menu_links table can be practically any size and we need a way to
   // allow contrib modules to provide more scalable pattern choosers.
   // hook_form_alter is too late in itself because all the possible parents are
@@ -221,11 +223,13 @@ function menu_parent_options($menus, $item) {
     $limit = _menu_parent_depth_limit($item);
   }
 
+  $menu_admin = TRUE;
   foreach ($menus as $menu_name => $title) {
     $tree = menu_tree_all_data($menu_name, NULL);
     $options[$menu_name .':0'] = '<'. $title .'>';
     _menu_parents_recurse($tree, $menu_name, '--', $options, $item['mlid'], $limit);
   }
+  $menu_admin = FALSE;
   return $options;
 }
casey’s picture

Version: 7.x-dev » 8.x-dev
acbramley’s picture

Agreed that this functionality should exist. Being able to structure entire sections of unpublished content before going live is essential for large public sites.

acbramley’s picture

Here's a small but effective patch that allows users with the administer menu permission to see unpublished node's menu items in the admin interface and on the node form. For Drupal 7.10

dbassendine’s picture

I notice there's now also a contrib module for 7.x which allows menu items pointing to unpublished nodes' to be visible in the admin interface: http://drupal.org/project/menu_view_unpublished.

I took a look at backporting this for 6.x, but this module relies on d7's hook_query_alter to go in and alter menu_tree_access_check where it filters the results by n.status = 1. This hook unfortunately isn't available for d6, so it doesn't look like this approach would work there.

btopro’s picture

Actively working on a module to combat this issues -- http://drupal.org/project/hidden_nodes . This basically gives you a hidden checkbox which ties into node_access instead of core publishing status checkbox. This allows you as a site admin that has the "view hidden content" permission to create nodes and hide them from users without the perm yet still be able to see them yourself. Recently came upon this core issue and didn't like the idea of altering the expected (even if wrong imo) results of the core menu query to fetch everything.

Status: Needs review » Needs work

The last submitted patch, 460408-unpublished-menu-items-d7.patch, failed testing.

btopro’s picture

Status: Needs work » Needs review

at #64 -- this looks like a work around.

Does anyone know why publish status is handled as it's own column in drupal and not as a core node_access grant using the access control system that's built in? This would allow for filtering out of nodes the same way other material is. Hidden nodes takes this approach but I'm wondering what the advantage is of a 1/0 publishing status that overrides ALL node access control mechanisms. Sounds like a node_access grant with a really high priority, obviously then anything without a grant is assumed published / accessible by users and everything with a record in there is not.

This could also play nicely with the idea in #214190: Publish nodes permission. View unpublished nodes could be a per-type permission at that point since it was reading off of the node access table. This may seem like a pretty radical solution but publish/unpublish is a pretty primitive concept by today's node access control standards.

gpk’s picture

@68: publish status is likely a very ancient Drupal construct which predates node access. It gives some ability to hide/show content without installing a node access module but as you say the integration with the node access system is awkward.

btopro’s picture

if there is interest in making publish / unpublish behave in a way more fitting with node_access grants I'd be willing to try and roll a patch. If there's no interest in this I'll just port hidden nodes to D8 and achieve the same effect while ignoring publish status :).

damontgomery’s picture

I'd like to throw my hat in and say that Menu View Unpublished worked for us for Drupal 7.

I think the default behavior should be as follows:

All menu links show up in the Admin menus.
Unpublished nodes do not show up in menu links for people without a "view unpublished" content permission.
This permission should not be Administer menus, but rather be the same, or a subset of the "view unpublished / revision" permissions.

We have a need for a review / editor role who is not able to publish content, but can create content or review the content. This role needs to see all the menus as they would be after the launch of a section of pages.

Thanks for the work you guys did in this thread and to casey for putting together the Menu View Unpublished module.

thekevinday’s picture

I found #54 to be perfectly functional.

I support $71s arguments.

dww’s picture

Hit the same bug on a site I'm working on (D6, in fact).

For D7, I noticed this now exists: https://drupal.org/project/menu_view_unpublished
Quick skim of the code reveals it's a bit of a hack, but it looks like it'd probably work.

Why tie this to 'administer menus'? Why not just let people who can view the unpublished node(s) see those menu items?

btopro’s picture

It seems to me like publish/unpublish as a whole would need to be overhauled for a pervasive solution. If publish/unpublish was tied to node access records then it would be a more comprehensive solution then simply allowing for menus to work one way and the dreaded "status=1" query being set elsewhere in the previously hardcoded method. Hidden nodes takes this approach in both D6 and D7 by tying the status to a node access record.

While it doesn't directly correct the publishing status like menu_view_unpublished (which I agree is a bit hacky) it does provide a decent methodology for replacing the publish/unpublish issue as a whole.

dww’s picture

node.status is not handled via the node access grant world for both performance and legacy reasons. I don't think you're going to get much support undoing that.

For now, I'll just be hacking core. ;) But, it'd be nice to actually fix this for real, and backport it to D7 (and D6, where there's no possible solution in contrib).

gpk’s picture

@73

Why tie this to 'administer menus'? Why not just let people who can view the unpublished node(s) see those menu items?

I guess for 6.x/7.x the question in my mind is whether that wld count as a feature change rather that just a bugfix. E.g. Are there any situations where one might want the unpublished node *not* in the menu? Prob I shod have a look back through the history of this issue. For 8.x the same question remains.

<looks back through the history of this issue>

Yes I agree.

Not sure about implementation though (not having had my head much in Drupal recently!). We don't want to be calling node_access on every node, but not sure how we could do the use case of #71. Core lets you view an unpublished node if you are a node admin (of some sort, depending on Drupal version) or if it is your own node (subject to additional permission in 7.x). We could hard-code those constraints into the query, but that wouldn't help #71.

Thoughts?

Update re. #71: db_rewrite_sql (6.x) or query altering (7.x) does the trick of course. Doh!

dww’s picture

Yeah, I agree we can't node_access() every node individually. My point is we could at least have the perm check be:

  if (user_access('administer menu') || user_access('administer nodes') {
    ...
  }

For example, on the newspaper site I'm working on, most of the staff have 'administer nodes' but not everyone has 'administer menu'. But, we know that people with 'administer nodes' can definitely see all unpublished nodes (and bypass all node access checks entirely, in fact), so it seems pointless to keep these menu items hidden from them.

damontgomery’s picture

Can you just add a permission, "View Unpublished Menus" and use that? For larger sites with workflows, there will be people who need to approve (outside of Drupal) content they cannot administer.

I believe I've hacked in the check that looks for "view unpublished" permission, since for us all authenticated (logged in) users are people who can see anything. Our anonymous traffic is our "live" site. In use case, everyone who is logged in has at least "view unpublished" permissions. We have some users where this is all they can do. They don't even see the admin menu, they are essentially seeing the live site with draft content in place of published content.

catch’s picture

Priority: Normal » Major

I just ran into this on a (Drupal 6) client site. Core lets you add the menu item, then redirects to the menu item list, and doesn't show you the item. Querying the db and browsing directly to the edit link you can edit it.

At the very, very least there should be a drupal_set_message() to explain what's happening, but really this should show the link. Since you can continue to add menu links that never show up ad infinitum to the same path and due to the fact there's zero feedback, bumping to major.

xjm’s picture

Issue tags: +Needs backport to D7
xjm’s picture

xjm’s picture

Issue tags: +Needs backport to D7

...

Siggy’s picture

kerasai’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
nerdcore’s picture

DrupalCn Portland: working on reroll (nerdcore + deepakml)

nerdcore’s picture

Worked with @deepakml based on the patch in #35, using user_access in D8 API and the role "administer menu".

Deepakml’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Usability, -invisible menu items, -missing menu items, -Needs backport to D7, -Needs reroll

The last submitted patch, menu_admin_unpublished_nodes-460408.patch, failed testing.

Deepakml’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Usability, +invisible menu items, +missing menu items, +Needs backport to D7, +Needs reroll
dww’s picture

Status: Needs review » Needs work

Thanks for getting this moving again. I had forgotten about this bug. Two problems with patch #86:

A) + if(!user_access('administer menu')) {

Code style: needs a space after if.

B) (I still think) we really want to check for both 'administer menu' and 'administer nodes' here.

dww’s picture

Status: Needs work » Needs review
FileSize
647 bytes

(it's a 3 line patch -- the patch is the interdiff). ;)

btopro’s picture

+1

So essentially you need override menu perms and override node perms to see nodes that are unpublished in menus. Makes sense to me that the bar is high for whenever this gets back-ported as to not freak people out that built their nodes into menu under the premise that they are inaccessible via the menu.

dww’s picture

Not really, no. Boolean logic can be tricky.

!(A || B) is equivalent to !A && !B

So, if you have 'administer nodes' OR you have 'administer menus' we do not add the condition that the node must be published. The logic being that people with administer nodes can already see unpublished nodes so there's no harm in showing those menu items, and if you're administering menus, you should be able to see all menu items no matter what.

I'm not sure if this would be any more clear:

   if (!(user_access('administer menu') || user_access('administer nodes'))) {
     $select->condition('n.status', 1);
   }

I think computer programmers basically need to understand boolean logic or they're doomed, so I'm not sure it's worth trying to comment this or write it differently. AFAIK, we don't have a coding standard for this (nor should we)...

But, I'm happy to re-roll this if people feel strongly this either needs a comment or that the !(A || B) form is more self-evident.

Cheers,
-Derek

btopro’s picture

lol.. wow, sorry bout that. No I get the principle I read too quickly (baby sleep deprivation). I agree there's not a great way of writing that differently but I think that makes the most sense.

From a node security / policy stand point though, should you be allowed to see even the title of a node if you potentially don't have access to it? I understand the administer menu function but if I don't have access to a node I'd still be able to "see" that it exists in a menu via this permission. Very very minor / edge I know but sounds like that'd constitute a security bug based on some others I've seen in the past. As this only affects nodes (and we aren't doing an access check against every node in the structure) would it make sense to just have the admin nodes permission to avoid any potential permission escalation (even if its just to see a title)?

thekevinday’s picture

The !(A || B) is equivalent to !A && !B is logically equivalent, but performance differences depending on hardware and, in some smaller way, sometimes software.
I wonder what the best performance (assuming there is a difference, software-wise) is for php, and therefore, drupal.

I am not noticing anything explicitly mentioned here:
- https://drupal.org/coding-standards
- https://drupal.org/coding-standards#operators
- https://drupal.org/node/328206

For the node security title comment, I would say that it is possible that a node may only contain a title.
If the node was behind access control and the node title was all that defined the node, then this would be a security issue.

Besides, exposing any information that has no reason to be exposed is always bad in my personal opinion.

Exposing a node title to unauthorized users would likely be a case-by-case situation.

dww’s picture

Re: performance: please. The nano-optimization (this is going to be too small to be called "micro") differences in performance for these two boolean logic approaches are absolutely meaningless in an interpreted PHP script that's hitting the filesystem, etc. That's completely irrelevant to this patch. What matters is correctness and understandability. Sorry to say this harshly, but the idea we should spend time trying to optimize this is absurd.

Re: security - hrm. The original point of this issue was a major usability WTF for people trying to administer menus on a site, adding menu items pointing to not-yet-published nodes while setting up a site, not being able to effectively administer the menu. In practice, this usability use-case is going to be handled by the fact that uid 1 and generally first-time site admins are going to have both perms. In advanced/weird edge cases, I could imagine a situation where a certain class of admins can administer menus, but not nodes, and then yeah, we'd be exposing node titles to people who shouldn't really see them. So sure, let's say you need to be able to administer nodes to see the menu items for unpublished nodes. In 99.9% of the cases, it's going to be irrelevant, and in the other 0.1% of cases, better to be correct about security and have the UX suffer than the other way around.

So here's a new patch that just checks 'administer nodes'. This needed to be re-rolled, anyway since #1498674: Refactor node properties to multilingual landed.

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Usability, -invisible menu items, -missing menu items, -Needs backport to D7, -Needs reroll

The last submitted patch, 460408-96.menu_admin_unpublished_nodes.patch, failed testing.

dww’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Usability, +invisible menu items, +missing menu items, +Needs backport to D7, +Needs reroll
star-szr’s picture

Does this need tests?

star-szr’s picture

Issue tags: -invisible menu items, -missing menu items, -Needs reroll +Needs tests

It's a bugfix and seems pretty testable, just going to go ahead and tag it.

dawehner’s picture

I try to understand why "administer nodes" is used here instead of "bypass node access".

dww’s picture

Status: Needs review » Needs work

Because in D7 that's what it was and I haven't followed D8 closely enough. :) Sorry.

aubjr_drupal’s picture

+1 (belated comment)

Using D7, but this issue has been around for a long time. It is a total PITA when working with menus in the Drupal admin section. The WTF feeling when an end user creates a menu link to an unpublished node in the Menu administration section and it doesn't appear is totally justified. There's no warning or explanation.

On #71, having the unpublished menu links appear in the admin area could be improved with a jQuery .hide/.show snippet - fired by an obvious "Show/Hide Unpublished links" link - to toggle their visibility (a usability upgrade). You'd probably want the unpublished links hidden by default?

Menus are a powerful part of Drupal, but their quirks/limitations (i.e. only one menu item per node, items not available like entities, this issue, etc.) can be really frustrating.

ladybug_3777’s picture

This has already been mentioned, but worth mentioning again since it's been over a year - Menu View Unpublished (although perhaps a little hacky) is still a viable option for a quick fix: https://drupal.org/project/menu_view_unpublished

So far it works well with workbench moderation too, which was big deal for my client.

ladybug_3777’s picture

Issue summary: View changes

Updated issue summary.

david_garcia’s picture

So I just came accross problem, prepared a small patch and then found this issue page, but I believe this was good to get a new approach on how to address the problem.

Evaluating proposed patches:

- Why in the admin interface should even node existance be checked? Even if links are broken or inconsistent they should show up in the admin interface to be fixed.

- Why are the proposed patches using permission access control when there is a global variable named $menu_admin that allows us to know IF WE ARE IN THE ADMINISTRATION INTERFACE.

My proposed solution is to: when $menu_admin global is set to TRUE bypass all node check (even if it exists).

Forgive me for not providing the patch in the appropiate format, I still need to spend some time learning on contributions.

david_garcia’s picture

Issue summary: View changes
FileSize
1.74 KB

Messed up not attaching the patch....

jojonaloha’s picture

I think #105 brings up a good point. I also think $menu_admin was suggested in #24 but I haven't read anything on why that shouldn't be used (sorry I read many of the responses but not all of them and only did a quick search for "menu_admin" to see if anybody opposed it).

I've attached an updated patch of #106 that fixes some formatting.

I also think the issues brought up in #61 still need to be addressed.

david_garcia’s picture

Yes I also cross-read all the posts in this issue and missed #61, yet, I believe #61 can be considered a separate issue as the original problem is about menu items not showing in the administration interface. Maybe if we split that into a new issue there is a better chance that something can get commited, this issue has been around for a long time with no positive response.

Anyways, combining the patch in #61 with the one in #107 would probably solve both problems at once.

thekevinday’s picture

In response to #105:

- Why are the proposed patches using permission access control when there is a global variable named $menu_admin that allows us to know IF WE ARE IN THE ADMINISTRATION INTERFACE.

If I remember correctly (because it has been a while), there is a need for nodes to appear in menus for people who own or otherwise have edit access to those nodes. These users are not admins and should not have admin access, let alone the $menu_admin permission.

That is to say, users with access to their nodes should be able to see their nodes in a menu before publishing their nodes.

This becomes an issue in a workflow style environment where some users can add content to menus but it is up to another user to approve the publishing
This also becomes an issue for an automated environment where when a user adds a node, it gets auto-added to some menu.

In my case, I solved this by explicitly adding a tag so that I could alter this using custom modules:
$select->addTag('menu_tree_check_access');

This would produce the fewest changes to core and users who want different behavior can just add/create a custom module.

david_garcia’s picture

I see your point, there are many things to be sorted out, but this is not directly related to the original issue: Links not showing in the administration interface. When in the admin interface, all links should show, even if related node is not published or does not even exist.

If I am not wrong, $menu_admin is not a permission as such, it is just a flag set to true and then back to false when performing some operations from the administration interface so that we can now if we are retrieving the menu in an administration context.

dotton’s picture

Re-rolled #91 against drupal-7.24

dww’s picture

Re: #105-107: Not all admins are created equal. Drupal has fine-grained access control, not just "regular user" vs. "admin". That's why we're checking for specific permissions. There are a few extremely powerful permissions that basically let you do anything, but there are plenty of partial-admin permissions (like ability to moderate comments) where a user would get limited access to the admin section of the site (such that $menu_admin would be TRUE) but who don't necessarily also have permission to see all unpublished content.

david_garcia’s picture

Re: #112

As per Drupal Documentation:

global $menu_admin

"Boolean indicating that a menu administrator is running a menu access check"

But this documentation statement is missleading: $menu_admin is manually set only on administrative interfaces when retrieving menus and does not, itself, depend on the user's role or permissions.

This is not just an "administrator" it is a "menu_administrator", wich I believe IMHO should suffice as a criteria to bypass all node check.

As for the example in your comment, $menu_admin is set only in menu administrative interfaces and very atomically set back to FALSE once the menu to be administered has been retrieved, so I believe that if any admin user makes it to the menu administration page it does not make sense to decide on the logged in user's role and/or permissions.

dww’s picture

No, we already had this conversation. Just because you can admin menus doesn't mean you automatically can see all private content. See comment #96 (and a few previous).

Thanks,
-Derek

david_garcia’s picture

Sorry to be insistent, just a last thought on this one, discussion is always good. The fact that an admin might have menu edit permissions but not node admin permissions does not mean that they should not be able to manage a link pointing to a node they cannot manage. You are not exposing "all private content" just the node's URL and probably title if the menu was not manually crafted. Yet this might make sense, I might want an admin to manage menu's but no the nodes they are pointing to themselves, but probably I won't care if the menu admin can see the node/xxxx path.

dww’s picture

I never said "all private content". The point is that the node title itself is (potentially) sensitive info. To repeat:

The original point of this issue was a major usability WTF for people trying to administer menus on a site, adding menu items pointing to not-yet-published nodes while setting up a site, not being able to effectively administer the menu. In practice, this usability use-case is going to be handled by the fact that uid 1 and generally first-time site admins are going to have both perms. In advanced/weird edge cases, I could imagine a situation where a certain class of admins can administer menus, but not nodes, and then yeah, we'd be exposing node titles to people who shouldn't really see them. So sure, let's say you need to be able to administer nodes to see the menu items for unpublished nodes. In 99.9% of the cases, it's going to be irrelevant, and in the other 0.1% of cases, better to be correct about security and have the UX suffer than the other way around.

I was wrong about the node permission we wanted to use, so when I said "administer nodes" it should have said "bypass node access" (or whatever it's called now). ;)

Cheers,
-Derek

jojonaloha’s picture

Re #112

There are a few extremely powerful permissions that basically let you do anything, but there are plenty of partial-admin permissions (like ability to moderate comments) where a user would get limited access to the admin section of the site (such that $menu_admin would be TRUE) but who don't necessarily also have permission to see all unpublished content.

and

The point is that the node title itself is (potentially) sensitive info

True and I agree, but the opposite is true as well when using 'administer nodes' or 'bypass node access' to check access for node links. Users that have access to those nodes (if checked via node_access()) cannot see the links on the menu admin page. Obviously we can't call node_access() on every node link for performance reasons as you mentioned in #77.

I think the closest thing to a compromise here would be a new permission, so it would be something like:

if (!(user_access('bypass node access') || user_access('administer nodes') || ($menu_admin && user_access('manage unpublished menu links')))) {
  $select->condition('n.status', 1);
}

Thoughts?

dww’s picture

I think that's overkill. We're totally over-complicating this issue. For the 3rd time:

So sure, let's say you need to be able to administer nodes bypass node access to see the menu items for unpublished nodes. In 99.9% of the cases, it's going to be irrelevant, and in the other 0.1% of cases, better to be correct about security and have the UX suffer than the other way around.

Let's not further complicate this problem and introduce an entirely new permission (confusing 100% of Drupal sites in the world) for the benefit of the 0.1% of cases of sites that really care about fine-grained admin permissions that will let a given role admin the menus but not see all unpublished node titles.

If anyone wants to help actually fix this bug, please write some tests for it. ;) Here's a re-roll with the right permission name so at least it doesn't need re-rolling for that.

Thanks!
-Derek

AaronBauman’s picture

Just lost about 2 hours to this bug.

dww's fix in #118 is an excellent interim fix, doesn't significantly impact any "security" around node access, and doesn't change existing API other than to provide at least some relief for this bug.

Can we get this *good* fix into core and then keep talking about a *perfect* solution?
It's been 4.5+ years.

rli’s picture

Status: Needs review » Reviewed & tested by the community

Agree with #119

Just wasted some time on this. We should not have wait for 4.5 years to fix this issue. Please commit the patch in #118 to at least save people's time. Then we can continue talk about all the possibilities for a perfect solution.

The last submitted patch, 106: menu.inc_.patch, failed testing.

The last submitted patch, 107: drupal-unpublished-menu-items-d7-460408-107.patch, failed testing.

The last submitted patch, 111: menu_admin_unpublished_nodes-460408-111.patch, failed testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Still needs tests.

david_garcia’s picture

Patch #111 and #118 will not take into account "broken" links that point to unexisting nodes. Are these also supposed to be ghost items when going to the menu's administrative interface?

marthinal’s picture

Let's try this test.

marthinal’s picture

Status: Needs work » Needs review
marthinal’s picture

This is really weird. My d8 is up to date and the test fail fixing the bug with the patch... let's try again only the test.

penyaskito’s picture

With PHP 5.5.3 I get an error too.

Fail Other MenuNodeTest.php 97 Drupal\menu\Tests\MenuNodeTest->tes
Link with label mKTluLT1 found.

Bots are running 5.3, could be related.

Status: Needs review » Needs work

The last submitted patch, 128: 460408-128-only-test.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
2.05 KB

Let's try again.

The last submitted patch, 131: 460408-131-only-test.patch, failed testing.

The last submitted patch, 131: 460408-131-only-test.patch, failed testing.

dawehner’s picture

+++ b/core/includes/menu.inc
@@ -1559,8 +1559,9 @@ function menu_tree_check_access(&$tree, $node_links = array()) {
+    if (!user_access('bypass node access')) {

We should really use \Drupal::currentUser()->hasPermission instead

pp’s picture

Status: Needs review » Needs work

I agree @dawehner in #134

marthinal’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

Sure! :) patch + test

csg’s picture

Tested #136 and it works. I think it can be set to RTBC if it passes the tests.

marthinal’s picture

Issue tags: +#D8SVQ, +#SprintWeekend2014
marthinal’s picture

Issue tags: -#D8SVQ, -#SprintWeekend2014 +D8SVQ, +SprintWeekend2014
star-szr’s picture

Thanks @marthinal! It seems like we should definitely test the menu UI as well since that is the reported bug here.

  1. +++ b/core/includes/menu.inc
    @@ -1559,8 +1559,9 @@ function menu_tree_check_access(&$tree, $node_links = array()) {
         // @todo This should be actually filtering on the desired node status field
         //   language and just fall back to the default language.
    -    $select->condition('n.status', 1);
    -
    +    if (!\Drupal::currentUser()->hasPermission('bypass node access')) {
    +      $select->condition('n.status', 1);
    +    }
    

    Can we remove the @todo above these lines (at least for the 8.x patch)?

  2. +++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuNodeTest.php
    @@ -91,6 +96,11 @@ function testMenuNodeFormWidget() {
    +    // Verify that the menu link is not avalilable for anonymous users because the page is unpublished.
    

    Typo here, "avalilable", and this comment line is too long. It should be wrapped to fit within 80 characters per http://drupal.org/node/1354#drupal.

marthinal’s picture

@Cottser the result is the same. In this test we are adding directly the link from the node creation. Fixed the 2 points.

Thanks for the review!

star-szr’s picture

Assigned: Unassigned » star-szr

After re-reading this issue I think the failing test in #131 is testing for a completely different bug which as far as I can tell has been fixed already. Going to send it for a re-test but it passes locally.

I'm also unable to reproduce the behaviour described in the OP on 8.x. So this may be ready to go back to 7.x but I'm going to investigate a bit further to be sure.

star-szr’s picture

131: 460408-131-only-test.patch queued for re-testing.

star-szr’s picture

Assigned: star-szr » Unassigned
FileSize
1.39 KB

git bisect told me this was fixed by #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit, not sure exactly how, it's a decent-sized patch :)

Here is a regression test for 8.x for review that follows the steps outlined in the issue summary. This test (and the fix code from #118) would be easy to backport to D7, and I've confirmed that the test fails and passes appropriately on 7.x.

Status: Needs review » Needs work

The last submitted patch, 144: 460408-144.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

144: 460408-144.patch queued for re-testing.

marthinal’s picture

144: 460408-144.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh it is good to know that the other issue fixed the access logic as well.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This is because we individually check access on menu items, and lost the optimization that did the node grants (and published query) check. That's going to come back to haunt us in other ways, but good that it fixed this bug.

Committed/pushed the test to 8.x. Moving to 7.x for backport.

star-szr’s picture

Issue tags: +Novice

This is a very novice-able backport. Please post a test-only and test + fix patch, see #144 for more details and https://drupal.org/contributor-tasks/write-tests for an explanation of the test-only patch.

gobinathm’s picture

Assigned: Unassigned » gobinathm
markie’s picture

Assigned: gobinathm » markie
markie’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.92 KB

Here is the my first attempt to backport to D7. Please let me know if you have any questions.

Status: Needs review » Needs work

The last submitted patch, 153: menu-access_unpublished_nodes-460408-153.patch, failed testing.

markie’s picture

Status: Needs work » Needs review
FileSize
1.71 KB

This is why using IDE's to create patch files is a silly idea kids. Learn from my mistakes.

Status: Needs review » Needs work

The last submitted patch, 155: menu-access_unpublished_nodes-460408-155.patch, failed testing.

markie’s picture

Status: Needs work » Needs review
FileSize
5.28 KB

Re-wrote test to be in it's own class because I think some of the build errors are outside the scope of this issue.

umar-ahmad’s picture

FileSize
24.49 KB

#157 Works perfectly well.
menu-link-for-unpublished

umar-ahmad’s picture

Status: Needs review » Closed (fixed)
acbramley’s picture

Status: Closed (fixed) » Needs review

@umar-ahmad this has not been committed yet, please don't close the issue.

david_garcia’s picture

Cannot administer menu item/link if it points to a non existent or broken node

That will be the tittle for a new issue even more obscure and affecting even less people that will be left over after this thread is closed.

If the former took 5 years to solve, and solve time is directly proportional to number of affected people, we might be talking about 100 years for the latter.

Just joking, this issue looks haunted, it took so long, yet we are still left with the problem described above :)

markie’s picture

Maybe we should change the title, but the issue seems to be corrected. As it was explained to me, previously, users with 'bypass node access' were unable to edit menu items that are not published. This fixes that issue, and the tests prove that users with 'bypass node access' permissions can, but users without can not. Proposed title:

Users with 'bypass node access' permissions cannot administer menu item/link if it points to an unpublished node

All in favor, say Aye.

ps: anyone want to test this and mark it RTBC would be spiffy. @umar-ahmad thanks for the vote of confidence.

star-szr’s picture

Status: Needs review » Needs work
FileSize
46.2 KB

Thanks for your work on this @markie! I think we should find a way to incorporate this test into the existing MenuTestCase. More importantly, I checked locally to see if the new test you wrote fails without the fix code and it does not, so it's hard to tell whether the fix is actually fixing the bug! When adding tests where the fix is not already in place it's good practice to first post a test-only patch, then a test + fix patch, for an example see https://drupal.org/contributor-tasks/write-tests.

A couple things to try:

  1. I would recommend running the test from #155 locally through the simpletest UI and looking at the verbose output: https://drupal.org/node/30036
  2. Also look again at the test that was committed to 8.x and notice we are not using $big_user because $big_user does not have 'bypass node access' permission.

The goal is to get a test that fails without the fix and passes with it.

Edit: Oh yeah I uploaded a screenshot, might as well embed it…

This shows the verbose output from simpletest:

Akif Khan akif500’s picture

markie’s picture

Cottser
Sorry for the radio silence on this issue these last couple of weeks.
I think I am confused on the goals of this test. Isn't a test more for making sure the code works as designed, opposed to proving a bug fix? From what I could tell, the point of this test is to show that people with proper admin access can update menu items on unpublished entities, while those without the proper permissions could not. Is this not the desired behavior? Since this bug originated in 8.x, are we even certain this is an issue in 7.x? I do agree, however, that this should probably be in the main menu tests and I will work to get it in there over this week.

Thanks for your patience
markie

iSoLate’s picture

Isn't it better to implement a hook for this? The "bypass node access" permission seems to broad. For instance the workbench moderation module implements a permission called "view all unpublished content", which could potentially fix the issue as well instead of using a global bypass permission.

gaellafond’s picture

Subscribe.
Why is this issue not fixed yet? I might have plenty of "floating" menu items that I can't delete because I can't even see them (even with admin which has all right and all "bypass" permissions). If I change the URL of a Page's view (for example), all menu items pointing to that view disappear. I can't even fix them, because they are not there any more. It's very annoying.

acbramley’s picture

@gaellafond if you see the number of comments on the issue, you can tell it's not a straight forward fix. It has been fixed in D8 and the backport is currently being worked on. Maybe you can try help out and test the patches above :)

AdeleBN’s picture

#157 works just fine for me.
Thank you!

joelpittet’s picture

#165 The fix looks fine but the tests need to be fixed up it seems from @Cottser's points.

@markie The goal of the tests is to provide a functional regression test. So that this bug won't happen again in the future. To do that we show: "In the following scenario USER WITH PERMISSION we can see on the ADMIN PAGE that LINK IS SHOWN"

If we post the test only patch without the fix here, tesybot will prove for us that the current HEAD is broken, and then with the fix and the test that it's fixed.

  1. +++ b/modules/menu/menu.test
    @@ -722,3 +722,128 @@ class MenuNodeTestCase extends DrupalWebTestCase {
    +    $this->assertText($item['link_title'], "Menu link to unpublished node is visible to users with 'bypass node access' permission");
    ...
    +    $this->assertNoText($item['link_title'], "Menu link to unpublished node is only visible to users with 'bypass node access' permission");
    

    Shouldn't these tests be checking for the 'hey monkey' title on the manage menu page?

  2. +++ b/modules/menu/menu.test
    @@ -722,3 +722,128 @@ class MenuNodeTestCase extends DrupalWebTestCase {
    +  /**
    +   * Horribly stolen from the MenuTestCase. Not in the least bit DRY.
    +   * @param int $plid
    +   * @param string $link
    +   * @param string $menu_name
    +   * @param bool $expanded
    +   *
    +   * @return mixed
    +   */
    +  function addMenuLink($plid = 0, $link = '<front>', $menu_name = 'navigation', $expanded = TRUE) {
    

    Don't think this tests the problem and can likely be removed if it's not testing the bug and may be a duplicate from MenuTestCase which is where @Cottser was suggesting these tests be moved into if possible.

  3. +++ b/modules/menu/menu.test
    @@ -722,3 +722,128 @@ class MenuNodeTestCase extends DrupalWebTestCase {
    +  /**
    +   * Again.. Stolen from MenuTestCase...
    +   * @param $mlid
    +   * @param array $expected_item
    +   */
    +  function assertMenuLink($mlid, array $expected_item) {
    
    

    Same here, this may be a duplicate test and is not needed if moved into MenuTestCase.

joelpittet’s picture

Assigned: markie » Unassigned

Maybe someone can pick this up? @markie I'm unassigning incase someone has time to tackle this.

gaellafond’s picture

@acbramley Sorry for my incendiary comment. I simply applied the patch #35 and it worked. When you look at menu_overview_form(), they set the Global variable $menu_admin just for the time of calling menu_tree_check_access(), but the variable is never used within menu_tree_check_access().

function menu_overview_form($form, &$form_state, $menu) {
  global $menu_admin;
  ...
  $menu_admin = TRUE;
  menu_tree_check_access($tree, $node_links);
  $menu_admin = FALSE;
  ...
}

It feels like they simply forgot to check it. It really looks like a strait forward bug with a strait forward solution, but I might not see all the implications of that simple fix.

ngocketit’s picture

This is very weird. We encountered the same problem and it took us nearly one hour to find out what's wrong. Luckily my colleague found this.

  • catch committed 7ce03d6 on 8.3.x
    Issue #460408 by marthinal, gpk, girishmuraly, jojonaloha,...

  • catch committed 7ce03d6 on 8.3.x
    Issue #460408 by marthinal, gpk, girishmuraly, jojonaloha,...
stefan.r’s picture

Issue tags: -Novice
aerozeppelin’s picture

Updates to patch as per comments in #170.

The last submitted patch, 177: test-only-fail-460408-177.patch, failed testing.

joelpittet’s picture

Re-uploading the test only patch, because it didn't apply above.

Status: Needs review » Needs work

The last submitted patch, 179: test-only-fail-460408-177.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs backport to D6, -Needs tests

Ok great, reviewed the patch and the tests did what was expected of them. RTBC the patch in #177

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 179: test-only-fail-460408-177.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 179: test-only-fail-460408-177.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

I stupidly restarted the tests. The RTBC patch is in #177

joelpittet’s picture

FileSize
1.2 KB

Here's D6 version in-case someone needs this for migrating to D6 and would like to see all the tree items regardless of their node status... like me.

joelpittet’s picture

stefan.r’s picture

Issue tags: +Pending Drupal 7 commit, +Drupal bugfix target
Fabianx’s picture

Assigned: Unassigned » Fabianx
vtkachenko’s picture

Please commit this patch. We seek for this feature on our project.

  • catch committed 7ce03d6 on 8.4.x
    Issue #460408 by marthinal, gpk, girishmuraly, jojonaloha,...

  • catch committed 7ce03d6 on 8.4.x
    Issue #460408 by marthinal, gpk, girishmuraly, jojonaloha,...
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Pending Drupal 7 commit

By changing the behavior of menu_tree_check_access() unconditionally, this patch creates side effects.

For example, these users will now start seeing the unpublished menu item on the front end of the site (when the menu is rendered) too. Is that really what we want? I tend to think not... if no other users will see the link in the menu on the front end (even ones who have access to see unpublished content), then we probably shouldn't show it to these administrators there either.

Maybe we need some kind of fix that uses the global $menu_admin variable (as first suggested in #24 and as used in some of the subsequent patches) in combination with the "bypass node access" check. (Or something that makes this take effect on the menu administration pages only.) Then https://api.drupal.org/api/drupal/modules!menu!menu.admin.inc/function/_... could label these in the menu administration tree with something like "unpublished content" (similar to how it gives other items that won't be visible for different reasons labels such as "disabled").

jamesrward’s picture

Issue tags: +Baltimore2017

While I understand the concerns in #193 I think the deciding factor here should be how D8 handles this. The patch in #177 would give us consistency with D8 behaviour. If we want to make a change to that behaviour it should happen in D8 first and be back-ported to D7. With that in mind I suggest we move #177 back to RTBC. Happy to look at this more as I'd love to see this issue concluded as part of DrupalCon Baltimore.

joseph.olstad’s picture

Issue tags: +Drupal 7.60 target
dsutter’s picture

RTBC+ patch #157

gdaw’s picture

# 157 working great for us, RTBC+1 for D7

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

I am with @jamesrward on this. RTBC #177 , It's got the test to prove it.

This issue was opened in 2009, it has been *EDIT* 11 years and counting.

mgifford’s picture

Any ETA on fixing this. The patch is now a year old.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 187: 460408-D6-187.patch, failed testing. View results

joseph.olstad’s picture

Status: Needs work » Reviewed & tested by the community

still RTBC, ***EDIT*** patch #177 is the one that passes and is the one we're using.
@joel_pittet, your patch has an incorrect path, cannot be applied.

joseph.olstad’s picture

Status: Reviewed & tested by the community » Needs review

yes what @david_rothstein said is valid concern.

But why was this committed to 8.x?

David_Rothstein’s picture

Drupal 7 is a stable release, so just because something was added to Drupal 8 years ago (before Drupal 8 was even released) does not mean it's appropriate for Drupal 7 :)

Also, in this case, the patch wasn't actually ever committed to Drupal 8 (the only thing committed to Drupal 8 in this issue was a test); rather, this problem was fixed in Drupal 8 as a side effect of another issue, which also evidently allowed the unpublished node menu items to be displayed on the front end of the site. The goal of this issue is just to fix the menu link administration, not to change the front end behavior, so for Drupal 7 we should limit the fix to that if at all possible.

Here's a patch along the lines of what I suggested in #193.

Note that we could actually get a partial fix along those lines just with a one-line interdiff from the previous patch, basically:

-    if (!user_access('bypass node access')) {
+    if (empty($GLOBALS['menu_admin']) || !user_access('bypass node access')) {

but that would leave a confusing situation where an administrator would have no indication that some links they see in the admin area don't actually display on the site itself, and it also would not fix things when editing menus on the node form. So I think this slightly more complicated patch is worth considering, although it will require a bit more review.

David_Rothstein’s picture

The last submitted patch, 204: 460408-204-TESTS-ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jamesrward’s picture

This is great @David_Rothstein. Running it through a few test scenarios on sites that used the old patch and so far it's working flawlessly.

jamesrward’s picture

Anyone running Big Menu such as with a Wetkit install profile, be aware the "unpublished" label doesn't appear when viewing the menu structure. Should be an easy patch for Big Menu once this gets into core.

jstoller’s picture

Status: Needs review » Reviewed & tested by the community

This is fabulous! Works perfectly.

I've also posted patches for the Menu Link Weight and Menu Editor modules, adding support for this change.

mforbes’s picture

To clarify, is the intention for the menu administration form to [A: show you links to unpublished nodes only when you have permission to "bypass node access"], or to [B: show you links to unpublished nodes that you have access to, which might be only your own unpublished nodes in the event that you can administer the menu but not "bypass node access"]?

A quick glance at patch 204 leads me to believe it is implementing [A], but [B] would be less confusing for, e.g., sites where (via OG) users can administer only the menu related to their portion of the site, and certainly do not have the "bypass node access" permission. Another example would be for sites using the view_unpublished module: you have access to all unpublished nodes and thus should see them in menu administration, but again you do not have the "bypass node access" permission.

EDIT: removed an additional point that wasn't correct.

Basically, [A] means that for users who can administer the menu but not bypass node access, toggling the status of their own node (that they authored) makes the menu item disappear and reappear in menu administration, while [B] means it consistently appears in menu administration.

mforbes’s picture

Having finally gotten through a bit more of this thread, I now see that a node_access() check was already discussed a bit, e.g. #76, #77, #117 and the thinking is that it might be too expensive. I wonder if that's still true (this issue being quite old), or if perhaps that check got cheaper over time. Admins are tolerant of slow pages for things they do only occasionally, and how slow could it even be if simply viewing a menu also does a node_access() check on each item (not for published status, but access generally) anyway?

donquixote’s picture

I was made aware of this issue in #2926344: Support menu_add_link_labels() in Menu Editor in menu_editor issue queue.
Some feedback about the most recent patch.
All of this can be seen as suggestions, not as blockers.

+  elseif ($item['link_path'] == 'user' && $item['module'] == 'system') {
+    $title .= ' (' . t('logged in users only') . ')';
+  }

I see this was copied from old code, but shouldn't we change == to === when comparing to strings literals?

----------------

 /**
+ * Adds labels to the title of a hidden, unpublished or logged-in menu link.
+ *
+ * @param string $title
+ *   The title of the menu link. This will be modified as necessary to add the
+ *   appropriate label in parentheses at the end.
+ * @param array $item
+ *   An array representing the menu link item.
+ */
+function menu_add_link_labels(&$title, $item) {

Why do we need this by-reference argument? What about this instead:

function menu_admin_link_label_suffix($item) {
  if (..) {
    return ' (' . t('something') . ')';
  }
  return '';
}

--------

And:
What if a link is disabled AND unpublished? Should we append a list like " (disabled, unpublished)"?

------------

Overall the logic seems like a not-so-perfect heuristic. There is no way for other modules to add their own admin menu link label suffix to specific paths. But maybe trying to improve this is way over the top for this issue, so we should keep it at this.

joseph.olstad’s picture

Issue tags: -Needs backport to D7, -Drupal 7.60 target +Drupal 7.61 target

Bumping to 7.61. This didn't make it into 7.60

joseph.olstad’s picture

cboyden’s picture

Basically, [A] means that for users who can administer the menu but not bypass node access, toggling the status of their own node (that they authored) makes the menu item disappear and reappear in menu administration, while [B] means it consistently appears in menu administration.

Looking at @mforbes' comments, I think the current (option A) patch is going to be confusing. We have a distribution where the highest role that's generally available to users doesn't have "bypass node access." This is partly because there's an Organic Groups component, and partly for general security reasons. There's not going to be any benefit from this patch for sites in similar situations.

cboyden’s picture

Digging a bit deeper, there are some more options such as using Menu view unpublished or implementing hook_query_TAG_alter that would allow us to use a different permission instead of "bypass node access," so the option A approach looks fine.

joseph.olstad’s picture

MustangGB’s picture

Issue tags: -Drupal 7.64 target +Drupal 7.69 target
MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
byronveale’s picture

Recently discovered the D8 behavior of menu items for unpublished content, found the patch in #177 and love it. Attaching a patch for the Book module that uses the same logic. If you have ever tried building a book with unpublished content, you understand why…

Sorry, I haven’t included any tests, am rather a novice and am also under a deadline.

byronveale’s picture

Whoops, make files like unique issue nid’s, so attached patch is #177 plus the same logic for book outlines.

joseph.olstad’s picture

Has tests, D8 already has this

izmeez’s picture

@byronveale I thought the working patch for this issue was in comment #204 not #177, can you explain how your patch in #219 and #220 build on that or are you adding a separate patch in which case it should probably be combined with the existing patch and an interdiff to show the change or what's added. Thanks.

ressa’s picture

Status: Reviewed & tested by the community » Needs review

Won't the latest patch need a fresh review, to clarify which patch (#177 or #220) should be used?

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

RTBC 204 by David Rothstein

vuil’s picture

I confirm that #204 works perfectly.

mcdruid’s picture

Issue tags: -Drupal 7.74 target +Pending Drupal 7 commit

#204 LGTM (unsurprisingly).

I was a little put off by the global variable at first glance, but appreciate that it's already used elsewhere in core (as noted in #24 and possibly elsewhere).

This doesn't solve all problems described in this 12 year old issue (e.g. #209 and others), but it's progress and I think it's worth committing.

mforbes’s picture

Despite my concerns in #209 (supported by #214), ultimately I agree with #226 especially considering the momentum of #224. Time to commit and break out edge cases (which, to be clear, are neither helped nor harmed by #204) into separate issues.

joseph.olstad’s picture

For the benefit of humanity we should commit the patch as it is a needed improvement. I still recall how hard it was to debug this issue and how much time it took just to find the solution. Many people not as tenacious and patient as we all are would have thrown in the towel rather than find the root cause of the problem.

Fabianx’s picture

Assigned: Fabianx » mcdruid

RTBC + 1, #204 looks correct to me, we add one new function, but that's fine.

  • mcdruid committed f4515a5 on 7.x
    Issue #460408 by marthinal, dww, David_Rothstein, gpk, joelpittet,...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Drupal bugfix target, -Pending Drupal 7 commit

Thanks everyone that contributed to this over the many, many years!

mcdruid’s picture

Assigned: mcdruid » Unassigned

Status: Fixed » Closed (fixed)

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