Port Menu breadcrumb module to Drupal 7

Yes, I'll volunteer for the D7 port. I've studied the module, and it's quite simple. I'll try do do it this week, fingers crossed.

I commenced work on it, but got distracted by other concerns. It's still on my list of things to do!

Small port, only fixes errors, may be work's :) My first patch.

Toktik, thanks very much for providing the patch with your work so far. Created a D7 branch and committed changes based on this.

There are instructions for how to make patches at http://drupal.org/patch/create - following them will help others to work with your code. The attached patch is your own, but using cvs diff -up and without personal directory paths included. I've made similar mistakes ;)

The D7 port should be available as a 7.x-1.x-dev download soon. Testing feedback welcome :)

EDIT: Not contrite enough, this patch introduced a foolish error, for which I apologise. Fixed in CVS now.

Some feedback at #914370: Notice: Undefined index: name in _update_process_info_list() (line 181 of /var/www/d7test/modules/update/update.compare.inc) - anyone else had similar experiences with the D7 branch to what's reported there?

I'm getting a warning when enabling the module using drush:

WD php: Warning: uasort(): Array was modified by the user comparison function in system_modules() (line 842 of /public_html/modules/system/system.admin.inc).

After installation I'm not getting any further warnings. From my first tests the active trail is correctly set in D7 now.

A thinko in the patch on #7 caused #914370: Notice: Undefined index: name in _update_process_info_list() (line 181 of /var/www/d7test/modules/update/update.compare.inc), which is now fixed. Setting back to "needs review", please test, find more problems if you can, and update status accordingly either way!

...subscribing

Subscribing

There's a 7.x-dev available, so fixed (as in no need to keep this issue open any more). People need to test that and report back so it can be improved.

Klonos,

It's not showing on the project page's list of releases yet.

#972124: Undefined index: render element in theme() (line 811 of ../includes/theme.inc). is an issue with 7.x-1.x-dev with regards to http://drupal.org/update/modules/6/7#theme_changes

The draggable table code which was introduced in #595282: Menu weights needs updating to handle theme changes in Drupal 7.

*Some* of that work is in the attached patch, but the draggy stuff is still not working ... so this is still very much needs work.

PS. now on github if you like that sort of thing.

@Adrian, #15: I know it doesn't show there yet. That's why I linked to it.

@grobot, #16: Thanx for the patch and for taking the time to explain and sorry for 'rushing' to mark this fixed.

No worries klonos, much appreciate all the effort people have made getting this module over to D7.

Attached is an update of the previous patch (which it replaces), and which should sort the "Enable / Disable Menus" section @ admin/config/menu_breadcrumb

This code needs review, please test and let me know if it behaves as it ought to.

---

A couple more things to sort before D7 is ready for release though.

1. It's possible to trigger a fatal error - here's my notes on how to reproduce.
D7 upgrade: with drupal_bootstrap() removed, a fatal error can be had here.
* Enable menu_breadcrumb and book modules.
* Create a book page, assign to new book.
* Create a second book page via overlay, assign to new book.
* On save:
PHP Fatal error: Call to undefined function menu_set_active_menu_name()
in ../sites/all/modules/menu_breadcrumb/menu_breadcrumb.module on line 324,
referer: http://drupal7.example.org/node/1
(didn't happen when creating a third book page without overlay, but did happen when editing third page in overlay)

and (less severe)
2. Menu Breadcrumb doesn't appear in configurable options at admin/config.

I think we're close enough to have 7.x-1.x-dev on the module page now. Still need review.

Current known issues are:

1. files[] = menu_breadcrumb.module not needed in the .info file, as file contains only functions.
2. Menu Breadcrumb doesn't appear in configurable options at admin/config.
3. Fatal error as in previous comment.

That should have been "still needs work".

subscribing

I have had a go at fixing the remaining issues I could find when using this module.

This patch should:

- Remove the unnecessary files[] entry in the info file
- Adds a Menu Breadcrumb area under admin/config
- Fix for #1103500: Multiple notices and a warning upon installation and several warning upon configuration - multiple array_merge() warnings
- includes change to renamed menu_set_active_menu_name function #1120218: menu_set_active_menu_name function - credit to vaeiou
- Fix for #1131088: Menu name don't show up - Menu names not displaying on configuration page

This is one of my first patches, hopefully I've followed the process correctly :) I've not used any of the regular expression parts of the module so that bit hasn't been tested.

...just to clarify before giving this a go: was #18 committed to latest dev (Feb 25) or do we need to apply previous patch(es) too?

Hmm, I read comment #19 and thought that the latest dev had included the patch in #18 too. Checking over the code it looks like some things from it were and others weren't, I can't see anything drastically wrong though (although I'm only just starting out with drupal!).

The patch I created was from the dev build with no other applied patches.

...thanx John. I think I'll just go back and re-check previous patches anyways and apply changes sequentially if missed by previous/next patches. An updated dev built with any progress this far included wouldn't hurt though ;)

