Hook_menu() conflict with Vocabulary Index

Yep. Both modules use hook_menu_alter to take control of the taxonomy/term/ pages. Can you check the module weights in the system database tables. I think it could work if tax_breadcrumb is given a greater weight than, vocab index. Can you give that a try and report back?

Thanks.

MGN, could you make hook_menu_alter() override taxonomy/term/% and pass on the existing callback and arguments to your own callback? Your callback would then simply set the breadcrumb and then execute the original callback. I see you are already using a system variable named taxonomy_breadcrumb_override_callback, but I can't see it being set anywhere in the code. I assume this is just an API function that other modules need to implement?

Hi Xano,

Yes. This is already done in the current dev...

Have a look in taxonomy_breadcrumb.inc :

function _taxonomy_breadcrumb_term_page($str_tids = '', $depth = 0, $op = 'page') {
  static $callback = array();
  if (empty($callback)) {
    $callback = variable_get('taxonomy_breadcrumb_override_callback', array() );
    if (!empty($callback)) {
      $require_path = isset($callback['file path']) ? $callback['file path'] : drupal_get_path('module', $callback['module']);
      require_once($require_path .'/'. $callback['file']);
    }
    else {
      // Default to core taxonomy_term_page callback.
      // Include the .inc file with all helper functions.
      require_once(drupal_get_path('module', 'taxonomy') .'/taxonomy.pages.inc');
    }
  }
  if (isset($callback['page callback'])) {
    $func = $callback['page callback'];
    // Assume that any function overriding the core taxonomy function would have the same parameters.
    $output = $func($str_tids, $depth, $op);
  }
  else {
    // Call the core taxonomy_term_page function.
    $output = taxonomy_term_page($str_tids, $depth, $op);
  }
...

Let me know if you see anything that needs to be changed, but I think its exactly as you suggested.

EDIT: Or is the problem in my assumption that "Any function overriding the core taxonomy page would have the same arguments"???

I took a good look at the code and this is how it goes: you override the existing taxonomy/term/% callback and save all callback information in a system variable. In your own callback you check if the variable exists and execute the saved callback (but only partly, this is where the same arguments come in) after setting the breadcrumb. Since variables may be deleted at any time, you fall back to Taxonomy's term page callback. A more solid approach is not to save the original callback in a variable, but to pass it on to your own callback, which executes the original callback again after setting the breadcrumb.

Yes. I agree, but the real problem is with the page arguments. I can pass the existing callback through my replacement callback by adding it as a (serialized) page argument, thus avoiding the use of a variable.

The problem is what to do next without recreating the menu system. The page arguments would need to translated and any necessary objects loaded in order to get the actual parameters needed to execute the original callback.

Looking through the core menu code, it doesn't seem like its modular enough for reuse. That means essentially re-writing chunks of the menu system to handle this task.

Please tell me I am missing something (or better yet offer up a patch!), because I agree a better approach is needed. Its not correct to assume that the callback functions will have the same arguments.

Note that it would be much easier if modules overriding taxonomy_term_page would provide compatible callback functions. Perhaps it would be easier (or at least possible) to modify vocabulary index ?

I haven't tested the patch, so there may be some small errors, but it will show you the general idea behind this approach at least.

Note that it would be much easier if modules overriding taxonomy_term_page would provide compatible callback functions. Perhaps it would be easier (or at least possible) to modify vocabulary index ?

Taxonomy Breadcrumb just puts itself between two existing features. Other modules could use your system variable approach, yes, but changing Taxonomy Breadcrumb makes sure they don't have to and guarantees people that it will work fine with any other module that uses taxonomy/term/%.

Adding a fixed parameter for the wildcard in /taxonomy/term/% to make sure it is always passed on to _taxonomy_breadcrumb_term_page(), no matter what arguments the original callback takes.

Also, I'd change all private functions back to normal functions again in your next major release. Most if not all can just as easy be public.

Yes, this is the approach I first went with, but it doesn't work because it doesn't translate the page arguments.....

For example, look at the taxonomy callback (for taxonomy_term_page)

Array ( [title] => Taxonomy term [page callback] => taxonomy_term_page [page arguments] => Array ( [0] => 2 ) [access arguments] => Array ( [0] => access content ) [type] => 4 [file] => taxonomy.pages.inc [module] => taxonomy )

notice that $callbacks['taxonomy/term/%']['page arguments'] = Array ( [0] => 2 )

Its '2' because the path is 'taxonomy/term/%' and arg(2) would be the term id specified in the path.

So to get the parameters to pass to the original callback function, you have to translate the page arguments. And for some callback function this could mean calling hook_load....

See http://api.drupal.org/api/function/menu_execute_active_handler/6 (and related functions) to see how the menu system does this....its a difficult problem.

Have you tried is or is this just theoretically speaking? My theory says it'll work, since you simply pass the integer argument on to _taxonomy_breadcrumb_term_page(), which ensures it is being translated before passing it on to the original callback. But then again, I haven't tried it.

Sorry Xano, your patch is fine. Turns out I was passing the page arguments through incorrectly which was causing them not be translated. Looking over you patch helped me to see the error in my ways. Thanks!

I see only one thing to fix in hook_menu_alter, check to make sure $term_callback['file path'] is set. If its not, then use the path to the module...

$filepath = isset($term_callback['file path']) ? $term_callback['file path'] : drupal_get_path('module', $term_callback['module']);

Found another issue. drupal_set_breadcrumb has to be called after the original callback function.

I guess the real question, is does this actually work with Vocabulary Index? Can someone verify that vocab index works as expected, and breadcrumbs are correctly displayed on taxonomy/term pages?

Nice one. Have you taken it for a spin yet?

Seems like we are getting good at cross posting here! I can verify that it works for tax breadcrumbs...

I'll check it out with Vocabulary Index then :)

Just to clarify, when you say you don't see breadcrumbs on index pages do you mean pages at taxonomy/term/tid where tid is a number specifying the taxonomy term id ? Could you give an example of the path you are using so I can see any additional arguments?

Since I have no experience with vocab index, Xano may be better able to help.

That is normal behaviour, since Taxonomy Breadcrumb only displays breadcrumbs at term pages. Vocabulary Index pages technically have no parent, so only "Home" is being displayed.

Taxonomy breadcrumbs handles breadcrumbs for the taxonomy/term/tid pages and for nodes with taxonomy terms. If the vocab index pages are module pages (not nodes), then taxonomy breadcrumbs can't do anything for these pages.

I think I can use the same technique developed in the patch to apply custom breadcrumbs (via the custom_breadcrumbs module) to module pages. But that would be a feature request for that module.

aluminium, can you give a specific example indicating what you would like the breadcrumb to be on vocab index pages (rather than home)?

I have too many other projects going on right now to set up a site and play with vocab index (or many of the other modules that people are using...) so you'll need to give detailed examples for me to be able to help you out.

It seems like we can mark the patch rtbc since we have confirmed that taxonomy breadcrumbs works as expected with vocabulary index now. Xano, do you agree?

I'd like aluminium to confirm that it works on term pages. If it does, then this baby's ready to be committed :)

