| Project: | Category |
| Version: | 6.x-2.0-beta3 |
| Component: | Category_menu |
| Category: | task |
| Priority: | normal |
| Assigned: | JirkaRybka |
| Status: | closed (fixed) |
Issue Summary
I feel a bit uneasy about the menu wrapper - this mechanism of wrapper modules is a risky business: Even now it's not just "minus menu-on-the-fly functionality": Installation of the wrapper means in fact reversal of several core patches/fixes (I did a diff against core original menu.module, and there are a few differences I don't fully understand). It's going to get worse over time. It's impossible to keep in sync with core, as we don't know which exact versions are used together. It also requires an extra step (installation of the wrapper). And all that only just to remove one incompatible thing from node forms?
Unless I'm missing something, we can just use hook_form_alter() to remove the menu-item fieldset from the node form - leaving no way to make changes / create new menu items, so the other code never fires, and so never saves any changes. Even core itself shows the fieldset rarely (access is limited by 'administer menu' permission), so it's absence shouldn't hurt.
I've put into my custom module:
function my_module_form_alter(&$form, $form_state, $form_id) {
if (substr($form_id, -10) == '_node_form') {
unset($form['menu']);
}
}It seems to work just fine, not needing the menu wrapper. (Another option might be to reset the access flag for that fieldset, or replace it with some fixed values.)
Of course, if implementing this in category_menu, there's the problem with order in which hook_form_alter() implementations are called, i.e. 'category_menu' goes before 'menu' (per alphabet), and so can't alter the later menu's alterations. This is solvable by either renaming the module so that it's after 'menu', or better by setting weight for the category_menu module in {system} table, to make it fire later than core modules.
Or are there other/deeper reasons/incompatibilities, requiring the menu_nodeapi() code to be removed entirely? (If so, we might also investigate on options as unsetting $node->menu in hook_nodeapi(), given that we fire before menu module, to prevent any actions taken by menu module.)
Comments
#1
subscribe
#2
Marked #67486: menu fieldset has no effect when category_menu enabled as duplicate.
Going to look into this as time permits, probably rolling an experimental patch first (only just removing menu OTF via hooks implemented from category_menu, not getting rid of the menu wrapper yet).
#3
First step: What exactly the Menu wrapper does.
After some digging in CVS and doing diffs, I found that:
- The Menu Wrapper is indeed a verbatim of core 'menu' module, except that it removes hook_form_alter() and hook_nodeapi() implementations from menu.module entirely. Other changes are just $id$ lines and comments, plus .info file and wrapper status check - in other words these are just additions needed for the wrapper installation workflow.
- The copy of menu.module was taken from Drupal 6.8 (December 2008), so installation of the wrapper brings us back to that date with core fixes. As of today (begin of June 2009), the following fixes are reverted by installation of the menu wrapper: #326210: (Needs tests) menu_submit & mlid on menu creation, #332123: Remove t() from all schema descriptions, #328110: Recoverable fatal error: Argument 3 passed to l() must be an array, #317775: Caching entire {menu_router} table causes MySQL error/slow rebuilds and slows menu_link_save. More is likely to come with further Drupal core releases.
Now I'm going to examine, what exactly the removed hook implementations do, and how to silence it from category_menu perspective, without needing to touch menu.module. Should be possible.
#4
Second step: What the removed menu.module code does, and how to stop it without removal.
So, I examined what happens in the two hook implementations in menu.module, that menu wrapper removes:
hook_nodeapi():
insert, update:
- Deletes/updates menu item. Only acts when there's $node->menu. If $node->menu is not set, it does nothing.
delete:
- Deletes all menu links pointing to the node, and having 'menu' module as owner. Does nothing about 'category_menu' links, already.
prepare:
- Attempts to load existing menu item for the node (owned by 'menu' module, so loads nothing for 'category_menu'), and sets some defaults to $node->menu. These defaults (unless changed in the form) are not triggering any action on save (due to empty title).
hook_form_alter():
- Adds the menu OTF fieldset to node forms, with access limited to 'administer menu' permission. Keeps data from existing $node->menu in the form, and adds a new submit handler 'menu_node_form_submit' to the form.
The added menu_node_form_submit() handler:
- Only just decodes submitted form data in a simple way, by splitting selected parent item to proper values. No real action performed here.
My plan: In the end, it might be sufficient to just remove the fieldset from node forms, to avoid users confusion and creation of duplicate menu items. The menu.module code doesn't touch menu items owned by other modules, already. For extra level of safety, just unset $node->menu in our own hook_nodeapi() 'presave' op (which is invoked at the begin of
node_save()) - then, menu module does *nothing* on node_save().Further discussion:
$node->menu being not set is entirely possible (it happens on all node_save() operations invoked without the node being edited through the node form, and so without op 'prepare' firing), so I'm pretty confident this is safe. With 'insert'/'update' ops silenced this way, we're done, as 'delete' does nothing in our case, and 'prepare' may prepare whatever - we're going to hide the form elements anyway, and unset $node->menu later in 'presave'.
It might be good idea to unset the resulting $node->menu in our own op 'prepare', just in case, but then we risk warnings if other modules try to access $node->menu in their hook_form_alter() implementations (this applies to the current menu wrapper too, as it removes the source of $node->menu entirely). We might slip in a copy of our own menu item, but I don't think it's a good idea to expose that to other modules on node forms. So in the end, I guess we might want to just set $node->menu to the empty default (which is most probably in there already), but it doesn't really hurt to leave a possibly pre-existing (menu module's) menu item in $node->menu - it's unlikely that there's one, and it's not going to be saved on node_save() anyway. (If there was such an item, then we have a duplicate in menu already. That's out of scope here.) So, no need to do anything with 'prepare'.
As for removal of the menu OTF fieldset, that's a simple hook_form_alter() stuff, after we set category_menu module to a positive weight in {system} table, so that it's hooks fire *after* core (otherwise we go before menu module, per alphabet). This is commonly used in contrib - pretty safe. Category_menu already went after category, so no risk in this interaction, with the weight changed.
EDIT: Now I see that category_menu have weight set to 8 already in hook_install(). But it wasn't present in 5.x, so we need to add hook_update_N to set the weight on existing sites (unless I missed something).
Going to roll a patch later today. Next part: Get rid of the menu wrapper (plus add hook_update_N).
#5
Here's a very basic initial patch: Forces menu module to never save any link on node_save() [I tested, it works], removes any menu-OTF information prepared for node forms (finally I put this in, as it's essentially for free code-wise, and in line with current menu wrapper behavior), and removes the menu OTF fieldset from node forms (assuming category_menu have weight set properly).
It doesn't do anything with category_menu module weight yet, and doesn't remove the menu wrapper. For testing this, uninstall the menu wrapper! This patch should replace it's "functionality".
Further work in progress,
#6
So, this is the whole trick in category_menu module - now with weight update added, the menu-on-the-fly removal works just fine by just enabling the category_menu module (and comes back by disabling it, BTW). No need to install Menu wrapper now.
To Jaza: Am I correct? Is this going to be sufficient? Or did I miss something?
Next step: Get rid of Menu wrapper. Work in progress.
#7
Well, here we are...
Full patch now, moving the functionality to category_menu module as described above (previous patch is included in this one without changes - look at that one to see the actual implementation of that easier), and now also getting rid of the menu wrapper install/uninstall/status.
Also update is added, to uninstall and remove previously existing menu wrapper (if present). I tested the update very carefully, emulating various filesystem permissions issues, and I'm pretty confident it works right.
BTW, the patch applies independently of #481280: Generated menu items vs. menu administration and weights - there's a bit of offset if applying both, but the outcome is fine (just to assure you).
One additional thing, which needs to be done is removal of the 'menu' directory under 'wrappers', as well as all the four files inside. I really don't know how to roll file/directory removal into a patch (I suspect it's not possible even). If it's not removed - well, no real harm, but it's not really nice to keep unneeded files.
Now, we only just need a review from Jaza, to see whether this is the right approach ;-)
#8
Status change lost in Drupal.org's tonight agony of overloaded database server...
#9
Committed to HEAD. Thanks! See:
#158598: Category port for Drupal 6.x
#10
Automatically closed -- issue fixed for 2 weeks with no activity.