tab_root_map and tab_parent_map notices undefined in _menu_translate()

Retitling for review by the relevant people.

Odd.

http://api.drupal.org/api/drupal/includes--menu.inc/function/_menu_trans...

Not sure how this can happen, but yeah, the lines involving implode() should be wrapped in corresponding if isset() conditions.

@sun: So the entire line should be wrapped in isset()? Tagging as Novice

Old patches need love too.

The original issue is probably invalid by now, but closing out on comment #2.

line endings...

I'm experiencing these same errors with Drupal 7.0. Haven't tried the latest patch though, so maybe that did the job.

I think these are getting fixed in 8.x now, and backported to 7.x.

Thank you for the fix!

d.o coding standards call for braces around "if" code blocks - {}

braces added

Subscribe

seems correct & tesbot are happy

I am getting the following errors after trying to create some content and all the pages in the site show "The requested page could not be found"

I tried applying the patch provided by uploading the file to the includes folder and using the patch command using putty - patch -p0 < menu.inc.patch

it doesn't fix the problem, any one know what am i doing wrong?

ERROR
Notice: Undefined variable: tab_root_map in _menu_translate() (line 772 of /home/tinselca/public_html/includes/menu.inc).
Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 772 of /home/tinselca/public_html/includes/menu.inc).
Notice: Undefined variable: tab_parent_map in _menu_translate() (line 773 of /home/tinselca/public_html/includes/menu.inc).
Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 773 of /home/tinselca/public_html/includes/menu.inc).

The patch work perfectly, should be added in the next version of drupal core, imo.

So far i was solving this by adding "@" in front of "implode" functions (=> "@implode") to disable PHP error messages.

I tested the patch on D7. Workes perfectly. I have no D8 running on my machine. But the tests run correctly on the QA System. I say add it to D8 and backport as soon as possible to D7 next release.

This is not major.

I applied the patch and it fixed the errors I was receiving.

Tagging issues not yet using summary template.

Error message is gone (drupal 7.7) but I still receive a "The requested page could not be found." on all admin and user pages.

This error occurred after changing the content type:Page, so that revisions are automatic.

The only page that works almost completely is the Appearance page. I can change the settings of my theme but cannot enable or disable themes.

Edit**

I found these two pages. One mentions Views being a problem, the other resolved my issue.
http://drupal.org/node/259709
http://drupal.org/node/262892

+++ b/includes/menu.inc
@@ -768,9 +768,15 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
-  $router_item['href'] = implode('/', $link_map);
...
+  if(isset($link_map)) {
+    $router_item['href'] = implode('/', $link_map);
+  }

$link_map is always defined.

+++ b/includes/menu.inc
@@ -768,9 +768,15 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
+  if(isset($tab_root_map)) {
...
+  if(isset($tab_parent_map)) {

Missing space after "if".

20 days to next Drupal core point release.

d7.8 Return of #1174268: After upgrade from 7.0 to 7.2 Undefined variable: tab_root_map in _menu_translate

    Notice: Undefined variable: tab_root_map in _menu_translate() (line 772 of /home/monkeype/public_html/includes/menu.inc).
    Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 772 of /home/monkeype/public_html/includes/menu.inc).
    Notice: Undefined variable: tab_parent_map in _menu_translate() (line 773 of /home/monkeype/public_html/includes/menu.inc).
    Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 773 of /home/monkeype/public_html/includes/menu.inc).

Why is this under version 8.x and not marked under critical priority? It effects 7.x and is making Drupal unusable (for some)

#24: The issue is tagged for backport, which makes it equivalent to being filed against 7.x. (See also the backport policy.) Can't speak to the priority, but I believe this issue is just about the notices. Clearing the menu cache seems to resolve the (different) problem of "Page not found" errors on admin pages (see #21). (A completely corrupt menu router could cause this problem as well as just one set of bad tabs.)

Per #21.

I am running 7.8 and began having same type error when using Page Manager in Views. I was trying to remove the wizard created home page, create a basic page for the landing page and setting up a menu.

•Notice: Undefined variable: tab_root_map in _menu_translate() (line 772 of /var/www/vhosts/elslandscaping.com/httpdocs/includes/menu.inc).
•Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 772 of /var/www/vhosts/elslandscaping.com/httpdocs/includes/menu.inc).
•Notice: Undefined variable: tab_parent_map in _menu_translate() (line 773 of /var/www/vhosts/elslandscaping.com/httpdocs/includes/menu.inc).
•Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (line 773 of /var/www/vhosts/elslandscaping.com/httpdocs/includes/menu.inc).

The reason I was doing this is the RDFa on the wizard created home page was not passing through.
Metatags were also not passing through to the wizard page.
That was the only page / panel I had created at that point.

$link_map left alone & Spaces after "if". Thanks :)