Might be best to keep this issue about the menu problem and to dicsuss new features in a new issue. Just to keep some oversight :)

@aluminium, can you confirm that taxonomy breadcrumbs are showing up on taxonomy/term/tid after applying the patch in #14 ?

It seems like this is what you indicated in #21

You'll see breadcrumbs only on taxonomy/term/tid, but on other pages only home link.

but I just wanted to be sure I am reading this correctly. Thanks again for helping on this.

Thanks for the screenshots! Could you post them as normal images next time?

It seems the patch still doesn't work. I'll debug it tomorrow evening then.

Reading through the vocab index code (6.x-2.x-dev) in cvs I now see that this is not due to a conflict between tb and vi at all.

It looks like vocab index doesn't actually override the core taxonomy/term pages. It just adds menu items for specific pages.

$items['taxonomy/term/' . $term->tid] = vocabindex_menu_base() + array( ...

This will stand alongside taxonomy's 'taxonomy/term/%' as a unique menu item. When responding to a request for the specific $term->tid, then vocab index's callback will fire (since it has a better fitness). But if the specific item isn't available, then the core tax term page callback will fire.

So breadcrumbs aren't showing up on the vocab index pages at taxonomy/term/tid, because they are not *really* taxonomy/term/tid pages...despite the close resemblance in the path.

I am guessing that vocab index could actually be using hook_menu (rather than hook_menu alter) for these pages since they are most likely not already in the menu router table...

I don't think that tax breadcrumbs should try to override specific menu items (i.e. without the wildcard, say by trying to override each item starting with taxonomy/term), because they may not even be taxonomy pages. Another module could choose to use that path and the term id may or may not be in the taxonomy database tables...

All of this is theoretical. Perhaps Xano could check to see if this is correct or not?

If I am right, then I am afraid this issue is destined for "Won't fix", though "can't fix" is more appropriate...

[ EDIT: Another cross post! I think the patch works fine and does what it was intended to do...the problem is that vocabulary index is not providing a menu item at 'taxonomy/term/%'....]

I think I'll convert the menu items to use taxonomy/term/% then.

Thanks Xano. I think that will work out better anyway. It adds fewer menu items to the database (thinking of sites with large taxonomies) and the module can easily handle the logic for dealing with the wildcard to load the necessary objects. We will put this on hold until its possible to test the patch with a modified version of Vocab Index.

Just setting issue status properly.

I wrote a patch for Vocabulary Index (#494408: Rework hook_menu_alter() for compatibility with other modules and less performance issues) that makes it use taxonomy/term/% as well. While writing that patch I came across some things that I want to add to the patch in this issue, so marking it needs work.

Here's the reworked patch which is simpler and IMO tidier.

I'd love to help clean up this module, so if you need another hand and you like my patch ( :P ), I hereby request CVS access.

Thanks Xano. The patch looks good...I'll test it out soon and get back to you. I've already given you CVS access, and appreciate all your help!

While testing the patch it I made the *mistake* of starting with the views taxonomy/term/% view enabled. I wasn't planning on testing the patch against views, but why not? If this approach is general enough it shouldn't matter what module has claimed taxonomy/term/%, right?

It didn't work, at first because views had a higher module weight.
Increasing the module weight of taxonomy_breadcrumb, produced an "access denied" message at taxonomy/term/tid and the following error

warning: array_merge() [function.array-merge]: Argument #2 is not an array in C:\wamp\www\html\sites\all\modules\taxonomy_breadcrumb\taxonomy_breadcrumb.module on line 104.

disabling the view eliminated the error message and restored the expected tax breadcrumb functionality.

I'll look into this more...

Not from me. I had a very busy summer, but I am trying to clean up my own issue queues and this issue, although not in one of those queues, is one I want fixed. MGN, could you check out the patch for Vocabulary Index and see how we can get this working? I'll try to do the same this week.

Sorry, I've also been busy and haven't had any new thoughts on this since #36. In the end, whatever solution we come up with also needs to avoid the errors describe above for views. I'll try to test the Vocabulary Index patch this week and report back.

Applied the menu_02.patch and now I'm getting some warnings about missing arguments, but still no breadcrumbs appear, only "Home". And when I go to the deepest level of taxonomy hierarchy, which has no dependent terms, I get "Fatal error: require_once() [function.require]: Failed opening required '/' (include_path='.;/usr/local/php5/PEAR') in Z:\home\sessii.net\www\sites\all\modules\taxonomy_breadcrumb\taxonomy_breadcrumb.inc on line 27" instead of list of nodes.

Sorry for the long delay in getting back to this.

I think I have found the error leading to the access denied messages described in #36 and have made a slight adjustment to Xano's patch in #34 to fix that. I no longer get the access denied error when views overrides taxonomy/term/% (I should point out that this patch will not provide taxonomy breadcrumbs when views overrides taxonomy/term/%, however, so I'll followup with another issue to provide for that case).

I've also tested this patch with Xano's vocabulary index patch in #494408: Rework hook_menu_alter() for compatibility with other modules and less performance issues (I had to manually patch since 6.x-2.x-dev has changed) and everything seems to work correctly now.

It would be great if others could test these two patches together.

I want to test these patches tomorrow, so we might be done with it this weekend. MGN, please poke me once in a while if forget this anyway (loads of contrib to maintain and a real life too :-P )

Page callbacks are required for menu items, so there should be no need to test for them.

Okay. I tested the patch and it seems like it all works fine. The breadcrumbs appear on the custom term pages as provided by Vocabulary Index.

Yeah. Should only be checking isset($items['taxonomy/term/%']) before assigning the reference - the callback will be defined if the menu item is defined. Anyway, that seemed to be the trick to avoid the error when views overrides taxonomy/term/%. I am still not sure I complete understand the origin of that error...

committed to 6.x-1.x-dev.

Cool, nice work! :)