drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus)

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.

This no longer happens on head.

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

  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);

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.

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);.

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.

Subscribing!

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

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.

Subscribing

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

active

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

same as at #7

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

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/.

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.

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?

#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.

Marked #1325984: drupal_valid_path() uses a non defined variable as duplicate.

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.

Setting NW for tests.

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.

Closed #1256978: drupal_valid_path: fix&document OR refactor as a duplicate

see what testbot said.

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/%'.

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/%.

related: #1634916: "My account" used across site although "User account" is shown for Menu link title and Path

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

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.

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

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

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

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.

Okay, here's the re-roll.

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

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).