Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Moving the "Administer" menu item out of the Navigation menu breaks a number of pages including /admin, /admin/content, /admin/build, etc.
Comment | File | Size | Author |
---|---|---|---|
#18 | admin_sit_5.patch | 4.9 KB | pwolanin |
#14 | admin_sit_4.patch | 4.8 KB | webernet |
#13 | admin_sit_3.patch | 7.73 KB | pwolanin |
#7 | admin_sit_2.patch | 7.44 KB | pwolanin |
#5 | admin_sit_1.patch | 5.28 KB | chx |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedYes, I've noticed this too. I think the way it is coded is a bit faulty in that it makes assumptions about the depth of the menu links which do not hold if they are moved. So, we need more robust code.
Comment #2
chx CreditAttribution: chx commentedAdmin! Bad dog! Sit!
Comment #3
pwolanin CreditAttribution: pwolanin commentedno - I think the logic is not good.
don't we need something like:
admin may not be in the 'navigation' menu, but there should only be one hook_menu defined instance of this path (right? http://api.drupal.org/api/function/_menu_navigation_links_rebuild/6).
we are only getting items at a single depth right, so there is no need for the ORDER BY. I think there is a sort later.
Comment #4
chx CreditAttribution: chx commentedI do not think there is a sort of the blocks. Let's see this version.
Comment #5
chx CreditAttribution: chx commentedUm. Maybe this.
Comment #6
pwolanin CreditAttribution: pwolanin commentedHmmm, now I remember - you want this to be blank if moved out of the navigation menu?
This is wrong no matter what:
and I still think it should be removed. The array of blocks should get indexed with 50000+weight and title and sorted like links, I think.
Comment #7
pwolanin CreditAttribution: pwolanin commentedI still don't like that solution. I think it's enough that the blocks don't show if 'admin' and the blocks are in different menus. Then you can move the blocks out of 'navigation' (or vis-versa) if you want to construct your own custom control panel. Also, changing the weight via menu module ought to affect the bloc ordering or item ordering.
how about this?
Comment #8
chx CreditAttribution: chx commentedplid checking in itself is adequate i think, no need for menu_name checking (as plid is nonzero). also admin/% path checking is not needed, whatever is under administer item is fine.
Comment #9
pwolanin CreditAttribution: pwolanin commented@chx - there is an index on (menu_name, plid) but not just on plid so I think the query would be faster leaving menu_name in the WHERE.
Comment #10
webernet CreditAttribution: webernet commentedThe admin pages now work if the 'Administer' menu tree is moved out of 'Navigation' (for example to 'Primary links'). Unfortunately the page titles (and breadcrumbs) are wrong for all of /admin* ("Home").
Also tested moving the 'Administer' menu tree under 'Create content' - everything works fine.
Comment #11
pwolanin CreditAttribution: pwolanin commented@webernet - actually the BC is right - for this reason: The menu system builds the BC based on the 'active' menu (defaults to the Navigation menu). So if you move an item out of the active menu, it will not have an active trail/BC other than "Home". Maybe we should cross-check what happens in D5, but I think it's similar.
Comment #12
pwolanin CreditAttribution: pwolanin commentedAh, apparently the titles are wrong too. However, that's all separate from this issue. Here's the issue I'd been planning to use to tackle these sorts of problems: http://drupal.org/node/155624
Comment #13
pwolanin CreditAttribution: pwolanin commentedok, I think this addresses problems with the previous patch in terms of queries and descriptions.
Comment #14
webernet CreditAttribution: webernet commentedMoving the administer menu tree around under navigation works fine.
Moving the administer menu tree out of navigation works, but the page titles are all 'Home'.
Assuming the page title issue will be addressed in another patch, this one is working properly and is ready to go.
Rerolled to remove whitespace changes which were already committed and added periods to two comments.
Comment #15
Gábor HojtsyIt would be great to document, why the 50000+ sorting weight is required. What does it need to be after?
Comment #16
pwolanin CreditAttribution: pwolanin commented@Gabor - in both places I added have this code comment:
since there is a detailed explanation in that function - do we also need a detailed explanation here?
Comment #17
Gábor HojtsyErm, a short doc would be great there.
Comment #18
pwolanin CreditAttribution: pwolanin commentedok, added additional code comments.
Comment #19
chx CreditAttribution: chx commentedAmple comments. Functionality same as the one webernet tested.
Comment #20
Gábor HojtsyThanks for the update, committed!
Comment #21
(not verified) CreditAttribution: commented