Generated menu items vs. menu administration and weights

Going to check how this plays together with menu administration screen (it's bloody easy to reorder items in there by drag-and-drop). Later today.

OK, changing scope a bit, to avoid the need to split my patch into tiny bits...

EDIT: If you didn't read this yet, try #4 first. I attempted a full, updated, and hopefully more compact explanation there, again.

Basically, my initial patch works well in terms of mirroring weights from categories and containers to the generated menu items, and so fixing the order in menus and TOC/whatnot derived from that. What it doesn't do though, is the reverse: If someone just go to admin/build/menu-customize/navigation, and starts dragging menu items around like mad, he most probably doesn't realize neither which items are maintained by category_menu, nor the consequences.

Honestly, I never quite understood the category_menu workflow, until looking into the code. Back on 4.7.x, I was furious like hell, when I tidied up my menus for 10th time (adjusting titles and weights), then someone else fixed a few typos in body texts of categories, and half of my changes in menu were reverted to the old state, links popped up in unexpected places again... I was yelling at the buggy Drupal, buggy Category, desperate to get rid of this madness, and finally I did - by disabling the category_menu module. What I didn't realize, was that I was actually customizing two entirely different kinds of links: Normal ones, and category_menu maintained links, which are not supposed to be edited, and are overwritten completely on each edit of their node. Now on 6.x, I had no choice than to install category_menu again, as it's now a dependency for category_display (which I need), and I see the problem is still there.

We should definitely try and fix the interaction between category_menu and the core menu administration UI. My patch is an attempt on this. What it does:

- Ties the weight of categories/containers to weight of their menu items. First part is the initial tiny patch from above, adding category weight to generated menu items, and so synchronizing the two on each node save. Second part is a tiny change to the default weight on category/container node forms, where the weight from menu item is used, whenever it's available (category_menu enabled, menu item existing etc.). The idea behind this is, that it's bloody easy to reorder weights in menus (drag-and-drop and all that), so we better allow that, and merge changed weights back on later node edits (so that they are kept), while changes to weights on categories occur later in that form, so we don't blow these away too. (The change to the query in category_menu_map_load() allows us to get the current weight of menu items, by just swapping order of {menu_router} and {menu_links} tables - both have a 'weight' column, and since router went last, we got the weight from there instead of desired {menu_links}.weight.)

- It guards the maintained menu items against outside changes. This is implemented in hook_menu_link_alter(), so it runs inside menu_link_save(), making it effectively impossible to change our maintained links from *any* module, not just from menu administration UI. Unless we unlock it with ['options']['category_menu_unlock'], menu_link_save() now ignores any changes to certain properties of links owned by 'category_menu' module. A message is shown to the user (only if there was really an attempt to change the values), explaining what happened, and linking to edit of appropriate node. The use case here is, that if you try to change, for instance, expanded status (which is available directly on menu overview screen as a checkbox), the change won't be saved (while still saving other changes done), and user see an explanation immediately, with a link where to go to change the setting. That's a lot better IMO, as old behavior was saving the change, and unexpectedly blowing it away later, when the node got edited. It would be even nicer to have these options grayed out, I admit, but I don't think it's a good idea to play with theme layer for this purpose, and can't see other way to do it (plus it won't catch changes done via other modules).

- Also guards the menu administrative page for editing a single item. This is a bloody simple condition inside hook_form_alter() implementation: If you click "edit" on a maintained menu-item, instead of showing the menu item editing form (which is bound to fail on save, due to the above guard), an explaining message is set, and you're redirected to the edit form for corresponding node instead (plus back afterwards), so in the end the edit page is just swapped to another edit page, where you *can* make the changes. ( I assume that whoever have access to menu administration, have mostly the "adminster nodes" permission too, so a 403 page won't show too frequently here - even if it did, the message should still be there, and this whole is edge case multiplied by edge case, really, so I didn't waste code on that.)

- Then, there's a simple solution to #78014: Unpublished nodes (container, assigned nodes) visible in menu and related, where menu got entries even for unpublished nodes. This problem doesn't really count on 6.x, as the core menu system does various access checks, and in final result hides the unpublished node links for us. But they are still saved in {menu_links}, possibly exposed to other modules, and I also suspect the core behavior to be flawed (as the links are shown just never, not even to users who *have* access to these nodes, not even to uid=1!), so who knows what changes might happen later. All the same, I guess it's better and safer to fix this on our side too - and it's quite simple: Just mirror $node->status to $menu_item['hidden'], so that menu items for unpublished nodes are maintained as disabled. IMO this makes sense, and it's just a single line. (I tested this part carefully, with the core access check short-circuited in _menu_link_translate() - works fine).

There are still a few flaws and ideas, but I believe the patched behavior is better than what we have, already. The known problems and alternatives are:

- The merging of changed menu weights happens on node form, so category_resave (and similar) doesn't get it. Unfortunately, I seem unable to merge it at hook_nodeapi(op='load'), which would be the best place, so this might be tricky. Needless to say, that before it didn't merge too (nowhere), so this isn't any worse.

- Category administration pages (admin/content/category/ and friends) have own ways to change weights and titles on categories, so these changes are not reflected in menu. This part might be a bit hard, since the code is already quite complicated, but once again - it's not broken any more than it was, as category_menu didn't get update from there before, too. I guess there should be some kind of hook/direct call from all that admin code, where category_menu get chance to update the menu, but that's way too large task for me now.

- Alternatively, we might just lock the weight on menu administration UI in the same way as the rest. Pros: No problems with back-merging changes from menu. Cons: A lot of warning messages, plus "broken" (partially not saved) order of items, if someone "drags menu items like mad". Might be the case while dragging just neighbor links, even.

After I slept over it: Not ready yet, going to improve later today!

Also, this evolved into more of a feature request now, although it might be easily considered a (non-minor) usability bug as well.

BTW - I'm deliberately *not* putting this into the Menu Wrapper, because I want to avoid creation of more dependencies to that wrapper, where possible. (I have doubts about that wrapper, which I'm going to further investigate at #457688: Get rid of Menu wrapper, moving functionality to category_menu.)

It's always better to come back to own patches next day, gives a brand new perspective :-) I must have been blind yesterday night, to overlook so obvious things... It was too late night, for sure.

Now I have a new patch, much cleaner, and after all my testing, I'm pretty confident now. Because things changed a bit since my previous patch, going to explain again:

It ties category and menu-item weights together, as this is needed for any categories reordering to work across category->menu->derived displays (TOC and friends). Whenever a change occurs to any of the two weights, both are updated immediately - either in node_save() as the menu item get regenerated [with injected category weight], or in menu_link_save() now extended through hook_menu_link_alter() implementation [which updates the weight in {category}]. So we can reorder categories by setting weights on themselves (as nodes), and we can also reorder them as menu items - both work virtually with the same setting now. (Unlike my previous patch, there are no out-of-sync states, so I guess it's nearly bulletproof.)

As for nodes - they have no weights themselves, but if you reorder them as menu items, the order is kept now. The weight is re-saved as-is when regenerating the menu item - I only just needed to fix the menu item loading query to retrieve correct weight (swapped order of {menu_router} and {menu_links}, as both have 'weight' column, and we want {menu_links} to win), which was the only reason why the weight was cleared previously. I added a comment, so that we don't forget in future.

As for unpublished nodes (another problem of 5.x and below, now only just hidden by current core behavior), the published status is mirrored into disabled status of menu links now, so that the items make sense by themselves (whatever whoever does with them in modules / in future).

The problem of menu customizations being blown away on later node edits (as the menu links are refreshed by category_menu), which is a major source of confusion and bad reputation of this module, as far as my personal experience goes, is addressed in this way:

- First, I hope we agree on this: This behavior is by design, because category_menu is supposed to *maintain* the links mirroring their categories/containers/nodes - otherwise it won't make much sense. These links should *not* be customized, but currently we don't have even a single warning on the UI to stop the user from doing that. This leads to the later confusion, and this is what needs to be fixed.

- Menu reordering (weight changes) are now supported (as said above) - we really need this, because disabling /ignoring weights on menu overview screen would be sort-of a bomb under menu drag-and-drop.
- Other values are locked, so that the user can't edit them, by greying out the checkboxes, and swapping out the link to edit the item. This also makes the row visually different, suggesting a special behavior (which is indeed the case).
- A new edit link is added instead, pointing to edit of the corresponding node (as this is where the item *may* be changed), with HTML title explaining a bit. (I had the title much longer initially, but since line breaks don't pass in there in any way, it ended up beyond screen boundaries - so I've better shortened it.)
- The locked values are guarded in the hook_menu_link_alter() implementation, prior to saving menu items to database, so that they *really* may not be changed from anywhere apart from category_menu (a warning to the user is shown if needed). This covers all the other ways to modify links - for example the warning fires when you drag-and-drop a maintained menu item out of it's original hiearchy (we really can't deal with this at menu-item level, but changes to other menu items are still saved), or if you navigate to admin/build/menu/item/[ID]/edit manually and submit changes (only weight works on that form, but since I removed the link to the page, it's not worth the trouble to alter). If someone have modules playing with menu links and/or menu overview form, "all hell breaks loose" around ways to make changes to menu items, but the guard in hook_menu_link_alter() still covers all that (unless someone saves to database directly) :-D

The only remaining flaw is, that reordering of categories at Category administrative pages doesn't update menu items. This results in the weight being out of sync (re-saving the nodes [either by category_resave or through normal edits] fixes that, reordering the menu items may blow the changes away). But this is an already existing problem of admin code lacking hooks/calls/whatever for category_menu to make updates, and I consider it out of scope here. It's much less broken now, speaking of all the confusion around menu customizations and broken order of items in menus/TOC.

Adding a screenshot from 'naviagtion' menu admin (sorry for broken Czech localization and custom theme being used - no time left now; but I guess you see the point, still). Note the rows with greyed-out checkboxes - these are links for categories, maintained by category_menu. There's "edit node" link instead of "edit" [the menu item], with tooltip (cursor hovers over "edit node"). The light-yellow warning at the top is shown, because I just dragged the item "Společně do bazénu" out of its original hiearchy, and attempted to save that (it didn't save).