Subscribing

#18 patch is dirty, mixed with #16 which has been committed to D7 branch.
Sorry to say but #22 looks like a bad patch.

+++ b/menu_breadcrumb.module
@@ -38,10 +38,16 @@ function menu_breadcrumb_help($path, $arg) {
  * Implementation of hook_menu().
  */
 function menu_breadcrumb_menu() {
-  $items = array();
 

I believe $items need to be declared as array.

+++ b/menu_breadcrumb.module
@@ -320,7 +326,7 @@ function menu_breadcrumb_init() {
-            menu_set_active_menu_name($menu_link_menu_name);
+            menu_set_active_menu_names($menu_link_menu_name);

should be menu_set_active_menu_names() EDIT: my mistake, this is correct.

EDIT: also 'access arguments' for admin/config page should be 'access administration pages' instead of 'administer site configuration'

Powered by Dreditor.

Here's reroll #18 & #22, see attached patch.

- Removed

files[] = menu_breadcrumb.module

from info file
- Cleaned up references to 'Drupal 6'
- Menu Breadcrumb at admin/config page
- Fixed fatal in #18
- Fixed #1131088: Menu name don't show up

Patch in #29 does not appear to fix the menu names not showing in the configuration UI. There are some errors that show up when trying to modify the configuration (I was looking through the HTML code to see which checkboxes go with what menu) that show up as (I'm guessing) a result of that fix not working.

@aaronpinero
Make sure you've cleared the cache.

The number of patches are getting confusing. Can anyone maybe upload a new 7.x-dev with all the latest updates in it?

I tried #29 in latest drupal 7.x dev and I got a WSOD when in Menu breadcrumb's settings page (../admin/config/menu_breadcrumb). The rest of the site worked ok though. I then tried to troubleshoot the WSOD by reverting the patch and applying it manually little by little. It comes to this part:

+    'description' => 'Breadcrumb based on menu.',
+    'position' => 'right',
+    'page callback' => 'system_admin_menu_block_page',
+    'access arguments' => array('access administration pages'),
+    'type' => MENU_NORMAL_ITEM,
+  );
+  $items['admin/config/menu_breadcrumb/settings'] = array(
+    'title' => 'Settings',

If I omit it the settings page displays fine.

...I guess NW then.

confusing, no more patch please, can anyone share a complete tarball?

@klonos I could not replicate your WSOD with patch from #29. Please test with this and let us know if it's still an issue?

This patch includes #29 and also moves MB settings from admin/config/menu_breadcrumb/settings to admin/config/user-interface/menu-breadcrumb, which I think is more in line with D7IA.

I've committed the above, so please grab the updated 7.x-1.x-dev tarball when it appears.

We might even be ready to release 7.x-1.0 if that goes well enough.

Anyone using this and able to say whether it's RTBC or not?

I was using it Chris, but now I am in the process of reviewing various other breadcrumb modules (currently Custom breadcrumbs).

Sorry I didn't reply to you in #36. I can understand your frustration -after 3 weeks of no reply- but is this urgent? ...I mean can I give your module a review once I've finished "playing" with Custom breadcrumbs? ...say over this weekend? I think I'll be able to reply by Monday then (unless somebody beats me to it). Will that do? Let me know.

Thanx and again sorry.

I'm using it together with Custom Breadcrumb.. Nobody want to test?

@klonos, any time you can contribute (as a tester or otherwise) is very welcome and much appreciated.

I am not frustrated with you at all, and apologise for any choice of words which led you to think I might be. As I'm not using Menu Breadcrumb on D7, I'm not able to test as thoroughly as someone who is actively developing on a site with it enabled. Hence I prefer to wait on community feedback before pushing 7.x-1.0.

Your previous comment suggested that there was still some issue which was unresolved, and I'd rather not release 7.x-1.0 until that is fixed or discounted.

@grobot: Hey Chris, I've switched three of my 7.x setups (2 production + 1 semi-production sites) from Custom breadcrumbs to the latest dev and it seems to do fine so far - no errors or anything obvious. I mainly use the "Append page title to breadcrumb", but I also gave a spin to the "Append as URL" and "Hide if only front page" options while I was at it. Anything else in particular you need me to test? Let me know.

PS: since we are heading for a stable 7.x, I'd like to take the opportunity and say that I'd love to see #1193500: Support adding current tab's title to the breadcrumb trail & #1193408: Allow appended title to be css selectable/themable (wrap in <span> tag + add a class or id to it) get implemented ;)

Just created the release node now. Tarball should be along shortly. Thanks toktik, ckng, klonos and everyone else who contributed code or testing to this.

Menu breadcrumb 7.x-1.3.

No problem mate ;) ...we thank you for the time & effort spent on this project.