Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

FileSize
1.81 KB

here's the minimal menu.inc part of the starting patch form the other issue.

pwolanin’s picture

Status: Active » Needs work
FileSize
11.29 KB

This patch is totally untested, but the re-parenting code is essentially done I think.

pwolanin’s picture

2 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().

pwolanin’s picture

FileSize
19.37 KB

Ok, 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.

pwolanin’s picture

FileSize
19.59 KB

fixed some bugs in reparenting code

chx’s picture

Why do we need menu_tree_all_data here? I do not think it's needed.

pwolanin’s picture

@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).

chx’s picture

menu_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.

pwolanin’s picture

I 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?

pwolanin’s picture

FileSize
45.51 KB

Per 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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
50.13 KB

Testing 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).

pwolanin’s picture

FileSize
50.9 KB

re-rolling patch to account for this commit: http://drupal.org/node/140218

pwolanin’s picture

FileSize
52.64 KB

previous patch missed some code in sysyem.admin.inc

pwolanin’s picture

ok, another informative IRC w/ chx: we need some additional changes to enable menu.module.

in particular, one should always be able to do:

$item = menu_link_load($mlid);
// make minimal changes- e.g. new plid
menu_link_save($item);

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).

pwolanin’s picture

Another 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.

pwolanin’s picture

FileSize
60.72 KB

updated patch- fixes some code style issues. Includes new code for part of the 5.x -> 6.x update (the simple part).

pwolanin’s picture

FileSize
60.61 KB

re-rolled to keep up with HEAD.

pwolanin’s picture

per 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').

pwolanin’s picture

FileSize
60.93 KB

with patch...

chx’s picture

FileSize
60.21 KB

menu_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.

pwolanin’s picture

FileSize
61.41 KB

@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).

pwolanin’s picture

FileSize
14.02 KB

very minor change- put new parameter to theme function last so it won't break existing custom theme functions.

pwolanin’s picture

FileSize
61.41 KB

oops- unlucky patch #13 doesn't include the part of the patch for menu.inc. Use this one instead.

pwolanin’s picture

Status: Needs review » Needs work
pwolanin’s picture

FileSize
65.55 KB

re-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

pwolanin’s picture

Status: Needs work » Needs review
pwolanin’s picture

pwolanin’s picture

investigating this somewhat related bug: http://drupal.org/node/147061

try a path like node/add/xxx

Error messages indicate a bug

pwolanin’s picture

FileSize
65.95 KB

The 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:

  $items['node/%node/%'] = array(
    'title' => 'View',
    'page callback' => 'node_page_view',
    'page arguments' => array(1, 2),
    'access callback' => '_comment_view_access',
    'access arguments' => array(1, 2),
    'type' => MENU_CALLBACK,
  ); 

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.

chx’s picture

FileSize
65.94 KB
$router_item['page_arguments'] = array_merge(menu_unserialize($router_item['page_arguments'], $map), array_slice($map, $router_item['number_parts']));

this was array_slice($parts which limits the number of params needlessly.

chx’s picture

FileSize
66.2 KB

This 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.

pwolanin’s picture

FileSize
69.04 KB

Added 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.

pwolanin’s picture

FileSize
68.94 KB

minor fix- 'Home' link in breadcrumb was broken due to href/link_path confusion. Thanks to Gurpartap for pointing this out.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I believe this is now ready to get in.

chx’s picture

FileSize
68.8 KB

This 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!

chx’s picture

FileSize
67.92 KB

I sneaked in a user module patch inadvertly. Sorry.

morphir’s picture

after applying chx last patch, I get:

# user warning: Unknown column 'ml.href' in 'where clause' 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 in /home/morphir/www/HEAD/includes/database.mysql.inc on line 163.
# notice: Undefined variable: blocks in /home/morphir/www/HEAD/modules/system/system.admin.inc on line 35.
# warning: Invalid argument supplied for foreach() in /home/morphir/www/HEAD/modules/system/system.module on line 2447.

also:

the admin page disappeared (the fieldsets)

chx’s picture

morphir, 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");

morphir’s picture

sorry, 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.

chx’s picture

yeah but creating a menu item is not this issue...

dmitrig01’s picture

It all worked for me...
BTW, I was told to go to admin/build/modules, enable menu.module, and stop there

dmitrig01’s picture

I 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

morphir’s picture

yes, forget my error there. This one is RTBC.

I actually get no errors anymore.

pwolanin’s picture

FileSize
69.36 KB

minor fix for external links

joshk’s picture

Status: Reviewed & tested by the community » Needs work

Hmm... 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.

joshk’s picture

Status: Needs work » Reviewed & tested by the community

After also applying http://drupal.org/node/144753 and rebuilding database, things seem to be working well.

chx’s picture

As that patch does not touch the database... it can't be anything else but not installin' from scratch.

pwolanin’s picture

@joshk - don't install the menu module without applying the other patch.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

pwolanin’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
786 bytes

fabulous, 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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

bjaspan’s picture

Status: Fixed » Needs work

system_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?).

chx’s picture

Status: Needs work » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)