Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedIf you create a menu item that is unpublished it doesn't show up, this gives no feedback
Comment #2
gpk CreditAttribution: gpk commentedChanging 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.
Comment #3
gpk CreditAttribution: gpk commentedTags.
Comment #5
gpk CreditAttribution: gpk commentedJust hit this again. But it seems maybe no one else has the problem ..??!! Guess I should look into it some more anyways.
Comment #6
petrelharp CreditAttribution: petrelharp commentedI agree. This is undesireable behavior.
Comment #7
andrewmacpherson CreditAttribution: andrewmacpherson commentedI 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?
Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson commentedFurther 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!
Comment #9
andrewmacpherson CreditAttribution: andrewmacpherson commentedTagging issue
+Usability
+Needs usability review
Comment #10
Bojhan CreditAttribution: Bojhan commented.
Comment #11
gpk CreditAttribution: gpk commented@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.
Comment #12
RichieRich CreditAttribution: RichieRich commentedThe 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
Comment #13
gpk CreditAttribution: gpk commented@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.
Comment #14
John Bryan CreditAttribution: John Bryan commentedClumsy fix below, which I currently recreate after each Drupal update:-
/includes/menu.inc
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.
Comment #15
nzcodarnoc CreditAttribution: nzcodarnoc commentedThanks 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:
Comment #16
Bojhan CreditAttribution: Bojhan commentedEhh? This is D7 bug
Comment #17
John Bryan CreditAttribution: John Bryan commentedMay 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.
Comment #19
ash_choice23 CreditAttribution: ash_choice23 commentedhow 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
Comment #20
andrewmacpherson CreditAttribution: andrewmacpherson commentedreply @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.
Comment #21
girishmuraly CreditAttribution: girishmuraly commentedSubscribing.
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
Comment #22
girishmuraly CreditAttribution: girishmuraly commentedSince I have attached the D6 backported version above, I thought I'd supply the patch for D7 as well.
Comment #23
bricef CreditAttribution: bricef commentedPatch in #21 works for me in drupal 6.16! Please includes it in Drupal 6.17, this behavior disturb my users
Comment #24
gpk CreditAttribution: gpk commentedI 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 :)
Comment #25
tcarnell CreditAttribution: tcarnell commentedActually, 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?
Comment #26
tcarnell CreditAttribution: tcarnell commented(sorry, I'm using 6.x)
Comment #27
gpk CreditAttribution: gpk commented@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.
Comment #28
gnindl CreditAttribution: gnindl commentedI 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:
So we just have to interpret this variable in the SQL statement - not filtering unpublished nodes - see patch for details:
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease do not change the version tag.
Comment #31
donquixote CreditAttribution: donquixote commentedsubscribe.
I guess the correct thing would be to use
node_access()
for each item that is a node.Or, more generic, for each item:
Or is this operation too expensive?
Comment #32
gpk CreditAttribution: gpk commented$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.
Comment #33
gpk CreditAttribution: gpk commentedComment #35
gpk CreditAttribution: gpk commentedComment #36
Webrotta CreditAttribution: Webrotta commentedI 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?
Comment #37
gpk CreditAttribution: gpk commented#35: menu_admin_unpublished_nodes-460408.patch queued for re-testing.
Comment #38
gpk CreditAttribution: gpk commented@36: do you have the block cache enabled?
Comment #39
Webrotta CreditAttribution: Webrotta commentedHad. But disabling and clearing caches wont help =(
Comment #40
Webrotta CreditAttribution: Webrotta commentedI have recenty updated my drupal 5 site to drupal 6. In drupal 5 I had this kind of trick made in menu.inc:
This Worked fine in Drupal 5 but drupal 6 seems to be a bit different so I tried that trick mentioned in #14.
Comment #41
gpk CreditAttribution: gpk commentedThe menu system was completely re-written in D6. Will shortly post a version of #35 for D6.
Comment #42
gpk CreditAttribution: gpk commentedAs 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.
Comment #43
Webrotta CreditAttribution: Webrotta commentedCan that be easily altered so that unpublished nodes would show up to all authenticated users. Not only admins ?
Comment #44
gpk CreditAttribution: gpk commentedYes, change
if (empty($GLOBALS['menu_admin'])) {
toif (!$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.
Comment #45
Webrotta CreditAttribution: Webrotta commentedHmmmm... 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.
Comment #46
gpk CreditAttribution: gpk commentedI 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.]
Comment #47
Webrotta CreditAttribution: Webrotta commentedSorry, 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.
Comment #48
Lukas von Blarerthis 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
Comment #49
wizonesolutionsYeah, 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.
Comment #50
wizonesolutionsAlso, 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.
Comment #51
wizonesolutionsTried 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
to
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.
Comment #52
geek-merlin+1 and sub!
Comment #53
wizonesolutionsJust 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.
Comment #54
Peter Törnstrand CreditAttribution: Peter Törnstrand commentedThis could easily have been accomplished if only there where more tagging the queries in core.
and then used:
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.
Comment #55
gpk CreditAttribution: gpk commented#35: menu_admin_unpublished_nodes-460408.patch queued for re-testing.
Comment #56
jelo CreditAttribution: jelo commentedJust 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?
Comment #57
John Pitcairn CreditAttribution: John Pitcairn commentedYeah. Sub (for D6, mostly).
Comment #58
thomasmurphy CreditAttribution: thomasmurphy commentedYeah, 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?
Comment #59
agerard CreditAttribution: agerard commentedThanks 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.
Comment #60
rbishop CreditAttribution: rbishop commentedim 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 ?
Comment #61
neilnz CreditAttribution: neilnz commentedI 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:
Comment #62
casey CreditAttribution: casey commentedComment #63
acbramley CreditAttribution: acbramley commentedAgreed that this functionality should exist. Being able to structure entire sections of unpublished content before going live is essential for large public sites.
Comment #64
acbramley CreditAttribution: acbramley commentedHere'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
Comment #65
dbassendine CreditAttribution: dbassendine commentedI 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.
Comment #66
btopro CreditAttribution: btopro commentedActively 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.
Comment #68
btopro CreditAttribution: btopro commentedat #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.
Comment #69
gpk CreditAttribution: gpk commented@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.
Comment #70
btopro CreditAttribution: btopro commentedif 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 :).
Comment #71
damontgomery CreditAttribution: damontgomery commentedI'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.
Comment #72
thekevinday CreditAttribution: thekevinday commentedI found #54 to be perfectly functional.
I support $71s arguments.
Comment #73
dwwHit 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?
Comment #74
btopro CreditAttribution: btopro commentedIt 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.
Comment #75
dwwnode.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).
Comment #76
gpk CreditAttribution: gpk commented@73
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!
Comment #77
dwwYeah, I agree we can't node_access() every node individually. My point is we could at least have the perm check be:
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.
Comment #78
damontgomery CreditAttribution: damontgomery commentedCan 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.
Comment #79
catchI 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.
Comment #80
xjmComment #81
xjmComment #82
xjm...
Comment #83
Siggy CreditAttribution: Siggy commented#33: menu_admin_unpublished_nodes-460408.patch queued for re-testing.
Comment #84
kerasai CreditAttribution: kerasai commentedComment #85
nerdcore CreditAttribution: nerdcore commentedDrupalCn Portland: working on reroll (nerdcore + deepakml)
Comment #86
nerdcore CreditAttribution: nerdcore commentedWorked with @deepakml based on the patch in #35, using user_access in D8 API and the role "administer menu".
Comment #87
Deepakml CreditAttribution: Deepakml commentedComment #89
Deepakml CreditAttribution: Deepakml commented#86: menu_admin_unpublished_nodes-460408.patch queued for re-testing.
Comment #90
dwwThanks 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.
Comment #91
dww(it's a 3 line patch -- the patch is the interdiff). ;)
Comment #92
btopro CreditAttribution: btopro commented+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.
Comment #93
dwwNot 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:
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
Comment #94
btopro CreditAttribution: btopro commentedlol.. 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)?
Comment #95
thekevinday CreditAttribution: thekevinday commentedThe
!(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.
Comment #96
dwwRe: 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.
Comment #98
dww#96: 460408-96.menu_admin_unpublished_nodes.patch queued for re-testing.
Comment #99
star-szrDoes this need tests?
Comment #100
star-szrIt's a bugfix and seems pretty testable, just going to go ahead and tag it.
Comment #101
dawehnerI try to understand why "administer nodes" is used here instead of "bypass node access".
Comment #102
dwwBecause in D7 that's what it was and I haven't followed D8 closely enough. :) Sorry.
Comment #103
aubjr_drupal CreditAttribution: aubjr_drupal commented+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.
Comment #104
ladybug_3777 CreditAttribution: ladybug_3777 commentedThis 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.
Comment #104.0
ladybug_3777 CreditAttribution: ladybug_3777 commentedUpdated issue summary.
Comment #105
david_garcia CreditAttribution: david_garcia commentedSo 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.
Comment #106
david_garcia CreditAttribution: david_garcia commentedMessed up not attaching the patch....
Comment #107
jojonaloha CreditAttribution: jojonaloha commentedI 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.
Comment #108
david_garcia CreditAttribution: david_garcia commentedYes 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.
Comment #109
thekevinday CreditAttribution: thekevinday commentedIn response to #105:
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.
Comment #110
david_garcia CreditAttribution: david_garcia commentedI 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.
Comment #111
dotton CreditAttribution: dotton commentedRe-rolled #91 against drupal-7.24
Comment #112
dwwRe: #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.
Comment #113
david_garcia CreditAttribution: david_garcia commentedRe: #112
As per Drupal Documentation:
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.
Comment #114
dwwNo, 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
Comment #115
david_garcia CreditAttribution: david_garcia commentedSorry 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.
Comment #116
dwwI never said "all private content". The point is that the node title itself is (potentially) sensitive info. To repeat:
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
Comment #117
jojonaloha CreditAttribution: jojonaloha commentedRe #112
and
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:
Thoughts?
Comment #118
dwwI think that's overkill. We're totally over-complicating this issue. For the 3rd time:
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
Comment #119
AaronBaumanJust 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.
Comment #120
rliAgree 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.
Comment #124
webchickStill needs tests.
Comment #125
david_garcia CreditAttribution: david_garcia commentedPatch #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?
Comment #126
marthinal CreditAttribution: marthinal commentedLet's try this test.
Comment #127
marthinal CreditAttribution: marthinal commentedComment #128
marthinal CreditAttribution: marthinal commentedThis 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.
Comment #129
penyaskitoWith PHP 5.5.3 I get an error too.
Bots are running 5.3, could be related.
Comment #131
marthinal CreditAttribution: marthinal commentedLet's try again.
Comment #134
dawehnerWe should really use \Drupal::currentUser()->hasPermission instead
Comment #135
pp CreditAttribution: pp commentedI agree @dawehner in #134
Comment #136
marthinal CreditAttribution: marthinal commentedSure! :) patch + test
Comment #137
csg CreditAttribution: csg commentedTested #136 and it works. I think it can be set to RTBC if it passes the tests.
Comment #138
marthinal CreditAttribution: marthinal commentedComment #139
marthinal CreditAttribution: marthinal commentedComment #140
star-szrThanks @marthinal! It seems like we should definitely test the menu UI as well since that is the reported bug here.
Can we remove the @todo above these lines (at least for the 8.x patch)?
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.
Comment #141
marthinal CreditAttribution: marthinal commented@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!
Comment #142
star-szrAfter 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.
Comment #143
star-szr131: 460408-131-only-test.patch queued for re-testing.
Comment #144
star-szrgit 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.
Comment #146
star-szr144: 460408-144.patch queued for re-testing.
Comment #147
marthinal CreditAttribution: marthinal commented144: 460408-144.patch queued for re-testing.
Comment #148
dawehnerOh it is good to know that the other issue fixed the access logic as well.
Comment #149
catchThis 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.
Comment #150
star-szrThis 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.
Comment #151
gobinathmComment #152
markie CreditAttribution: markie commentedComment #153
markie CreditAttribution: markie commentedHere is the my first attempt to backport to D7. Please let me know if you have any questions.
Comment #155
markie CreditAttribution: markie commentedThis is why using IDE's to create patch files is a silly idea kids. Learn from my mistakes.
Comment #157
markie CreditAttribution: markie commentedRe-wrote test to be in it's own class because I think some of the build errors are outside the scope of this issue.
Comment #158
umar-ahmad CreditAttribution: umar-ahmad commented#157 Works perfectly well.
Comment #159
umar-ahmad CreditAttribution: umar-ahmad commentedComment #160
acbramley CreditAttribution: acbramley commented@umar-ahmad this has not been committed yet, please don't close the issue.
Comment #161
david_garcia CreditAttribution: david_garcia commentedThat 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 :)
Comment #162
markie CreditAttribution: markie commentedMaybe 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.
Comment #163
star-szrThanks 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:
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:
Comment #164
Akif Khan akif500 CreditAttribution: Akif Khan akif500 commented157: menu-access_unpublished-nodes_460408-157.patch queued for re-testing.
Comment #165
markie CreditAttribution: markie commentedCottser
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
Comment #166
iSoLate CreditAttribution: iSoLate commentedIsn'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.
Comment #167
gaellafond CreditAttribution: gaellafond commentedSubscribe.
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.
Comment #168
acbramley CreditAttribution: acbramley commented@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 :)
Comment #169
AdeleBN CreditAttribution: AdeleBN commented#157 works just fine for me.
Thank you!
Comment #170
joelpittet#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.
Shouldn't these tests be checking for the 'hey monkey' title on the manage menu page?
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.
Same here, this may be a duplicate test and is not needed if moved into MenuTestCase.
Comment #171
joelpittetMaybe someone can pick this up? @markie I'm unassigning incase someone has time to tackle this.
Comment #172
gaellafond CreditAttribution: gaellafond commented@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 callingmenu_tree_check_access()
, but the variable is never used withinmenu_tree_check_access()
.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.
Comment #173
ngocketit CreditAttribution: ngocketit commentedThis 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.
Comment #176
stefan.r CreditAttribution: stefan.r commentedComment #177
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUpdates to patch as per comments in #170.
Comment #179
joelpittetRe-uploading the test only patch, because it didn't apply above.
Comment #181
joelpittetOk great, reviewed the patch and the tests did what was expected of them. RTBC the patch in #177
Comment #183
joelpittetComment #185
joelpittetI stupidly restarted the tests. The RTBC patch is in #177
Comment #186
joelpittetHere'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.
Comment #187
joelpittetTypo in #186 patch. Hiding other patches
Comment #188
stefan.r CreditAttribution: stefan.r commentedComment #189
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #190
vtkachenko CreditAttribution: vtkachenko commentedPlease commit this patch. We seek for this feature on our project.
Comment #193
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedBy 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").
Comment #194
jamesrward CreditAttribution: jamesrward as a volunteer commentedWhile 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.
Comment #195
joseph.olstadComment #196
dsutter CreditAttribution: dsutter as a volunteer commentedRTBC+ patch #157
Comment #197
gdaw CreditAttribution: gdaw as a volunteer commented# 157 working great for us, RTBC+1 for D7
Comment #198
joseph.olstadI 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.
Comment #199
mgiffordAny ETA on fixing this. The patch is now a year old.
Comment #201
joseph.olstadstill 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.
Comment #202
joseph.olstadyes what @david_rothstein said is valid concern.
But why was this committed to 8.x?
Comment #203
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDrupal 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:
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.
Comment #204
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedChanged the test a bit so that it now tests for the "unpublished" label also, as well as a few other minor improvements. No changes to the rest of the code (just to the test).
Comment #206
jamesrward CreditAttribution: jamesrward as a volunteer commentedThis 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.
Comment #207
jamesrward CreditAttribution: jamesrward as a volunteer commentedAnyone 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.
Comment #208
jstollerThis is fabulous! Works perfectly.
I've also posted patches for the Menu Link Weight and Menu Editor modules, adding support for this change.
Comment #209
mforbes CreditAttribution: mforbes commentedTo 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.
Comment #210
mforbes CreditAttribution: mforbes commentedHaving 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?
Comment #211
donquixote CreditAttribution: donquixote commentedI 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.
I see this was copied from old code, but shouldn't we change == to === when comparing to strings literals?
----------------
Why do we need this by-reference argument? What about this instead:
--------
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.
Comment #212
joseph.olstadBumping to 7.61. This didn't make it into 7.60
Comment #213
joseph.olstadComment #214
cboyden CreditAttribution: cboyden commentedLooking 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.
Comment #215
cboyden CreditAttribution: cboyden commentedDigging 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.Comment #216
joseph.olstadComment #217
MustangGB CreditAttribution: MustangGB commentedComment #218
MustangGB CreditAttribution: MustangGB commentedComment #219
byronveale CreditAttribution: byronveale at Princeton University commentedRecently 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.
Comment #220
byronveale CreditAttribution: byronveale at Princeton University commentedWhoops, make files like unique issue nid’s, so attached patch is #177 plus the same logic for book outlines.
Comment #221
joseph.olstadHas tests, D8 already has this
Comment #222
izmeez CreditAttribution: izmeez commented@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.
Comment #223
ressa CreditAttribution: ressa at Ardea commentedWon't the latest patch need a fresh review, to clarify which patch (#177 or #220) should be used?
Comment #224
joseph.olstadRTBC 204 by David Rothstein
Comment #225
vuilI confirm that #204 works perfectly.
Comment #226
mcdruid#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.
Comment #227
mforbes CreditAttribution: mforbes commentedDespite 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.
Comment #228
joseph.olstadFor 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.
Comment #229
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1, #204 looks correct to me, we add one new function, but that's fine.
Comment #231
mcdruidThanks everyone that contributed to this over the many, many years!
Comment #232
mcdruid