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.
Because it checks paths under 'admin', not the contents of the management menu.
Comment | File | Size | Author |
---|---|---|---|
#70 | drupal8.admin-menu-root.70.patch | 7.42 KB | sun |
#68 | drupal8.admin-menu-root.68.patch | 5.98 KB | sun |
#67 | admin-menu-root.67.png | 28.77 KB | sun |
#66 | drupal8.admin-menu-root.66.patch | 6.13 KB | sun |
#65 | toolbar_596010_65_D7_management_path.patch | 6.28 KB | johnv |
Comments
Comment #1
casey CreditAttribution: casey commentedForgive me, but I don't see what you mean :p
Comment #2
yoroy CreditAttribution: yoroy commentedmarked #721328: Dashboard link no longer appears in the toolbar duplicate:
Comment #3
yoroy CreditAttribution: yoroy commentedmarked #721326: Link to Dashboard is gone now that dashboard lives at /admin a duplicate, but bumping this to critical because we should not release D7 with this kind of stuff happening.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedSo basically, the fix here is just to make the toolbar and management menu exact mirrors of each other, right? (And hopefully, that results in ripping some ugly code out of the toolbar module too.)
Doing that now would result in the "Add content" menu item showing up in the toolbar too (which as I recall, was the reason the toolbar didn't do it this way in the first place?). However, that would likely be taken out by #408160: Normal users should not see the create content link appear by default in a menu called "Management", which is another critical issue already.
In other words, I think we want:
MANAGEMENT
* Content
* Structure
* Appearance
* etc
This raises the interaction question of how a user gets to /admin (or /admin/index, or whatever it is going to become) from that menu, but the same issue exists now in the case where you have the Toolbar module enabled but not the Dashboard - I don't believe there's any good way to get to the "Administer" screen in that case except by breadcrumbs. So that part needs to be solved either way.
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedsubscribing.
Comment #6
horncologne CreditAttribution: horncologne commentedSubscribing.
Comment #7
sunThe /admin (Administer) item cannot be taken out of this menu and has to stay where it is.
For one, users without Toolbar and Dashboard and whatnot, need to have a way to access admin/ in the first place. If there is no link to /admin, you can't reach the sublinks.
Second, our menu system is fragile. Especially when it comes to translating menu router paths into menu links and deriving menu names and menu link parents. If you try to move /admin, you are opening a big, really big can of worms. See also #550254: Menu links are sometimes not properly re-parented
Off-topic for this issue, but for quite some time already, I wanted to suggest that we should rather prohibit altering menu links below /admin. I.e. lock the entire Management menu.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedPerhaps we can just leave Administer in that menu, then, and pull everything else out?
The attached patch is more or less the same patch I proposed last year at #508458: Config and modules page. The code is a bit ugly, but the code it removes is pretty ugly too :) Plus hopefully it makes some sense since we already allow router items to specify their default menu.
Issues with this:
Screenshot of patch with the Dashboard module disabled - a direct link to Administer winds up in the toolbar:
Screenshot of patch with the Dashboard module enabled - that link is replaced with a Dashboard link, as intended:
Comment #9
sunugh.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedBelieve me, I'm not in love with it either, but we need something :)
I seem to remember the problem with having the Toolbar use a separate list of links is that there would then be two competing administrative lists (Management menu and Toolbar) that do not stay in sync at all. I'm not sure the Toolbar should have its own IA that doesn't apply anywhere else.
Also, assuming that "Administer" page migrates to something more like a site index at the other issue, then it won't be as weird and won't necessarily destroy the hierarchical structure - admin/index is on the same level as everything else (even if /admin points to it as well).
The breadcrumbs aren't (yet) broken with this patch any worse than they are already broken, and in some ways are better. Are you sure it will break things in the end? The patch doesn't really do anything that a site administrator can't already do on their own with the Menu module, so if the breadcrumb system can't handle it, that seems like a bigger problem.
Comment #11
sunBreadcrumbs are based on menu links. By moving those links to the top-level of the menu, the indication of being in the administrative area of the site is completely lost.
Yes, breadcrumbs are currently broken in HEAD. However, (not so) recent changes that (more or less) went in for admin_menu, and the patch over in #576290: Breadcrumbs don't work for dynamic paths & local tasks will finally fix them.
Note that I personally like the change to move the sub-items to the top-level. However, that change would be yet another hack, with the sole purpose of supporting one of the new UX features. Effectively breaking other valid use-cases for a natural, hierarchical menu link tree structure below /admin in contrib and Drupal distributions.
At the very least, the "Administer" item would have to move to the Navigation menu. (And Dashboard would have to hook_menu_alter it into the Management menu, or ideally, rather, remove that entire bad hack of overriding an existing, fundamental core menu item)
Comment #12
sunDue to #550254: Menu links are sometimes not properly re-parented, you need to run the following query on an existing site to see the result:
New installs work just fine.
Comment #14
catchYes I've been saying this for months now, using the menu system for those links only causes countless issues and workarounds.
Comment #15
sun#12: drupal.management-menu.12.patch queued for re-testing.
Comment #17
sun#12: drupal.management-menu.12.patch queued for re-testing.
Comment #18
Gábor Hojtsy@sun: your approach to move the actual items to display to the management menu instead of the parent admin path looks reasonble. It lacks a way to go to the admin overview/dashboard page though. This might not be an issue in the toolbar but it will be for users without that attempting to use the management menu block, right?
Comment #19
sunOnly if you choose to hide the Navigation menu. To clarify, the new flow is:
1) Click "Administer" in Navigation.
2) See "Management" popping up (if that menu block is enabled).
3) Click further through "Management".
Comment #20
Bojhan CreditAttribution: Bojhan commented@sun Can you provide some images please :)
Comment #21
Gábor Hojtsy@sun: how does this approach solve the missing Dashboard link in the toolbar that was the starter problem in this issue?
Comment #22
sunSee #11:
With this patch, we can remove that ugly hack, and just make Dashboard use admin/dashboard. Display in the toolbar as "Dashboard", displayed in the regular Management menu block as "Dashboard", and there's no strange, unpredictable, or hard to account for behavior in core.
People can click "Administer" and get the usual administration page.
People can click "Dashboard" (most likely in the Toolbar) and get the dashboard.
Comment #23
Gábor Hojtsy@sun: yes, we added the dashboard in that way originally and some people insisted on overriding the main admin page instead. I thought dashboard can be a standalone thing and not the admin index, but that was ruled out for some reason. I've reopened #652122: Fix dashboard as the default /admin local task for a rollback. Seeing you've also supported the rollback previously so I've re-enumerated the reasons for rolling it back.
Comment #24
sunRight. So this patch should be RTBC.
Comment #25
Bojhan CreditAttribution: Bojhan commented@sun Can't set it RTBC without images :)
Comment #26
sunhttp://drupal.org/patch/apply
Comment #27
aspilicious CreditAttribution: aspilicious commentedhahaha, sun that is not nice ;).
But I'll apply it and post some screenshots!
Comment #28
aspilicious CreditAttribution: aspilicious commentedDo I need patch in #652122: Fix dashboard as the default /admin local task
Because I get the screenshot as result and I don't think that is a good thing...
Comment #29
sunThe actual change for Toolbar wasn't contained yet, right.
The "Dashboard" link may appear at a wrong position (not first), but that will be fixed in #652122: Fix dashboard as the default /admin local task
Comment #30
Bojhan CreditAttribution: Bojhan commented@sun No screenshot, still.
Comment #31
aspilicious CreditAttribution: aspilicious commentedCause I am not able to show a decent one, need a fresh install... (again).
I'll be back soon.
Comment #32
aspilicious CreditAttribution: aspilicious commentedWell it wasn't my fault...
Patch isn't working...
I can't see the dashboard link AND everything is in the wrong order.
(Or am I doing something wrong :( )
Comment #33
sunDid you apply the patch before or after installing Drupal? If after, then you need to force the menu system to rebuild administrative links by executing the query in #12 and rebuilding the menu afterwards.
Comment #34
aspilicious CreditAttribution: aspilicious commentedI did it *before* the install.
First I did it after, I wasn't sure about the result, so I cleared my localhost version, did a fresh checkout, applied the patch, installed drupal again.
I'll do it once more just to confirm my results...
Comment #35
aspilicious CreditAttribution: aspilicious commentedTriple checked, patch isn't working
Comment #36
Scott Reynolds CreditAttribution: Scott Reynolds commentedAnd as mentioned previously things are out of order.
Comment #37
Scott Reynolds CreditAttribution: Scott Reynolds commentedForgot to mark CNW
Comment #38
Scott Reynolds CreditAttribution: Scott Reynolds commentedThe reason why its failing to show the dashboard link its because the $item['link']['hidden'] = -1 for admin/dashboard.
Comment #39
Scott Reynolds CreditAttribution: Scott Reynolds commentedAnd this is because dashboard_menu_alter() makes it the default local task for admin/
Which using the menu system, it then will not pass the bit mask check for visible menu item and thus it won't show up.
This is solved with this additional patch. I have mucked up my Drupal git so I need to reset it from the start. But this makes the Dashboard item show up.
https://gist.github.com/ff5c6ad993d5c6c18032
Comment #40
aspilicious CreditAttribution: aspilicious commentedWhere is the find content in the shortcut bar?
Why are the items in a new order?
Comment #41
sunAlright, postponing on #652122: Fix dashboard as the default /admin local task
Sorry for not realizing this hard dependency earlier.
Comment #42
Bojhan CreditAttribution: Bojhan commentedSo I discussed this in length with tha_sun and although the technical difficulties are immense in any other way.
We broke apart Administrative links into "Management" and Site links into "Navigation" - because testing showed that these blended together created a lot of confusion. This patch breaks this concept, by putting an administrative link into Management. Arguably its only 1 link, however the problem that it causes could be greater - since it destroys the mental model of Navigation being for the anonymous visitor.
Comment #43
YesCT CreditAttribution: YesCT commented#41 says this is postponed waiting on #652122: Fix dashboard as the default /admin local task, but 652122 says that it can be fixed by this issue.
so it seems 652122 is waiting on this... like circular logic?
Comment #44
yoroy CreditAttribution: yoroy commentedUnpostponing this, just because we have to revisit this, rather sooner than later.
I think the original reasons for postponing this should be pointed out (is that in #39?), then decide again which issue needs fixing first.
Comment #45
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm not sure what remains to be done here, when the two other issues will get in:
I'm not sure this one qualifies as a critical bug. The toolbar is generated from the elements below the 'Administer' link. This is by design. We could change the Management menu (as previous patch in this issue do), so that all the links are top-level, but the current behavior doesn't feel like a critical bug to me.
Downgrading to major.
Comment #46
yoroy CreditAttribution: yoroy commentedWhat needs to be done is that the link to the 'Dashboard' needs to be put back into the toolbar.
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commented@yoroy: the link in the toolbar should be handled by #652122: Fix dashboard as the default /admin local task
Comment #48
arianek CreditAttribution: arianek commentedsubscribing so i can try and catch when this gets fixed and update the text/screenshots here http://drupal.org/handbook/modules/toolbar, since we didn't realize this was a bug until i started asking around why there was no way to get to the dashboard...
if someone knows for certain what menu title will be in the toolbar, what permissions it'll depend on, and has a screenshot, i'd much appreciate that info being posted to http://drupal.org/node/652304 so the associated handbook docs issue can get re-closed. thx!
Comment #49
sunRe-rolled against HEAD.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedadmin/content and admin/help were missing from the patch, so I added those in.
Comment #53
sun.core CreditAttribution: sun.core commented#51: drupal.management-menu.51.patch queued for re-testing.
Comment #55
geerlingguy CreditAttribution: geerlingguy commentedI just upgraded from a D6 to a D7 site, and as I stated in #865702: Broken admin pages/links after upgrade if "Administer" was not in the "Navigation" menu, there are no links at all (besides the "Hello username" and "Log out" links) in the top toolbar. Shortcuts seem to work properly. Is this problem related to this issue?
Comment #56
virgo CreditAttribution: virgo commentedHavin same problem as #55 comment, any info ?
Comment #57
siliconmeadow CreditAttribution: siliconmeadow commented@geerlingguy & @virgo: I have had the same problem. I've gone through this whole thread in detail and found that cleaning the menu router table as per #12 above solved my problem. I'll have a look around further and see why that's not executed during update.php. Might need a patch to the menu module's install file?
Comment #58
sunMeh. Everyone will have to live and work with this mess for the next couple of years.
Comment #59
quicksketchI think Damien's comment in #45 sum this up quite nicely. I'm going through issues re-categorizing them now that major and critical bugs are actually preventing development of Drupal 8 (See #1050616: Figure out backport workflow from Drupal 8 to Drupal 7). This is actually more of a functionality change than fixing a bug, as the actual bugs have already been addressed (other than the item sun mentions in #12, which is part of the critical bug #550254: Menu links are sometimes not properly re-parented). Please re-re-categorize if needed.
Comment #60
sunWith #594660: Rename default menu names, the 'management' menu actually turns into the 'admin' menu, typically containing all links to paths that begin with 'admin/*'. That patch is quasi-RTBC, just needs someone other than me to actually mark it as that.
Given that, this patch here makes even more sense. To recap:
Deal?
Comment #61
Bojhan CreditAttribution: Bojhan commentedYup!
Comment #62
johnvAttached is a recent D7-version of this patch. I'll try and create a D8-version, too.
sun's comment #12 is still valid:
@#60: on my D7-test, removing the '/admin/' path from management, moves it to 'navigation', not 'account'.
Comment #63
johnvThis is the D8 version.
Comment #65
johnvThis is a better D7 version.
Comment #66
sun@johnv: Please do not post D7 patches. This change will not get backported to D7.
Attached patch implements #60. Includes additional diff context lines to ease patch reviews.
Comment #67
sun#66 works as expected:
However, we'll have to enhance/fix the
system_admin_menu_block()
page callback that is used to produce the content for the admin/index + admin/tasks pages. With this patch, those pages output "You do not have any administrative items." The cause for this is thatsystem_admin_menu_block()
usesmenu_get_item()
to retrieve the menu_name of the listing page and dynamically creates the page output based on that router item/menu link. Since the menu_name is different now, it does not find any child links.Comment #68
sunAttached patch additionally adjusts
system_admin_menu_block_page()
to optionally accept a custom menu router $item that is used instead ofmenu_get_item()
for building the list of links in the page content.Comment #70
sunUgh. Life is full of surprises :-/
By moving Administration into a different menu, the resulting breadcrumb is Home » Structure » Taxonomy. That's obviously not what we want. :(
I see two possible ways out:
A) Dynamically inject it into the breadcrumb via System module on all /admin/* paths.
B) Rewrite the menu, menu links, and breadcrumb subsystems in order to allow a menu to be hierarchically located as a child of another menu link. I.e.: The menu link for /admin has a child "link" tree that is the 'admin' menu. Or in other words: Nested menus.
Since B) obviously is a major undertaking, attached patch implements A).
Comment #85
quietone CreditAttribution: quietone at PreviousNext commentedThere hasn't been any discussion here in 11 years. I don't work on the menu system but I will go out on a limb and say that the changes in the last 11 years make this outdated. If there is still things to do here then a new issue should be opened.
Thanks