We had a client who wanted the site's menu system to be used as the sole organizing principle. To allow editorial access based on menu items, however, we had to figure out how to write JOINs from the {menu_links} table to the {node} table, which is currently not supported.

We created the Menu Node API to address this problem. However, it works mainly by altering and watching form submissions, since there is no direct menu save hook (something similar to hook_nodeapi).

Would having a {menu_node} table in core be useful? If so, we can craft a patch to allow the maintenance of this table without hook_form_alter().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sime’s picture

I think this is a useful contrib module, I have this request a lot.

I agree it's difficult that you can't see where links were created (either from menu admin or the node form), this would also solve that problem (unless I've missed something about the menu system).

martidau’s picture

The ability to join the menu_links table from a view was one of the first things I needed to do after using drupal for 2 days. Should definitely be core functionality.

agentrickard’s picture

@martidau

Off topic: Did you manage to do it? Can you post something over at http://drupal.org/project/menu_node?

Mac Clemmens’s picture

+1 on this.

ronan’s picture

+1 from me also. I've implemented the same functionality in the Node Hierarchy module. I'm sure these aren't the only two implementations of this in contrib.

agentrickard’s picture

Status: Active » Needs review
FileSize
5.15 KB

Here's a patch to get started with. Requires running update.php.

I put the code into menu.inc, on the theory that you can create node menu items without menu module turned on. If this is a false assumption, then we simply move the code into menu.module.

This feature should do the following:

-- CRUD storage of the {menu_node} table, storing MLID and NID.
-- Fire internal hooks notifying modules of menu node changes.

These hooks are currently menu_node_insert($item, $node), menu_node_update($item, $node) and menu_node_delete($item, $node).

TODO:

-- Tests.
-- Clean up the update hook.
-- Decide on hook names.
-- Decide if this belongs in menu.inc or menu.module.

sun’s picture

Why can't you use http://api.drupal.org/api/function/hook_menu_link_alter/7 in your module?

If at all, this is node-related, and thereby should go into node.module.

agentrickard’s picture

@sun

Hmmmm. That is an excellent point and may work in the D6 version so I can ignore all the form_alter nonsense currently used in Menu Node API.

That's why the extra eyeballs are a good thing.

But hook_menu_links_alter() only works for save. What about delete? I suppose we can get that in hook_nodeapi() and hook_node_delete().

/me thinks about the re-roll....

sun’s picture

Status: Needs review » Needs work

Well, yes, that part of the menu system is still a bit confusing.

We have http://api.drupal.org/api/function/menu_link_maintain/7, but that's only ever called from aggregator.module. Additionally, my first assumption on this function was that it's rather a hook, or it (also) invokes a hook that allows other modules to perform additional actions whenever a menu link is altered. But no-go. That part we have to improve for sure.

agentrickard’s picture

Status: Needs work » Needs review

Well, I started looking at moving this into the node module. The problem is that you can create and delete menu items that point to nodes from _outside_ the node module. (And, in fact, that logic is currently handled by menu.module using node hooks.)

For example, if you delete an entire menu, then the {menu_node} table will not get updated because hook_menu_item_alter() doesn't fire on delete, just on save. (Which is why the D6 module uses hook_form_alter() to watch multiple forms).

I also would prefer to fire a menu_node specific hook, and don't really want to do that inside hook_menu_item_alter().

I think we have to place this as high up in the menu call stack as possible, which is why I originally targeted menu.inc.

So I think this has to stay in menu.inc or menu.module.

agentrickard’s picture

FileSize
5.49 KB

Updated patch corrects the hook_update_N function a little.

We still need review on where this should live, and then tests.

agentrickard’s picture

Status: Needs review » Needs work

Cross post. I need to review #9.

agentrickard’s picture

OK, I read #9 and menu_link_maintain() is just an odd function. It looks like someone wrote it for general use and then it sits unused. Technically, I think menu.module should be using it for its current node handling, but it doesn't.

Perhaps we need a more generic hook in menu.inc that is then implemented in node.module or menu.module. The hook would respond to menu link CRUD.

The issue there is that, for this API to work properly, we need all the information about the link before it is actually deleted.

agentrickard’s picture

Title: Create {menu_node} tracking table? » Track foreign keys in {menu_links}
Status: Needs work » Needs review
FileSize
5.88 KB

After a discussiuon with sun in IRC, we decided to try tracking the object's foreign keys directly in the {menu_links} table. This approach will let us run direct JOINs on that table, and treats other items as first-class objects (not just nodes).

To test:

-- Run update.php to update {menu_links}.
-- Add some node-based menu items

Since menu items can be created from the menu module and from nodes, we need to declare a hook_menu_object() function to derive the object type. Otherwise, we can pass the object data when calling menu_link_save() directly from a module.

The patch also introduces hook_menu_item_insert() hook_menu_item_update() and hook_menu_item_delete().

TODO:

-- Tests
-- Logic checking
-- Conversion of other menu_link_save() calls to add object data. (Though node may be the only one right now.)
-- Documentation

Damien Tournoud’s picture

- hook_menu_object() seems unneeded. Considering a wildcard loader like '%node', 'node' is the object type, and its value in the path is the object id
- hook_menu_item_insert() and hook_menu_item_update() looks like scope creep, especially because they are not used in the current patch

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

hook_menu_link_save() or hook_menu_link_maintain() is the part of this patch we want to backport to D6, if possible. As agentrickard mentioned, modules have to hack their way through all possible forms that may lead to a menu link update to track potential updates.

agentrickard’s picture

I agree that hook_menu_object() is not needed -- we can do this in hook_menu_link_alter() instead. But I don't think we can deduce the object from the router path reliably. (We can for core, but since modules can alter the router path, that makes the whole mess unstable.)

But I can roll a version that reads the router path.

I actually have use-cases in contrib that use the hook_menu_item_update() functions, which are currently very hard to implement.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
7.11 KB

Here is a patch that reads the menu object type and id based on the router_path and its associated loader function. Note, however, that this deliberately fails for menu item with multiple loaders.

TODO:

-- Tests
-- What to do (if anything) about multiple loader arguments.

Dries’s picture

- I agree that menu_link_maintain() is odd -- maybe it needs to be reconsidered?

-

+    // Notify modules we have acted on a menu item.
+    ($existing_item) ? $hook = 'menu_item_update' : $hook = 'menu_item_insert';
+    module_invoke_all($hook, $item);

It is cleaner and more readable to write that as a regular if-else.

- Elsewhere in Drupal, we write ID instead of id -- at least in comments and PHPdoc.

- Should we write tests for this?

Damien Tournoud’s picture

Status: Needs review » Needs work

Storing the object_type and object_id in the menu link is really odd. Those belong to the menu router (and there is direct relation from the menu link to a menu router via the menu_link.router_path column).

The information about the object_type is already in the menu_router table by the way (in the load_functions serialized blob), but it makes sense to make that a proper column if we want to be able to join on this.

agentrickard’s picture

@Damien

I see no use-case currently where someone would want to query for the object type of a router item. What we want is information about the items displayed by the menus, which is stored in {menu_links}.

The entire point to the patch is to be able to write efficient JOINs against {menu_links} directly. I suppose we could store this data in {menu_router}, but that adds a layer of indirection that we may not want, particularly when using modules like Views to write queries.

Since we cannot store the object id in the router table, it makes sense to me (from an optimization standpoint) to store the object type directly in {menu_links}, which is where we need to run the queries. Compare:

SELECT p1 FROM {menu_links} WHERE object_type = 'node' AND object_id = 20;
SELECT ml.p1 FROM {menu_links} ml INNER JOIN {menu_router} mr ON ml.router_path = mr.path WHERE mr.object_type = 'node' AND ml.object_id = 20;

The primary use-case of this patch is to allow Views of menu items and to allow Node Access systems to leverage the menu hierarchy.

Damien Tournoud’s picture

@agentrickard: yes, sorry, scratch #21 completely, I should not try to do several things at the same time.

agentrickard’s picture

Not a problem.

What do people think about the problem of multiple loader functions? Is taking the first match acceptable?

And, yes, we should write some tests that cover:

-- Do the three hooks fire as expected?
-- Are node items added to the menu identified correctly?
-- Are taxonomy term paths added to the menu identified correctly?

That should be enough, I suppose.

agentrickard’s picture

FileSize
7.13 KB

Minor patch revision, in light of #20.

Now for tests....

agentrickard’s picture

Status: Needs work » Needs review
FileSize
10.29 KB

Revised with one set of tests.

Any ideas for how to test the three new hook implementations?

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

I suppose tests have to be packaged separately?

agentrickard’s picture

Status: Needs work » Needs review
FileSize
11.43 KB

Nope, there was a change.

Here is the current patch, with tests, but the update test fails because menu_link_maintain is inconsistent.

#423690: menu_link_maintain(), $op update critically broken.

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
14.08 KB

Testbot is wrong. Patch applies.

Now with API docs.

agentrickard’s picture

FileSize
14.06 KB

Bah. Left in a debug message.

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
14.1 KB

One more time, robot overlord!

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
15.71 KB
14.13 KB

Two versions of the revised patch.

The first is just this issue.
The second incorporates the latest patch against #423690: menu_link_maintain(), $op update critically broken.

Who knows how testbot will respond, but all tests pass on my dev.

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Yea. testbot is just f'ing wrong.

/me goes to do something more productive.

pwolanin’s picture

Status: Needs work » Needs review

I see possible good and bad points here - the good might include getting away fromt he current hack of parsing th epaths to find node links for an optimized access check.

agentrickard’s picture

@pwolanin

If we have to scale back the patch, I would argue for the 'hook_menu_item_X' elements of the patch. Having those will allow me to do the object tracking separately. (In D6 I have to hook_form_alter() something like 5 forms to track the data I need.)

The original intent was for tracking nodes for use by node access modules and Views, so that we can use the menu system as a site hierarchy. I have no vision of this patch in terms of optimizing the menu system itself.

agentrickard’s picture

Bumping back to testbot.

Status: Needs review » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Title: Track foreign keys in {menu_links} » Adds hooks to track changes to {menu_links}
Status: Needs work » Needs review
FileSize
7.75 KB

OK. I have backed off making any changes to the {menu_links} table and storing the menu object type in the db.

This is a more limited patch which does 4 things:

1) Introduces hook_menu_item_insert()
2) Introduces hook_menu_item_update()
3) Introduces hook_menu_item_delete()
4) Cleans up menu_link_maintain to use menu_link_save() on the update op. (See http://drupal.org/node/423690#comment-1775258 for reference.)

I would _really_ like to see this get in..... Tests all pass. There is API documentation.

What else do we need, if anything?

pwolanin’s picture

Status: Needs review » Needs work

Any function relating to a menu link should use "link" not "item" (which is ambiguous and should be removed where-ever possible).

agentrickard’s picture

Status: Needs work » Needs review
FileSize
7.7 KB

Re-rolled.

pwolanin and I just discussed in IRC splitting off the object tracking to a new issue, as well. But this has to go in first, IMO. #557356: Track object types for menu links

pwolanin’s picture

this seems to make changes to menu_lnk_maintain that I thought were already in another patch

agentrickard’s picture

@pwolanin

I merged it here, since that issue had been marked 'fixed' once and people didn't seem to want to re-open it.

pwolanin’s picture

Aside from hook proliferation, this seems pretty reasonable

we already have a menu_link_alter hook, which is essentially the "pre-save" step.

pwolanin’s picture

Issue tags: +API change, +API addition

tagging

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Simple enough change that makes the menu link API richer. They allow potentially much broader use of the menu hierarchy functions for other hierarchies.

agentrickard’s picture

My understanding at DrupalCON Paris was that this was held up pending some performance concerns.

If this is true, can someone explain what those might be, so I can whip up a test or two?

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Oh. I think Peter voiced some concern about how this would impact a menu_rebuild(), but those are pretty slow anyway. :P I see he's marked it RTBC despite these concerns.

I checked this over once more, and it looks good. I've run into a situation where I wanted to keep menu items and taxonomy terms in sync and this would've made it much easier. Even comes with tests. :D

Committed to HEAD. Marking needs work for documentation.

agentrickard’s picture

What other documentation do you want? The patch contains elements for menu.api.php already...

Module upgrade docs?

BTW: I think that static variable testing trick might be useful in more places...

agentrickard’s picture

There are some typos in the API docs. So will need a follow-up.

webchick’s picture

Upgrade docs, yeah. We should inform developers about new APIs as well as changes to existing ones.

agentrickard’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.3 KB

Fixed errors in the documentation.

agentrickard’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -API change, -API addition

Automatically closed -- issue fixed for 2 weeks with no activity.

Status: Closed (fixed) » Needs review

carcheky queued 56: 457450-followup.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 56: 457450-followup.patch, failed testing.

David_Rothstein’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)