Menu weights

Argh. I figured I might miss something when splitting these smaller patches out, but I didn't think it would be something as trivial as that. Thanks for the catch/fix :)

The problem is that menus can be dynamically generated.

Most notably by the core book module (each book adds a new menu). Admittedly book.module then calls menu_set_active_menu_name() which over-rides menu_breadcrumb -- see #595280: Ability to retrieve the selected menu. -- but if you're circumventing that issue, then you potentially do want control over where newly-created book menus fit into the ordering.

And books aside, any contrib module could do similar things, so I do think that a menu weighting system needs to provide for the possibility, even if few people end up needing that functionality.

I do agree that it's a bit weird/ugly. We could potentially hide it behind a checkbox (tick the box to enable control over where the new menus will appear, otherwise put them last?) I'm not sure it's such a bad thing to force people to consider it, which is what the current approach does, but that might just be me.

Yeah, if the issue is potential confusion, then any descriptive text that helps people understand is a good thing.

I tried to cover that with "Any menus added subsequently (for example, by adding a book) will be processed at the position of the NEW MENUS GO HERE line", but I guess that could still be confusing; especially to people who are new to Drupal. Your "new menus, dynamically generated menus" line is more descriptive. Maybe we could merge that in like so:

"Any menus added subsequently (new menus, dynamically generated menus¹) will be processed at the position of the NEW MENUS GO HERE line"

With footnote:
¹ for example, by adding a book

To me "Other" implies that there might be other existing menus that aren't in the list.

I think I just tried to be really unambiguous when I wrote it :) Maybe "New menus" would be enough. We could add a #description to the menu list with additional text about this, to further explain it if necessary.

Explicitly saying that new menus will be processed at that position until menu_breadcrumb is next re-configured might help?

We need both blacklists and whitelists.

Blacklists are needed if you WANT dynamically-added menus to be processed by menu_breadcrumb.

Imagine if every time someone added a new book, you then needed to go and add that book's menu to the menu_breadcrumb whitelist!

The "---NEW MENUS GO HERE---" item is indeed representative of the zero weight, but we need it for the javascript draggable tables functionality -- without it, you have no control over what the assigned weight values actually end up being. Its purpose is to allow the weights generated by the javascript to be adjusted with respect to a known zero position, so that any subsequently-added menus will then be processed in the correct position without re-configuring menu_breadcrumb.

We could possibly look at making that javascript-only functionality. Without javascript, the weight values are visible, and hence the zero position is under manual control.

I'm happy for excess complexity to be made optional -- simpler interface by default, and the extra bits if you ask for them -- but I'd be very disappointed if this stuff was just removed. It's not that confusing. I really just think we need some sensible descriptive text to explain what it does and why.

dboulet: The dynamically-added menus are real menus. Add a book, and its menu will show up in the menu_breadcrumb menu list. But what you said is correct otherwise -- I want users to have some control over the precedence of those menus without needing to visit the menu_breadcrumb settings page every time one is added. Even assuming the user adding the book actually had permission to configure menu_breadcrumb, that would be an absurd requirement.

That could work (and I like the result if it does).

As far as catching the appearance of a new menu goes, menu_link_save() calls drupal_alter('menu_link', $item, $menu), so I guess we can implement hook_menu_link_alter() and check to see if $item['menu_name'] is known. (Does that cover all cases? I'm not 100% sure.)

Do we need to do that, though?

In _menu_breadcrumb_get_menus() I merge in any existing and previously-unknown menus, and set 'selected' => FALSE.

If instead we set 'selected' to the value of the NEW MENUS checkbox, I think we get the effect we want?

(unless I'm missing something; I've not thought this through thoroughly yet.)

It's a valid point. Sorting across multiple pages becomes an issue, though.

For paging without javascript we could turn items into hidden fields according to the current page number. We don't need to modify the form much, but the user has to ensure that they click submit on every page they modify before going to the next, so... less than perfect.

A javascript-only paging mechanism, might be better. I'm usually happy to assume that most site administrators will enable javascript. That avoids reloading the page, or making any modifications to the underlying form code, and there could be options to dynamically set the page size, or "show me everything".

You still have a real usability problem when you want to drag and drop the menu order, though. Well, you have one already if your menu list is that large, but this seems to make it worse. I can envisage some fancy solutions whereby you can select/collapse groups of menu items based on some criteria, and drag them as a single unit, but I don't want to write that code :) (although... if we made the assumption that books are the only thing really liable to cause this problem... potentially we could do just that. Book menu names all match the same pattern. We could collapse them to a single draggable item by default, and expand on request...)

For my part, I think I'd be happy to defer this to a separate issue. It seems like a can of worms that doesn't need to be opened just yet (but certainly worth looking into later).

That said... how about this as a solution?

Under a collapsed-by-default 'Advanced' settings form field, we have a text area where you can configure regular expressions for aggregating specified menu name patterns, and their 'replacement' title. By default we set this to /book-toc-\d+/Books/, but users can add their own patterns if they have a need.

_menu_breadcrumb_get_menus() filters out menu names that match these patterns, and just returns a single sub-array for each one, probably with some additional properties like 'type' => 'pattern', 'title' => $replacement_title. The array key would be the regexp.

Because that return value feeds the settings form generation, it only generates one draggable item for each pattern. It also feeds menu_breadcrumb_menu_list() which is used to process the menu_links for the current path. menu_breadcrumb_menu_list() itself wouldn't really change. The regex can be used as the key in its return value, and if we require each regex to start and end with the traditional '/' then we can recognise them as patterns.

When processing the menu_links query results, if the current $menu_name is actually a pattern, then instead of the array_key_exists() check, we check for a regular expression match against each of the menu_links.

Just delete the pattern from the settings form to revert to manual control over each book, if for some reason you really need that.

As a bonus, if you had a lot of books and wanted to treat one or two of them as special cases, you could edit the pattern to exclude those menu names, and then you'd get to order them individually from the others.

Nice one.

I wonder if checkboxes might still be nicer, if we add table headings?

Screenshot attached for comparison.

I'm working on #19.

Here's a first draft of the pattern-matching. It includes #20 and #21.

Now that we're once again only considering menus that are 'selected', we could revise some of the code that used to think that 'unselected' was also potentially important.

In particular, given the use of 'enabled' in our descriptions, perhaps we should change the internal property name from 'selected' to 'enabled'?

It would make sense with the current approach, and make the code that little bit more obvious to read.

Minor tweaks.

On my to-do list:

1. Some efficiency optimisations. I'm doing far too much work per request at present. I more or less know what I want to change, but the weather is nice today, so it may have to wait :)

2. Decide how (or if) to deal with defunct menus. At the moment, they'll remain in the persistent variable. Are there situations where deleting them from the variable automatically would be bad?

Improved version attached. Regex matching is now only done the first time a new menu is noticed, or if the settings form is submitted (in which case all menus are re-checked).

I'm still wondering whether unique weights should be enforced in _menu_breadcrumb_get_menus(). I'm not sure that it really makes a difference, but it would look a bit cleaner internally.

Re: my last comments, I'm still not doing anything with defunct 'real' menu names, but I am removing defunct patterns.

Note to self: It helps to actually test the final changes you make. Introducing bugs in your clean-up pass over the code is just embarrassing. See new attachment for fixed version.

_menu_breadcrumb_get_menus() seems quite large now.

I think breaking out the contents of the if ($unknown_menu_names) { ... } and if ($new_menus) { ... } blocks into separate functions would be sensible.

Otherwise I think I'm done for the time being :) Please do start looking this over when you have time.

I think breaking out the contents of the if ($unknown_menu_names) { ... } and if ($new_menus) { ... } blocks into separate functions would be sensible.

Done.

Note: make sure hook_uninstall removes any new persistent vars.

I'm looking at the following:

1) Changing the internal 'selected' property to 'enabled' (see #24).

2) Removing defunct menus from the variable, as I can't think of a reason not to do this (see #25).