Confirming this is still an issue in 7.9, and the patch is working for 7.9. Thank you for the patch.

Retitle, this is not beta version specific.

Thanks for the .patch.

Working to me as well in 7.9

Thanks oriol_e9g and all. Shall we call this RTBC?

Latest patch looks good. But I'm still not sure under which circumstances this bug appears, and why we don't see these PHP notices in any of the existing tests.

Thus, although the code logic error is kinda obvious, I believe we want to add a test that catches the actual bug, so this doesn't happen again in the future.

The implode warnings are showing up for my site too, not sure what has caused it - this site uses menus, views, panels and custom module menu hooks, so it could be tricky to narrow down. Any idea when this patch will be merged into core?

Still not fixed in 7.10... what's holding this up?

@scott.whittaker you will notice that issue needs test the quickest way to ensure this patch gets committed is to create tests for it. Drop into #drupal-contribute where people will be happy to provide you with help in creating tests you don't know how to create them.

This error also appeared for the first time for me today after updating to core v 7.12. Patch fixed it though I'm also not sure why this error started to appear today.

I get this when using Panels and creating a "home page" landing template. I set the template's path to be <front>, then create a menu link for the home page (<front>) and when doing so, I randomly get the same error.

The workaround, is to create a solid path for the panel template such as "front-page", then manually create a menu that links to "Home", using <front> as the path, as opposed to creating a menu from the landingpage template.

I also get those two messages:

Warning : implode() [function.implode]: Invalid arguments passed dans _menu_translate() (ligne 772 dans /home/sexua916/public_html/lesexeasesraisons.com/lesexeasesraisons/includes/menu.inc).
Warning : implode() [function.implode]: Invalid arguments passed dans _menu_translate() (ligne 773 dans /home/sexua916/public_html/lesexeasesraisons.com/lesexeasesraisons/includes/menu.inc).

Has anyone figured out what's causing the bug?

Thx!

I upgraded to Drupal 7.12, everything seems fine now... I'll keep you posted if anything comes up!

The patch passed but won't be committed until #32.

Doesn't that mean "needs review" is the wrong status?

#4: 951098-_menu_translate.patch queued for re-testing.

The scenario described in #37 fit my environment, and the workaround worked. Thanks, @philsward.

#37 worked for me, too.

#27: menu_translate-951098-27.patch queued for re-testing.

Changing to 7.x as 8.x restructures the function and already puts the offending lines into appropriate isset checks.

#27: menu_translate-951098-27.patch queued for re-testing.

Now that's interesting. The bot passes the patch under D7.

Okay, I asked yareckon to switch the issue version to D7 because the code isn't relevant in D8, BUT I did confirm in IRC with catch that we still need the tests in D8.

However, since it's not broken in D8, but is still broken in D7, I suggested that yareckon might find it easier to write the tests for D7 to demonstrate the bug and then port it to D8.

I also get the Notice when setting the installations default frontpage to "" and creating a view with the path "" too. Only after flushing cache i see:

Notice: Undefined variable: tab_root_map in _menu_translate() (Zeile 785 von /home/user/public_html/includes/menu.inc).
Warning: implode() [function.implode]: Invalid arguments passed in _menu_translate() (Zeile 785 von /home/user/public_html/includes/menu.inc).

For me this path great during much releases.

Here rerolled patch.

p.s. Seems it same as #27

I closed #1956542: tab_root_map and tab_parent_map notices undefined in _menu_translate as a duplicate and am updating the title slightly, since it doesn't seem to be related to upgrades.

Also, why was this moved back from Drupal 8 to Drupal 7? The same code is in Drupal 8 and doesn't have any isset() checks.

works in D 7.23. thanks

Please commit this to D7, it is needed and working as adovcated!
Greetings, Martijn

I think this is important one problem, even for D7 new version. 7.28 should update.

I just installed on 7.24 and it seems to be working well

Applied patch at #50 to 7.25 and it seems to be working there too. A little frustrating I need to apply the patch again every time I update. :(

1+ for #56 - is there any way to find out the cause of this issue, or commit, so that there's no need to patch future updates?

+1 for #50. Notice still active in Drupal 7.26

50: undefined-menu-translate-notice-951098-50.patch queued for re-testing.

I doubt this patch makes sense against drupal 8.

#50 was against Drupal 7 (updated version of #27 with same changes).
Here is a patch against D8 (same code changes).

This is not needed for D8, see #2107533: Remove {menu_router}

This can go straight into D7 now.

50: undefined-menu-translate-notice-951098-50.patch queued for re-testing.

50: undefined-menu-translate-notice-951098-50.patch queued for re-testing.

it's still ok )

The patch in #50 worked great for me vs Drupal 7.29.

