Bug in parent assignment via taxonomy term causes parent assignment via node type to fail [#547634]

Looks like this patch http://drupal.org/node/364717#comment-1237256 which implemented the following change has introduced a new bug.

-        if ($term_trails[$term->tid]) {
+        if (isset($term_trails[$term->tid])) {

Because of the isset() statement menutrails fails to assign a trail based on node type if the page/node in question has any vocabulary associated with it.

The $term_trails array is populated regardless of wether or not the page in question is making use of the menutrails taxonomy settings or not. You end up with an array where the key is the term id, and the value is either blank, or the path of the menu item that you have choosen to use as a parent for this particular term.

$term_trails[1] = '';
$term_trails[2] = 'some/path';
$term_trails[3] = ''

Attached patch fixes this by replacing isset() with !empty()

Comment	File	Size	Author
#9	patch_trailsandtaxonomy.txt	497 bytes	ayalon
	eojthebrave_menutrails_fix-nodes-with-terms.patch	489 bytes	eojthebrave

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rwd CreditAttribution: rwd commented 20 August 2009 at 16:43

Confirming that this issue:

is critical as it prevents menu trails being highlighted under the conditions described by eojthebrave;
is still present in the 6.x-1.x-dev (2009-Aug-18) release; and
is fixed in this release by the patch.

Comment #2

Les Lim

he/they

English

CreditAttribution: Les Lim commented 24 August 2009 at 17:36

Status:

Needs review

» Reviewed & tested by the community

Confirmed. As this is a critical problem and a near-trivial tweak of the code, I'm inclined to mark this RTBC.

Comment #3

jenpasch CreditAttribution: jenpasch commented 2 September 2009 at 15:32

6.x-1.x-dev patched with patch above.
Uninstalled and reinstalled. Cleared the caches for good measure.

But taxonomy terms still override my node type

ideas as to where I have gone wrong?

ex:
http://new.fafia-afai.org/en/news/withdraw_the_public_sector_equitable_c...

patched code:

 $type_trails = variable_get('menutrails_node_types', array());
    $href        = $type_trails[$node->type] ? $type_trails[$node->type] : FALSE;
    $term_trails = variable_get('menutrails_terms', array());
    if (!empty($node->taxonomy)) {
      foreach ($node->taxonomy as $term) {
        if (!empty($term_trails[$term->tid])) {
          $href = $term_trails[$term->tid];

Comment #4

jenpasch CreditAttribution: jenpasch commented 2 September 2009 at 15:45

Status:

Reviewed & tested by the community

» Needs review

(I am respectfully adjusting the status here. Hoping that its not user error...or actually that'd be fine too, if someone can point out the error of my ways)

Comment #5

grendzy CreditAttribution: grendzy commented 2 September 2009 at 23:51

Status:

Needs review

» Reviewed & tested by the community

The patch is working for me.

jenpasch: Please see: #539922: Bug fix for taxonomy parent(s) overriding node type parent - $href is overwritten in menutrails_get_breadcrumbs Taxonomy terms override node types, by design.

Comment #6

jenpasch CreditAttribution: jenpasch commented 3 September 2009 at 02:27

oh man, really? I understood "Menu trails are evaluated in the order they are shown below" to mean that the top item (node type) took precedence over all others...thanks for clarification

Comment #7

hefox CreditAttribution: hefox commented 2 October 2009 at 15:34

jenpasch: Might be a useful idea to introduce a patch that weights the importance of parent items and decides the order items are evaluate. Or, re word that message. I don't need that, but it could be fairly simple.

(Current Patch works for me, thanks).

Comment #8

hefox CreditAttribution: hefox commented 2 October 2009 at 15:36

(Oh right I was going to say, could be useful performance wise, for nodes that have a lot of terms, to do an initial check with !empty(array_filter($term_trails)) to skip that foreach completely).

Comment #9

ayalon CreditAttribution: ayalon commented 8 October 2009 at 12:07

File	Size
patch_trailsandtaxonomy.txt	497 bytes

Today I had the the same problem. I debugged and finally come to the same solution as posted above. Unfortunately I did not search the forum before writing a patch :-)

This bug is really critical! Please commit this fix immediately, because all nodes that have taxonomy terms attached will fail with menu trails is a node type parent is defined.

Comment #10

jrabeemer CreditAttribution: jrabeemer commented 23 October 2009 at 12:33

Confirmed #9 fixes menutrails from not working at all to responding as it should. RTBC!

Comment #11

Milkrow CreditAttribution: Milkrow commented 3 November 2009 at 23:13

Forgive me if I this comment doesn't belong on this thread, but I tried this patch and could not get it to work on Drupal 6.14 with MT 6.x-1.0 (I could not find a dev version of this module). I got an error on lines 132 and then 482 when I tried the patch above (the one with !empty, not isset). I'm not facing an issue with taxonomy, rather, I just can't see where "active" is added to the li...My content types are assigned to parent elements of the primary links, but active states do not appear unless the nodes parent menu is set to Primary. I've seen this work on another site, but for the life of me can't determine the issue. I'm not a PHP whiz...so kids gloves if you have them, please.

Comment #12

hefox CreditAttribution: hefox commented 3 November 2009 at 23:30

The dev version is accessible under Development Snapshots on the front page; my guess is the patch is against that but it's been too long.

Comment #13

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 4 November 2009 at 15:42

@Milkrow, can you confirm that the patch applied correctly.

This line

if (isset($term_trails[$term->tid])) {

in menutrails_node_location should have been changed to

if (!empty($term_trails[$term->tid])) {

If that change was made and you're still not seeing the "active" class I'm guessing the issue is on your theming layer. See this article for some discussion of possible issues with theme_* function implementations. #328517: Having menu highlighted when I visit individual nodes

Comment #14

Milkrow CreditAttribution: Milkrow commented 4 November 2009 at 20:53

That change was made (ostensibly, correctly). While I can see class="leaf active-trail" in a menu block (a menu I created) I cannot see the same class="leaf active-trail" in the Primary Links. I will read that other thread and see if that helps clear it up...but I'm only using Zen with CSS edits to theme...not making any major template changes.

Comment #15

Milkrow CreditAttribution: Milkrow commented 13 November 2009 at 15:04

Just a follow up here. I determined that the reason why the nodes weren't triggering "active-trail" is because they were part of a menu I created (not generated from childofprimary) and therefore those links were treated as external links and therefore will NOT trigger "active trail". Attributing those nodes to be a child of a primary link works. However, now I have to recreate new subnav menus using Menu Block (module). I hope this helps someone trying to figure this out.

Comment #16

ayalon CreditAttribution: ayalon commented 22 December 2009 at 16:52

Can someone explain me, why this path isn't commited yet? I lost another 2 hours because of this buggy module.. More than providing a patch is not posible. And the patch was confirmed as a working solution

Comment #17

hefox CreditAttribution: hefox commented 22 December 2009 at 18:32

To my understanding, Sun is the active maintainer of this module and Sun is currently engaged in drupal 7 core issues, so is not as activally keeping up with 6.x modules as others would like (This statement based on comment/s in form module's issue queue; I do not Sun or anything ).