To replicate change the Menu Title for "My Account" to anything else and click save.

This will change it, but Drupal throws an undefined variable message at you.
user-links-undefined-variable.png

Files: 
CommentFileSizeAuthor
#36 path-876580-36.patch3.94 KBpingers
FAILED: [[SimpleTest]]: [MySQL] 56,761 pass(es), 46 fail(s), and 18 exception(s).
[ View ]
#31 path-876580-31.patch3.94 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,400 pass(es), 46 fail(s), and 18 exception(s).
[ View ]
#30 path-876580-30.patch2.89 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,115 pass(es), 46 fail(s), and 18 exception(s).
[ View ]
#21 876580.patch1.88 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 35,624 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#15 form_item_cruft-876580-d7.patch686 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_item_cruft-876580-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 form_item_cruft-876580-14.patch706 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]
#11 876580.patch686 bytesgrendzy
PASSED: [[SimpleTest]]: [MySQL] 33,307 pass(es).
[ View ]
#7 MyBlogError.png46.55 KBStayingEnPointe.com
user-links-undefined-variable.png37.93 KBJeff Burnz

Comments

Jody Lynn’s picture

Great find. I tracked down the problem to an oversight in drupal_valid_path which was introduced in #190867: Remove access check for anonymous users when creating aliases.

marcingy’s picture

Status:Active» Closed (fixed)

This no longer happens on head.

tedfordgif’s picture

Component:menu.module» path.module
Status:Closed (fixed)» Active

The offending code is still in 7.x and 8.x, so the underlying issue is not fixed.

<?php
 
elseif ($dynamic_allowed && preg_match('/\/\%/', $path)) {
   
// Path is dynamic (ie 'user/%'), so check directly against menu_router table.
   
if ($item = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $path))->fetchAssoc()) {
     
$item['link_path']  = $form_item['link_path'];
     
$item['link_title'] = $form_item['link_title'];
     
$item['external']   = FALSE;
     
$item['options'] = '';
     
_menu_link_translate($item);
?>
marcingy’s picture

Status:Active» Postponed (maintainer needs more info)

Can you provide steps to replicate then as following the instructions in step one I could not recreate on head. Or a test that will cause the failure.

tedfordgif’s picture

Do you have display of notices turned on in your php.ini? If that doesn't do it, I should be able to write a test -- just call drupal_validate_path('node/%', TRUE);.

Ingmar’s picture

I also have this error...everytime I try to add a menu-item of change one.
The record is written to the database, but it doesn't show in Drupal.

StayingEnPointe.com’s picture

StatusFileSize
new46.55 KB

Subscribing!

I'm having the same problem when I try to change the Parent Link of the "My Blog" menu item.

Does anyone know of a work-around for this problem until it's fixed?

Edit: I was able to work around this by editing the DB entries directly, then I had to reload the theme cache by accessing the "Admin/Appearance/" menu.

_KASH_’s picture

Subscribing

_KASH_’s picture

This happens for me if I add a link to the user menu that uses the % to insert the user id to the menu path. For example user/%/bookmarks

_KASH_’s picture

Status:Postponed (maintainer needs more info)» Active

active

grendzy’s picture

Title:User menu undefined variable notice if you change the My Account title» Notice: Undefined variable: form_item in drupal_valid_path
Version:7.x-dev» 8.x-dev
Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new686 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,307 pass(es).
[ View ]

Edit: not sure this is right, _menu_link_translate() might need link_path to have some value...

mefisto75’s picture

same as at #7

xjm’s picture

Status:Needs review» Needs work

Edit: $form_item does not actually appear to be set anywhere.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new706 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]

I did a little git detective work and this dates to #151583: Menu - Fixes that make menu module work in D6 in D6, in menu_valid_path(). Back then $form_item was the name of the function's parameter. Since $path is now just a string, the patch is correct. There's no need to override values loaded from the DB and nothing to override them with.

Here's a reroll for core/.

xjm’s picture

StatusFileSize
new686 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_item_cruft-876580-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

D7 version attached for convenience and testing. If you are encountering this problem, please confirm whether applying this patch and clearing your cache resolves the issue.

grendzy’s picture

I'm starting to suspect the whole $dynamic_allowed thing is conceptually flawed. For example, how could drupal_valid_path('node/%', TRUE) possibly be evaluated if the node ID is unknown? The function is expected to check access, and _menu_check_access() needs to have the access arguments. Can anyone shed some light here?

xjm’s picture

#16: I think the answer to that question is outside the scope of this issue? It's not that the nid is not known; it's that it's a part of the pattern. I may be misunderstanding the question, though. :)