Rerolled patch for D7.

The patch in #73 worked great for me. Drupal 7.31.

It also worked for an earlier version of drupal 7, not positive what version i applied it to awhile back.

Removing tags, this issue no longer needs a summary update or a test as far as I can tell. It's no longer a backport to D7, now just a small fix for D7.

I have successfully used the patch from #73 and now it is all green on tests - let's commit it.

been using in production for months; seems a good candidate for 7.33 if it's within the window

This looks like it might be hiding a bug rather than fixing it? I can't see any reason from the code why $tab_root_map and $tab_parent_map would legitimately be unset (since they are built from $router_item['tab_root'] and $router_item['tab_parent'] which should always be part of the router item, unless a malformed router item is being passed in to this function).

See also #32. This may or may not need tests (depending on the root cause) but we at least need to understand the root cause.

Agreed with #84.

For me it was rather strange use case (rather misconfiguration than a bug) - the site_frontpage variable was set to <front> and this path was assigned to the Panels page. After changing it just to frontpage the error message disappeared.

AndyB

#85: Interesting...

I just checked my site that is exhibiting this problem and had the exact same issue! For me, changing both the Panel page path and the site_frontpage variable to the value "home" resolved the problem, but now the homepage technically exists in two places "/" and "/home". Anyone have any thoughts how best to solve that?

Global Redirect - https://www.drupal.org/project/globalredirect - should help here. It has the "Frontpage Redirect Handler - If enabled, any request to the frontpage path will redirect to the site root." option.

I am having this issue when trying to create a custom landing page with panels.

Depending on the order in which I do these thinks I will find the issue's error or a blank front page :/

Setting default path for front page at /admin/config/system/site-information
Creating a panels page, checking "Make this page your site homepage" or giving the page the path from above
Disabling the Home menu item (clicking on logo is enough)

The configuration I had before doing this for frontpage was another custom path ("landing") at /admin/config/system/site-information which didn't correspond to any Panels or view page or node. It was used to show a block that had the option to show only on the listed pages: ""

Seems major to me.. :/ basically I cannot create a proper page frontpage!

One potential cause of this is the Any Menu Path module. So if you're using that module try investigating there.

See #2423885: Undefined variable: tab_root_map & tab_parent_map in _menu_translate() with some any menu path items

This patch is working for us; I've rerolled to latest 7.x.

Applies cleanly still. RTBC.

+1 for RTBC

Question:

Why would it be empty in the first place? Has that been checked already?

Should we not ensure the indexes are at least set to '' or FALSE instead of just skipping them?

Oh and as this is a bug we will need tests for it.

@FabianX This happens in _menu_translate(), when either $router_item['tab_root'] or $router_item['tab_parent_map'] are not set. It checks that condition, initiates the $tab_root_map and $tab_parent_map variables, but doesn't initiate it if the checks fail.

The infringing commit is https://github.com/drupal/drupal/commit/c9de4646c570a45de03d6e7ec470daf0... , from #907690: Breadcrumbs don't work for dynamic paths & local tasks #2.'

Not sure why it's empty, just know it's resulting it LOTS of PHP notices. Looking through the code, it's clear how the notices appear. The origin is unknown, and I don't have time to investigate further. This fixes the clear bug in the code.

#96: Fabianx (lowercase x please).

Given it is a regression in pretty complicated code, I am okay with no tests, but could we please keep 'BC', which means setting the value to NULL before the isset().

Its kinda useless, but if someone depends on this being set to NULL at least their code won't break.

Adds the isset.

RTBC! - Thanks, assigning to stefan.r for confirming my review and potential commit.

I don't see any problem with the patch itself, but I'm not sure about potentially hiding another bug, as we still don't have any explanation for why this would be empty in the first place (see #32, #84, #94) -- faulty contrib code I guess?

Unsure where it comes from. Faulty menu/contrib code, coming from _menu_translate(). There doesn't seem anything wrong with the menu itself, just that the array elements are not there when it's being passed in. Whether that's considered hiding an error is beyond me.

Hello!

I see that bug too. Function "menu_link_get_preferred" creates db_select from "menu_links" with a left join to "menu_router". Some menus on my website are in "menu_links" table but aren't in "menu_router" table. After that request, I get info with a part of NULL data. When Drupal sends the data to "_menu_translate()" function it generates notices about NULL data.
If we don't plan to use "menu_router" table in D8, we can accept the last patch.

Patch tested by me, it works great, but I'm not sure that it has to be in the core - seems it is just a workaround.
Problem if in the incorrect link definition inside the DB, someone (and somehow) saved menu link without the needed arguments

Want to mark it as "won't fix" but probably I'm wrong?

