Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
splitting this off from http://drupal.org/node/144753 to make a patch that just deals with menu.inc and other parts of the menu system that are separate from menu module, but which will be required for menu module to work.
Comment | File | Size | Author |
---|---|---|---|
#50 | menu_link_del_1.patch | 786 bytes | pwolanin |
#44 | menu_inc_reparent_19.patch | 69.36 KB | pwolanin |
#36 | menu_reparent_1.patch | 67.92 KB | chx |
#35 | menu_reparent_0.patch | 68.8 KB | chx |
#33 | menu_inc_reparent_18.patch | 68.94 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedhere's the minimal menu.inc part of the starting patch form the other issue.
Comment #2
pwolanin CreditAttribution: pwolanin commentedThis patch is totally untested, but the re-parenting code is essentially done I think.
Comment #3
pwolanin CreditAttribution: pwolanin commented2 thoughts:
1) should function menu_get_item_by_mlid() be renamed menu_link_load()?
2) There should probably be an API function menu_link_delete().
Comment #4
pwolanin CreditAttribution: pwolanin commentedOk, starting into some good stuff- new API function: menu_tree_all_data()
Also, put back in the normal install the cache_menu table, and start fixing caching code.
Comment #5
pwolanin CreditAttribution: pwolanin commentedfixed some bugs in reparenting code
Comment #6
chx CreditAttribution: chx commentedWhy do we need menu_tree_all_data here? I do not think it's needed.
Comment #7
pwolanin CreditAttribution: pwolanin commented@chx - menu_tree_data() returns a tree based on the current page (maybe we should rename this function). It only returns the visible tree.
In contrast, the function I just wrote, returns the full tree for any menu. It's not impelmented yet, but there will be a flag for skipping the access check- I think this is what is needed for the admin interface for menu module. A user who can administer menus can re-arrange all the links, even if she cannot access the link, right?
Also - this too is not implemented yet - both of these two functions should cache the result before running _menu_link_translate() on each item. I think this is the only sane way to cache, since we can't possibly follow all the changes that might alter access to an item (for example some one using taxonomy_access changes the permissions associated with a term).
Comment #8
chx CreditAttribution: chx commentedmenu_tree_all_data IMNSHO belongs to menu module -- I prefer writing a query instead of trying to provide a query for every possible occassion. menu module uses pager_query, anyways.
Comment #9
pwolanin CreditAttribution: pwolanin commentedI think we'll need this query for book module, so at the least I'd like to plan ahead with an appropriate API function rather than having book module manipulate/query the menus directly.
In what context does menu.module use a pager_query?
Comment #10
pwolanin CreditAttribution: pwolanin commentedPer discussion with Karoly on IRC:
1) menu and menu link items are always arrays, never objects
2) a few functions renamed - e.g. menu_get_item_by_mlid() becomes menu_link_load()
3) preparing for caching by moving access checks to the point after the menu tree is build
Comment #11
pwolanin CreditAttribution: pwolanin commentedTesting this code:
-Seems to work to reparent menu links.
-Basic caching of several data structures also implemented.
-Also, restored some functionality for primary and secondary links (of course you'll need the menu module to take easy advantage of this).
Comment #12
pwolanin CreditAttribution: pwolanin commentedre-rolling patch to account for this commit: http://drupal.org/node/140218
Comment #13
pwolanin CreditAttribution: pwolanin commentedprevious patch missed some code in sysyem.admin.inc
Comment #14
pwolanin CreditAttribution: pwolanin commentedok, another informative IRC w/ chx: we need some additional changes to enable menu.module.
in particular, one should always be able to do:
right now this may not work since href and link_title may be overwritten during _menu_link_translate().
solution- keep the original values. change DB column back to link_path from href. href is generated by _translate() as is title from link_title (possibly via localization code).
Comment #15
pwolanin CreditAttribution: pwolanin commentedAnother follow up to chx regarding the addition of caching:
real quick benchmarking with ab -n100, mysql query cache disabled, hitting: http://127.0.0.1/drupal6/admin/settings/logging/dblog
(after allowing anonymous users all sorts of permissions) - this is one of the deeper menu links in the the navigation menu.
Requests per second: 5.97 [#/sec] (mean), or with -c5, Requests per second: 11.66 [#/sec] (mean)
After commenting out the caching code in function menu_tree_page_data()
Requests per second: 5.54 [#/sec] (mean), or with -c5, Requests per second: 10.72 [#/sec] (mean)
So the difference is not huge, but certainly measurable for this page, and probably more significant for a page with several menus, more links, etc.
Comment #16
pwolanin CreditAttribution: pwolanin commentedupdated patch- fixes some code style issues. Includes new code for part of the 5.x -> 6.x update (the simple part).
Comment #17
pwolanin CreditAttribution: pwolanin commentedre-rolled to keep up with HEAD.
Comment #18
pwolanin CreditAttribution: pwolanin commentedper conversation with Josh K on IRC, minor additional feature- menu links on the path to root (i.e. in the active trail) also get the class "active-trail" on the
<a>
changes relative to the last patch mostly in function theme_menu_item_link, but also minor changes in menu_tree_all_data, and menu_tree_page_data to insure that the current link always gets this class.
(note for code reviewers - a lot of the new functionality in the patch is enabling for menu module and ideally for book module, but has no UI here. You can, for example, run PHP in a node and use menu_link_load() and menu_link_save() to test out the reparenting code. i.e. to move items around in the navigation menu, or to add items to it or to menu_name == 'primary-links').
Comment #19
pwolanin CreditAttribution: pwolanin commentedwith patch...
Comment #20
chx CreditAttribution: chx commentedmenu_link_save had a broken query:
$parent = db_fetch_array(db_query("SELECT * FROM {menu_links} WHERE menu_name = '%s' AND mlid = %d", $item['plid']));
I added the missing $menu_name. menu_flip_item works in menu.module. As I go forward with the module I will post more fixes if they are needed.Comment #21
pwolanin CreditAttribution: pwolanin commented@chx - thanks
Minor change- this patch moves around the "active-trail" class to put it on the
<li>
. Also renames in code the menu link element $item["path_to_root"] ro $item["in_active_trail"] for greater ease of understanding (with an existing DB, run menu_cache_clear_all() or menu_rebuild() to freshen cached data).Comment #22
pwolanin CreditAttribution: pwolanin commentedvery minor change- put new parameter to theme function last so it won't break existing custom theme functions.
Comment #23
pwolanin CreditAttribution: pwolanin commentedoops- unlucky patch #13 doesn't include the part of the patch for menu.inc. Use this one instead.
Comment #24
pwolanin CreditAttribution: pwolanin commentedneed to deal with menu code in random places, such as here:
http://api.drupal.org/api/HEAD/function/drupal_not_found
http://api.drupal.org/api/HEAD/function/drupal_access_denied
Comment #25
pwolanin CreditAttribution: pwolanin commentedre-rolled for new schema API. Moved table definitions to system.schema and fixed them up a little.
initial part of the 5.x->6.x update seems to work
in some functions renamed $item to $router_item to help make the code more self-explanatory and consistent.
fixed the code in common.inc for drupal_not_found and drupal_access_denied
Comment #26
pwolanin CreditAttribution: pwolanin commentedComment #27
pwolanin CreditAttribution: pwolanin commentedre-rolled patch to account for http://cvs.drupal.org/viewcvs/drupal/drupal/includes/menu.inc?r1=1.167&r...
Comment #28
pwolanin CreditAttribution: pwolanin commentedinvestigating this somewhat related bug: http://drupal.org/node/147061
try a path like node/add/xxx
Error messages indicate a bug
Comment #29
pwolanin CreditAttribution: pwolanin commentedThe error seems to be occurring, since the router path being returned is 'node/%/%' rather than 'node/add'
'node/%/%' is declared by the comment module, though I'm not sure what it's for:
The problem seems to be in node_load - it dies if we pass a string in. Maybe related to the bug here: http://drupal.org/node/146667
PHP is willing to try to access a string as an array.
I get the same error if I go to 'node/ddd' -!!!- easiest solution, node_load needs to check is_array() on $param. The same general thing happens if you go to 'user/x'. Again, PHP is willing to take the string as an array.
attached patch changes user_load and node_load. The only other solution I can think of off hand is complicating the menu system by having 2 types of router wildcards - one for numeric values, one for strings.
Attached patch also puts in the menu module schema- per request form Karoly so that this patch doesn't collide with his patch for menu module.
Comment #30
chx CreditAttribution: chx commentedthis was
array_slice($parts
which limits the number of params needlessly.Comment #31
chx CreditAttribution: chx commentedThis is everything but menu.module , that's still a separate issue. I fixed the system menu queries (hidden=0) and added default menus to menu.install -- neither this or the menu schema is strictly necessary for this patch BUT this separation let's Peter work on this, me on menu.module and Josh/Peter on outline. However, I need these small changes here so there is no collision. It causes no harm.
Comment #32
pwolanin CreditAttribution: pwolanin commentedAdded better menu_link_delete() function per IRC w/ chx. If an item is deleted, children are re-attached to that item's parent.
Also, found a couple stupid bugs in menu_link_save(): need to use array_shift not array_pop, need to assign mlid to item if existing item.
RTBC? We need this ASAP so dependent patches (menu module, book module) can have a base to work from.
Comment #33
pwolanin CreditAttribution: pwolanin commentedminor fix- 'Home' link in breadcrumb was broken due to href/link_path confusion. Thanks to Gurpartap for pointing this out.
Comment #34
chx CreditAttribution: chx commentedI believe this is now ready to get in.
Comment #35
chx CreditAttribution: chx commentedThis is frustrating, every time I roll this patch I remove the temporary block for menu module. Please stop readding it, ppl can't test the menu module patch!
Comment #36
chx CreditAttribution: chx commentedI sneaked in a user module patch inadvertly. Sorry.
Comment #37
morphir CreditAttribution: morphir commentedafter applying chx last patch, I get:
also:
the admin page disappeared (the fieldsets)
Comment #38
chx CreditAttribution: chx commentedmorphir, the error is on your end, that line is removed in my patch: - $result = db_query("SELECT * FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.href like 'admin/%' AND ml.href != 'admin/help' AND ml.depth = 2 AND ml.menu_name = 'navigation' ORDER BY p1 ASC, p2 ASC, p3 ASC");
Comment #39
morphir CreditAttribution: morphir commentedsorry, my fault. I got this right now I believe.
I get one error when creating a menu item:
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, '' was given in /home/morphir/www/HEAD/includes/menu.inc on line 377.
Comment #40
chx CreditAttribution: chx commentedyeah but creating a menu item is not this issue...
Comment #41
dmitrig01 CreditAttribution: dmitrig01 commentedIt all worked for me...
BTW, I was told to go to admin/build/modules, enable menu.module, and stop there
Comment #42
dmitrig01 CreditAttribution: dmitrig01 commentedI was told to be more specific... so here goes :)
I visited administer, then site building, then modules.
Page titles were correct every time, so was the breadcrumb.
Nav menu was right too.
This is def RTBC
Comment #43
morphir CreditAttribution: morphir commentedyes, forget my error there. This one is RTBC.
I actually get no errors anymore.
Comment #44
pwolanin CreditAttribution: pwolanin commentedminor fix for external links
Comment #45
joshk CreditAttribution: joshk commentedHmm... I followed the instructions and applied before loading a fresh HEAD. This results in tables {menu_custom}, {menu_links} and {menu_router} but no {menu}. However, there remain many queries to {menu}.
I'm not sure if the issue here is that the {menu} table should be created, or if the remaining references in menu.module need to be updated.
Comment #46
joshk CreditAttribution: joshk commentedAfter also applying http://drupal.org/node/144753 and rebuilding database, things seem to be working well.
Comment #47
chx CreditAttribution: chx commentedAs that patch does not touch the database... it can't be anything else but not installin' from scratch.
Comment #48
pwolanin CreditAttribution: pwolanin commented@joshk - don't install the menu module without applying the other patch.
Comment #49
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #50
pwolanin CreditAttribution: pwolanin commentedfabulous, thanks!
Of course, now that I'm starting to use this API for book module, I found a minor bug in the code to delete a menu link. See attached patch.
Comment #51
Dries CreditAttribution: Dries commentedCommitted. Thanks.
Comment #52
bjaspan CreditAttribution: bjaspan commentedsystem_update_6020() needs to be rewritten. Short form: You cannot assume that .schema files contain the right thing during update functions. Long form: http://drupal.org/node/144765#comment-245805.
I'll write a patch, but not at midnight. Since this is now a bugfix, it does not need to get done before 6/1 (right?).
Comment #53
chx CreditAttribution: chx commentedThe update is at http://drupal.org/node/147657
Comment #54
(not verified) CreditAttribution: commented