3) Adding the missing variable_del()s to hook_uninstall() (see #29).

4) Removing the 'zero' aspect of the 'zero_row' entry. I don't believe it is necessary any longer, as all new menus are now immediately noticed and processed. This means there is no particular reason for the default weight to be zero -- we'll just assign new menus whichever weight this row happens to have.

I'll rename it to something more representative of its remaining purpose (maybe 'menu_breadcrumb_default_menu').

After that, I'll set this issue status to "needs review", if no one has any objections.

edit: All done. Will do some more testing later, then upload the new patch.

n.b. You will need to variable_del('menu_breadcrumb_menus'); if you've been testing an earlier version, due to the name change for 'zero_row'.

Okay, I'm done here. I think this is good to go, so please review.

If you've used a previous version of this patch, or one of the recent -dev builds, or a patch implementing menu black/white lists, then you may have an incompatible 'menu_breadcrumb_menus' variable in your database. You can use the "Reset to defaults" submit button on the settings form at admin/settings/menu_breadcrumb to resolve this conflict (this will clear all menu_breadcrumb variables).

Thanks dboulet.

#1 - I'll look into this.

#2 - It's always been the case with this module -- if menu_breadcrumb doesn't call menu_set_active_menu_name(), then it remains set to its default value of 'navigation'. Can we safely set it to a non-existent menu name? (we can't use a real-but-non-matching menu -- aside from anything else, in edge cases there might not actually be one). I decided it was out of scope for this issue, anyhow. We don't want to disable the checkbox, because the enabled/disabled status is still relevant when we're looking for a menu to use.

#3 - D'oh.

dboulet: Well I've left #1 alone. My thinking is that because Book is a core module, it just makes more sense to handle it automatically. In reality I doubt there are many non-Book applications for the pattern matching, and I doubt there are many situations where aggregating books would not be the desirable thing to do.

If you have no books, then you won't see an entry in the table for them. You'll just see the pattern in the Advanced fieldset, if you open it. I don't want to only set that default value if the book module is enabled, because then we don't handle them by default if someone enables it later on (after we've already set a value for that variable).

I've added a fix for #3, using a copy of Drupal 7's ID attribute sanitizer (which doesn't seem to be in the API docs yet, but it's part of D7's common.inc).

I've also updated the translation .pot file using http://drupal.org/project/potx

xurizaemon: I started pondering that update function, but realised I need sleep instead...

Adding pre-patched (#35) tarball of module to help testers who aren't used to patches/CVS.

Please note the comment in #31, though -- for the moment, that still applies.

I've added in the hook_update function. I removed the code that checked for the menu_breadcrumb_default_menu array key every time, and added it if it wasn't there. I'd really only put that in because of the name change from zero_row, but this way we can check for it in the update function (which I've modified accordingly), and do fractionally less work on each request.

(Note to any new testers: This deals with #31, if you run update.php after installing.)

I added a CHANGELOG.txt file as well.

I had another thought while testing... should we aggregate the known problematic menus in a pattern, by default?
/^(admin_menu|devel)$/Known problem menus/

They would remain disabled by default, but this would make it more obvious that they shouldn't be enabled.

if someone is already confused/uncertain, documentation via regular expressions probably won't help them :)

Heh. Yes, I had some reservations along those lines myself.

Never mind. I think I'm probably more enamoured of the idea because it would have meant there was more than just one common application for that pattern matching code :) Maybe we could just throw it into the readme file as an example usage.