The cleanup above should be made regardless, but let's have someone who can reproduce the bug test the patch and confirm whether it resolves the issue, or whether the part of the problem where the menu items are broken persists.

xjm’s picture

xjm’s picture

Also, WRT the sort of tests that need to be added, from the other issue:

For tests I mean that the fact all core tests pass at the moment means we must have no tests in core that cause this hunk to be evaluated to true - if we did then we'd be getting PHP errors.

if ($item = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $path))->fetchAssoc()) {

So without tests for that code, if another regression was introduced into this function in the same place, we'd not know, hence it makes sense to add them while fixing this.

xjm’s picture

Status:Needs review» Needs work

Setting NW for tests.

Jody Lynn’s picture

StatusFileSize
new1.88 KB
FAILED: [[SimpleTest]]: [MySQL] 35,624 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

I added a test that adds a dynamic menu item and I can't get it to work.

_menu_link_translate always gives back $item['access'] = FALSE for dynamic paths and I can't make much sense out of that function in general.

So currently it is impossible to add dynamic paths to menus, making this a pretty serious bug.

Jody Lynn’s picture

Title:Notice: Undefined variable: form_item in drupal_valid_path» drupal_valid_path allows fails for dynamic paths
Jody Lynn’s picture

Jody Lynn’s picture

Title:drupal_valid_path allows fails for dynamic paths» drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus)
droplet’s picture

Status:Needs work» Needs review

see what testbot said.

Status:Needs review» Needs work

The last submitted patch, 876580.patch, failed testing.

kiamlaluno’s picture

I still have to understand what the purpose of allowing users to define a menu item with a link containing % is, if drupal_valid_path($path, TRUE) returns TRUE only for tracker/%, between the default paths defined from Drupal.

I tried the following code, and $result is TRUE only when $path is equal to 'tracker/%'.

<?php
global $menu_admin;
$menu_admin = TRUE;

if (
$item = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $path))->fetchAssoc()) {
 
$item['link_path']  = $path;
 
$item['external']   = FALSE;
 
$item['options'] = '';
 
_menu_link_translate($item);
}

$menu_admin = FALSE;
$result = ($item && $item['access']);
?>

The first condition necessary for drupal_valid_path() to return TRUE is that the menu item has to_arg functions; the condition is verified for search/node/%, search/user/%, and tracker/%. Between those paths, drupal_valid_path($path, TRUE) returns TRUE only for tracker/%.

YesCT’s picture

TonyK’s picture

This issue is almost 3 years old. Will this ever be fixed in Drupal 7?

dawehner’s picture

StatusFileSize
new2.89 KB
FAILED: [[SimpleTest]]: [MySQL] 56,115 pass(es), 46 fail(s), and 18 exception(s).
[ View ]

It is a huge problem that we need to support both the old and the new routing system at the same time.

Here is a rough outline how to support the new routing system on top of the menu_router bit in drupal_valid_path.

dawehner’s picture

StatusFileSize
new3.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,400 pass(es), 46 fail(s), and 18 exception(s).
[ View ]

This fixes dynamic paths for the new routing system, but NOT for the old one, see @todo

pc-wurm’s picture

Version:8.x-dev» 7.x-dev

I'm setting the issue back to 7.x.

mradcliffe’s picture

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review

I think this needs to be 8.x so #31 can get tested.

Status:Needs review» Needs work

The last submitted patch, path-876580-31.patch, failed testing.

pingers’s picture

Priority:Normal» Major

This is related to #2086559: Adding an invalid path alias shows an exception in the UI, in which a ResourceNotFoundException is causing WSOD.

For this reason, I'm promoting this to major and we should test that aliasing a path such as "admin/invalid" at admin/config/search/path/add does not WSOD.

I'll try to re-roll what's there to catch this Exception. I.e. checking if a path is valid should not cause WSOD.

pingers’s picture

Status:Needs work» Needs review
StatusFileSize
new3.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,761 pass(es), 46 fail(s), and 18 exception(s).
[ View ]

Okay, here's the re-roll.

Status:Needs review» Needs work
Issue tags:-Needs tests, -needs backport to D7

The last submitted patch, path-876580-36.patch, failed testing.

tim.plunkett’s picture

Priority:Major» Normal
Issue summary:View changes

Did this work in D6? I can reproduce in D7, meaning that this is not a regression.

David_Rothstein’s picture

Note that #2106129: drupal_valid_path() Notice: Undefined variable: form_item is semi-duplicate of this issue but now has commits for both Drupal 7 and 8. I'm guessing some of this issue is still valid though (especially the tests).