This is actually a fatal error in PHP 8.x and needs to be addressed ASAP.
The patch looks good, though it could have been a bit shorter implementation:

 $router_item['tab_root_href'] = isset($tab_root_map) ? implode('/', $tab_root_map) : NULL;
 $router_item['tab_parent_href'] = isset($tab_parent_map) ? implode('/', $tab_parent_map) : NULL;

FYI, in the context of PHP 8.0 there is also a high potential for similar bug in other places.

Marking this as "Reviewed and Tested" because:
- it is tested
- doesn't matter why tab_root_map and tab_parent_map note set - with PHP 8.x it is a fatal error!

@euk Thanks for pointing at that, but according to the Issue priority definition this is not critical.

If you are experiencing the issue, can you post here a simplified call stack, so we can have a better understanding about how this error is triggered?

From what I have seen, the _menu_translate() is called on four places in the Drupal 7 core:

1. In menu_get_item() - if the error comes from here, then it seems to be a problem in a contrib module, because there is a check so that this function is called only if $router_item exists (if ($router_item)). Then there is an alter hook called on the router item and that should be a problem, if some contrib module unset these two array keys there:

drupal_alter('menu_get_item', $router_item, $path, $original_map);

2. In menu_local_tasks() - router items are passed directly from the menu_router database table, so I do not see a problem here (it is not possible that the two columns will be unset).

3. In menu_contextual_links() - the same as the point 2.

4. In menu_link_get_preferred() - if the error comes from here, it can be cause by what was described in #103.

------------

Summarized - if the error is caused by the point 1., then I think this fix will only mask the problem and should be fixed in the contrib module. If the error is caused by the point 4. and we can confirm that the database state can occur somehow, then we can probably fix this without worries.

But the call stack will definitely help here :)

I agree and respectfully disagree at the same time =)

Yes, there might be some changes coming from a 3rd-party code, which break the site.

But, a the same time, by using if (isset()) {} already twice for each variable (see the snippet below) tells that it is expected to have at least one of those variables as unset. Since the alter hooks can alter whatever they want, right? One unset variable is enough, btw. Also, as I pointed out, PHP 7.4 was more forgiving when passing unset variable to implode(), but not PHP 8.0.

So why not use another isset() call to avoid any issues whatsoever, considering we already assume the variable can be unset? It doesn't hurt, it would definitely be a more compliant (based on how it written currently) and robust code.

  if (isset($router_item['tab_root'])) {
    $tab_root_map = explode('/', $router_item['tab_root']);              // <-- HERE
  }
  if (isset($router_item['tab_parent'])) {
    $tab_parent_map = explode('/', $router_item['tab_parent']);        // <-- HERE
  }
  for ($i = 0; $i < $router_item['number_parts']; $i++) {
    if ($link_map[$i] == '%') {
      $link_map[$i] = $path_map[$i];
    }
    if (isset($tab_root_map[$i]) && $tab_root_map[$i] == '%') {          // <-- HERE
      $tab_root_map[$i] = $path_map[$i];
    }
    if (isset($tab_parent_map[$i]) && $tab_parent_map[$i] == '%') {    // <-- HERE
      $tab_parent_map[$i] = $path_map[$i];
    }
  }

PS. There are plenty of D7 sites which are now forced to move to PHP 8.0. These minuscule nuances we are debating over (with a fix as simple as we have), make it really hard on those who have to support D7. I had to push back my deploy several times because such minor things popup where you wouldn't expect them.

To explain it a bit further - I understand that it can be frustrating to see such an easy fix to wait, but we are responsible for the Drupal 7 core to be (at least) stable, robust and fast. We need to be cautious and consider each change if it is suitable for core or not - for example introducing a lot of unnecessary conditions can make the core slower which is not what we want. Therefore, problems caused by contrib modules/custom code are considered specifically and the pros and cons are evaluated. If we find no problems, then it probably can be committed.

Specifically this case - I wrote some options on this. I still think that if it is caused by a contrib module (see the point 1.), then it is not the core responsibility to fix this, as the contrib module is sending a malformed router item. We have fixed such problems in the past (as an exception), but only if they were caused by multiple contrib modules (and it was unlikely they all will be fixed). So if this would be a problem of 1 or 2 contrib modules, they should fix it. Because if we would fix any such problem (e.g. "babysit" contrib code/data), it can result in more problems than benefits.

That said, it would be great if we can at least know the reason for this, so we can evaluate it and see what we can do. That does not mean this won"t get committed (there was a great progress with PHP 8/8.1/8.2 issues in core), but we need to be sure and have all information (to not only hide the problem with the fix). The similar opinions were presented before by former D7 maintainers like @David_Rothstein, @stefan.r, ...

I was unable to simulate the problem and we do not have any failing test for this, so someone else will need to provide these additional information (like the backtrace).

Thanks!