Adds hooks to track changes to {menu_links}
agentrickard - May 8, 2009 - 14:46
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | API addition, API change, Needs Documentation |
Description
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().
#1
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).
#2
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.
#3
@martidau
Off topic: Did you manage to do it? Can you post something over at http://drupal.org/project/menu_node?
#4
+1 on this.
#5
+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.
#6
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.
#7
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.
#8
@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....
#9
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.
#10
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.
#11
Updated patch corrects the hook_update_N function a little.
We still need review on where this should live, and then tests.
#12
Cross post. I need to review #9.
#13
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.
#14
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
#15
- 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
#16
The last submitted patch failed testing.
#17
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.
#18
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.
#19
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.
#20
- 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);
- Elsewhere in Drupal, we write ID instead of id -- at least in comments and PHPdoc.
- Should we write tests for this?
#21
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 themenu_link.router_pathcolumn).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.#22
@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.
#23
@agentrickard: yes, sorry, scratch #21 completely, I should not try to do several things at the same time.
#24
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.
#25
Minor patch revision, in light of #20.
Now for tests....
#26
Revised with one set of tests.
Any ideas for how to test the three new hook implementations?
#27
The last submitted patch failed testing.
#28
I suppose tests have to be packaged separately?
#29
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.
#30
The last submitted patch failed testing.
#31
Testbot is wrong. Patch applies.
Now with API docs.
#32
Bah. Left in a debug message.
#33
The last submitted patch failed testing.
#34
One more time, robot overlord!
#35
The last submitted patch failed testing.
#36
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.
#37
The last submitted patch failed testing.
#38
Yea. testbot is just f'ing wrong.
/me goes to do something more productive.
#39
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.
#40
@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.
#41
Bumping back to testbot.
#42
The last submitted patch failed testing.
#43
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?
#44
Any function relating to a menu link should use "link" not "item" (which is ambiguous and should be removed where-ever possible).
#45
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
#46
this seems to make changes to menu_lnk_maintain that I thought were already in another patch
#47
@pwolanin
I merged it here, since that issue had been marked 'fixed' once and people didn't seem to want to re-open it.
#48
Aside from hook proliferation, this seems pretty reasonable
we already have a menu_link_alter hook, which is essentially the "pre-save" step.
#49
tagging
#50
Simple enough change that makes the menu link API richer. They allow potentially much broader use of the menu hierarchy functions for other hierarchies.
#51
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?
#52
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.
#53
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...
#54
There are some typos in the API docs. So will need a follow-up.
#55
Upgrade docs, yeah. We should inform developers about new APIs as well as changes to existing ones.
#56
Fixed errors in the documentation.
#57
Upgrade docs added, too --> http://drupal.org/node/224333#menu-link-hooks
#58
Committed to HEAD.
#59
Automatically closed -- issue fixed for 2 weeks with no activity.