Still a few things to address...

I just discovered, that the guard-code in hook_menu_link_alter() actually fixes one more problem than I thought: The syndrome of "scattered menus" on change to menu settings on a container. There are some old issues mentioning that (#70044: Cat_menu: updating containers scatters menus everywhere for instance), and I encountered quite the same problem now in my tests.

The problem goes like this: Have a container with category_menu links enabled, and properly built for container and it's categories. Now go, and disable menu links on the container (editing the container node). When the container node edit is saved, it's menu link gets deleted (per the new setting - correct). Now, the core menu system goes, and "helpfully" re-parents all children of the deleted link to it's parent - so all categories under that container (whose links are not removed yet, as the categories were not edited with the new setting, yet), all these categories get their links moved to the root of navigation menu (assuming the container link was in there previously). This is really unpleasant result, especially because these are auto-generated links owned by category_menu module, and you can't do much with them via admin UI. On 5.x and below, this was tough to solve; on 6.x the category_resave module fixes things, fortunately.

But still, it's unpleasant to have the menus scattered after the settings got changed. Category_menu is supposed to maintain the hiearchy automatically, so this effect shows as a bug of category_menu, even if it was actually caused by core behavior.

Now the good news: This patch fixes it! The guard code in hook_menu_link_alter() runs on these more-or-less-recursive calls of menu_link_save() from inside the menu system, too - so the protection against changes applies to this auto-reparenting as well. When I save the container now, the links are not re-parented, so they have no valid parent now, and don't show anywhere (just waiting there to be removed/updated by category_menu on next re-save). Note that links owned by system module have a similar protection already (hardcoded in _menu_delete_item()), so this is nothing really new to the menu system.

The only unpleasant side-effect currently is, that the user gets warnings about the changes being disallowed, when he actually didn't do any changes to the single items explicitly. This shows that the warning in previous patch is too unreliably targetted, and I'm just dropping it now (not much of a problem).

Also, the need to clean up menu items through category_resave should be indicated to the user, somehow, to avoid further confusion. So I added a message on container edit submits, when the menu settings got changed, prompting the user to use category_resave to refresh menu data. (Container menu item re-parenting doesn't trigger this message, as it only affects the container menu item (already re-saved on the container submit).

To recap - this patch just drops one drupal_set_message(), and adds a new one elsewhere. Otherwise no changes. Work still in progress, so leaving at "needs work".

Last improvements: This patch have two additions in the category_menu.install file.

- Whenever the module is disabled, it gives a warning about generated menu links being no more maintained. Disabling category_menu is perfect way to get out-of-sync, or even create dead links (by deleting nodes without category_menu enabled), so a bit of recommendation is useful here, IMO, to further reduce possible confusion.

- Uninstall hook now improved, so that it removes also variables (this part was obviously missing), and removes also all generated links (this is new, but I think it makes sense when uninstalling the module completely - you won't be able to maintain and/or remove these links later, as they are owned by now missing module).

I'm pretty much done here, I tested all aspects of the patch I can think of, and I believe this will reduce the confusion / bad feelings around category_menu a lot.

Oops... Fixed code comment. No other changes.

Just improved an UI message logic: The reminder about the need to refresh site's data with category_resave now shows only where really needed. Previous patch did put these also to entirely new/empty containers, previews and the like. Now it only shows, if fundamental category_menu settings got changed and submitted on existing container with at least one category in it (otherwise there's nothing to refresh, as the container updated itself already as the edit got saved).

This bit of code evolved into something slightly larger than I expected for a mere UI message, but I still believe it's worth the effort: Unexpected behavior of [stale, incrementally updated] menu links after the settings got changed, that's a vast space of problems and confusion, so the user really needs to know about the need to do a resave.

The same patch rerolled, so that it applies together with my other patches currently in the queue. I moved one line, where it doesn't really matter, to avoid collision of the patches. Things are starting to be difficult, now that I maintain several patches at the same time.

The reminder message about need for category_resave is still bugging me... After all, I might want to move that into hook_nodeapi(), to avoid unreliability of the message logic after previews (and hopefully cut the overgrown code behind the message a bit).

Otherwise this patch is still fine AFAICT.

Reminder message now moved to hook_nodeapi, so that it may not fire on previews. Also means less code, and is more clean&robust. No more doubts in my mind.