Normal pager doesn't work with menu
keith.smith - January 10, 2008 - 18:07
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
After updating to a new version of HEAD, dropping my test database (and recreating another one) and installing a fresh copy of head, I went to the Modules page and enabled every core module. Then I went immediately to the /admin/build/menu-customize/navigation page, and moved to the second page from the pager.
The item "Sources" (from Aggregator) appears linked to the disabled "Search", as in the screenshot. However, it you edit "Sources" it shows its parent as "Feed aggregator" (screenshot attached).
| Attachment | Size |
|---|---|
| mb1.PNG | 46.36 KB |
| mb2.PNG | 26.08 KB |
#1
Huh. I can confirm this every time I try it:
- download and install a clean copy of HEAD with new database
- enable all core modules
- go to menus, navigation menu (note that menu items are displayed on two pages)
- go to second page of menu items
- note that "Sources" (from aggregator) appears under "Search"
#2
I can reproduce with a clean install and all core optional modules enabled.
#3
I can reproduce this as well - the menu_links table appears to be OK.
Probably related to fact that it's two pages, and the child item appears on a different page from the parent.
Also -- Note that 'Recent posts' is on the second page, when it should be near the top of the menu tree.
#4
More info:
It displays more pages than are needed. To reproduce only activate 'aggregator'.
menu_overview_formis passing an incorrect count of total items displayed topager_query.The pager cuts the rows based on the query and 'sources' is affected.
#5
#6
if the data is OK, but it's displayed wrong it may not warrant marking this critical, though it may totally ruin Drag-n-drop functionality
#7
This is a *very experimental patch* (waiting chx to give an opinion) that resets all menu_links that are not customized or active.
#8
Well, it's an experimental patch that works, on first impressions at least. On a 5.x-6.x upgrade, reproduced the extra menu items, ran update.php (with no updates, but all the way through anyway), and the extra items were gone from the admin panel. I'd been messing with menu drag and drop for the other js issue, so some of the old links were 'customised' and hence not deleted, but I'll try to do some more detailed testing later.
#9
http://drupal.org/node/211359#comment-696623
#10
The isn't a duplicate. And the patch isn't right -- that simply rebuilds the menu tree, reordering the records in the table making this appear to be fixed, when in fact the source of the problem is the pager.
I believe that pager most likely takes the first n items from the menu table for page 1, whether or not they belong on page 1.
#11
I think webernet is correct about the pager (which I always had some concerns about). Prior to the drag-n-drop it was not as much of a problem since you had to go to a separate screen to edit the parent/weight.
To do this right, it really needs to be a custom version of the pager that makes sure that all the children of a parent appear on the same page with it.
#12
I think that there are two problems here:
1) The count query passed is counting all the items and not the items that end being displayed (hidden, not active).
$sql_count = "SELECT COUNT(*) FROM {menu_links} ml WHERE menu_name = '%s'";$result = pager_query($sql, 200, 0, $sql_count, $menu['menu_name']);
2) webernet is right on that the childs can be separate from parents because of the pager not knowning anything about families.
To solve 1) I would like to pass the total number of rows to pager_query with a number and we only need a menu function that counts active items.
// We calculate the total of pages as ceil(items / limit) if a number is not provided.if (is_numeric($count_query)) {
$pager_total_items[$element] = $count_query;
}
else {
$pager_total_items[$element] = db_result(db_query($count_query, $args));
}
To solve 2) yes a custom pager.
#13
Marking "needs review" so you all look at it, though this is obviously not totally polished. Seems to work and to render all the top level links in the right order. Gobor or someone want to test with localized titles?
#14
It is bad enough that we can't use the built in parent choosers because they display full menus but to display all children if a parent is visible is :( let's decide whether we want core to support big menus or not.
#15
The patch in #13 certainly solves my initial problem, and I couldn't break it with casual testing.
Minor, but if you have to re-roll for some other reason, I did notice an "admintrator" typo in a code comment.
#16
updated patch with one slight change to the code and lots of doxygen comments.
#17
here's the alternative suggested by chx - just lose the pager.
#18
Removing the pager completely sounds like the simplest and safest solution. It also fixes the issue of not being able to drag and drop over multiple pages. Hopefully the page won't get too big.
#19
You can always menu_alter this page to your contrib module which does something better or just access FALSE this page and provide a totally alternative interface. With the drilldown parent choosers moved to D7 we already decided that while the backend provides support for insane big trees, the core UI does not. Case closed.
#20
Tested OK.
#21
This patch is the same as above, but I also noticed that this form builder doesn't store in any accessible place which menu it is working with - thus added an additional line.
#22
+1 to no pager -- I found it to be annoying when moving one menu item to another page. Works better with DnD. :)
#23
Well, since even Dries says we should remove the pager, I committed #21. Better solutions for big menus will hopefully be available in contrib.
#24
Automatically closed -- issue fixed for two weeks with no activity.