While menu_valid_path definitely shows we wanted to allow dynamic paths, this does not work (thanks to cwgordon7 who discovered they do not work though Charlie did not know that we intended them to work). Well, it's just a couple of to_arg functions missing and some checks against the global $menu_admin. After that I could vastly simplify menu_valid_path. Also, passing in TRUE as load argument did not work.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Note: another patch is to rip out those lines of menu_valid_path for D6 and deem this a feature request for D7. I am waiting for a decision on this and then I will add texts: doxygen and some user text in the description on the menu item add/edit screen to let the user know about these paths.

chx’s picture

FileSize
10.75 KB

So, the patch allows you to enter "edit current node" which is only active on node pages into the menu for example. All this is only needed if want that, "edit my account" needs much less. However, the latter was broken in the original patch, now "user/%/edit" behaves the same as "user/%" which can be quite expected.

chx’s picture

Title: Adding of dynamic paths are broken » "edit my account" does not work
Version: 6.x-dev » 7.x-dev
Category: bug » feature

We agreed with pwolanin that this is a feature.

chx’s picture

Title: "edit my account" does not work » Allow dynamic paths
pwolanin’s picture

Title: Allow dynamic paths » Adding of dynamic paths are broken
Version: 7.x-dev » 6.x-dev
Category: feature » bug

agreed - I recall the intent for D6 was to allow the editing (or addition, I guess) of items in the Navigaiton menu like user/% that always have a valid return from the _to_arg.

chx’s picture

Title: Adding of dynamic paths are broken » Allow dynamic paths
Version: 6.x-dev » 7.x-dev
Category: bug » feature

If agreed then you crossposted me.

lilou’s picture

Status: Needs review » Needs work

Patch partially applied :

--- Successfully Patched ---
modules\aggregator\aggregator.module
modules\contact\contact.module
modules\filter\filter.module
modules\forum\forum.module
modules\node\node.module
modules\taxonomy\taxonomy.module
--- Failed ---
includes\menu.inc (Cannot apply hunk @@ 7 )
modules\menu\menu.admin.inc (Cannot apply hunk @@ 7 )
modules\menu\menu.module (Cannot apply hunk @@ 6 )
modules\user\user.module (Cannot apply hunk @@ 6 )
DamienMcKenna’s picture

Would this allow the following scenario?

* Create a taxonomy vocabulary "product"
* Create the terms "cats", "dogs", "elephants" in the "product" taxonomy
* Create a view page named "product/%" that accepts an argument of a term from the "product" taxonomy
* Create a menu item with the link_path "products/cats"

Currently in D6 this is not possible, and is a major limitation to the flexibility of all modules that can create custom pages, e.g. Views, Panels, etc.

Damien

chx’s picture

Title: Allow dynamic paths » Allow menu links pointing to dynamic paths
Status: Needs work » Needs review
FileSize
14.58 KB

No, that's a whole another issue. This one is about the ability to add links pointing 'node/%/edit'. Edit: and similar dynamic paths. It adds a few cleanups/fixes:

  1. You can call menu_get_object safely any time because menu_get_item is now safeguarded from endless recursion.
  2. Indeed we call many times menu_get_object from the new _to_arg handlers.
  3. menu_valid_path becomes simpler because pointing to existing dynamic paths is simply allowed. This cleanup unlocks #190867: Remove access check for anonymous users when creating aliases btw.
  4. The menu interface handles properly dynamic links, there is not much to handle about them, just they can not be displayed as links because they do not have a fixed target so we simply display their title and add (dynamic).
  5. The test is amended to add a link to node/%/edit and check on it.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
15.59 KB

Fixed two new to_arg functions to use objects instead of arrays. I rolled #372914: Link titles are broken when using a non-t callback in this to fix blog. Just wasted an hour to figure out I am biten by that bug again, it'd been much easier if it'd been in.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
15.6 KB

I have overcorrected so now forum tests pass and User language settings 55 passes, 0 fails, and 0 exceptions too.

chx’s picture

FileSize
15.54 KB

With less debug. No other change. Thanks Dmitri.

stephthegeek’s picture

Please please please!

This is one of the questions I see most often on IRC/forums. It would make user profiles more linkable, for example if you're encouraging a user to fill out a certain section of their profile and need to have their UID in the path. Also an issue I see often with Ubercart -- you can't link to user/[uid]/files to send a user directly to their purchased files tab, order history, invoices, etc.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.74 KB

Keeping up with HEAD.

webchick’s picture

Status: Needs review » Needs work

From #drupal:
The thing I don't like about it is that it seems like *every* module that defines a %something arg needs to define a function like aggregator_feed_to_arg(). and if one of them doesn't, then the behaviour people get is inconsistent.

The only difference between menu_to_arg and node_to_arg is how you're doing menu_get_object and what $arg is returned. That seems like a lot of copy/pasting for module developers for two lines of code. We're also exposing menu system internals like $map and $index.

What if these functions were something like:

function node_to_arg($node) {
  if ($node) {
    return $node->nid;
  }
}

and the menu system handled the background stuff?

chx’s picture

Status: Needs work » Needs review
FileSize
15.52 KB

The problem is that while user_uid_optional_to_arg can happily work without having a $account as an input, the to_arg functions introduced here can not. What's more you need to run user_uid_optional_to_arg before you can load the current user, so it is not really possible to switch the order for that call. Also, the $map that is available in the translate functions is the exploded version of the link path not the $map of the router item. menu_get_object gives you access to the $map of the router item. So I propose the introduction of a wrapper around menu_get_object to significantly lower the amount of code to be copy-pasted.

chx’s picture

FileSize
14.61 KB

Huh, I left in some cruft in the beginning, sorry.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
14.88 KB

Keeping up with HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

I'm unlear from the whether a to_arg function is required or not for each % argument?

catch’s picture

Category: feature » bug

Bumping priority. Not being able to link to valid paths is a bug, and it's causing complaints at #308263: Allow privileged users to bypass the validation of menu items (which I downgraded).

SocialNicheGuru’s picture

will this work in D6?

chx’s picture

There is more , a lot more to this. We could, then, make tabs ordinary menu links themed as local tasks...

catch’s picture

Tabs as menu links would make things like og_panels a lot easier, and means you could do things like add a views argument as a tab.

drewish’s picture

subscribe

Damien Tournoud’s picture

Status: Needs work » Needs review

Let's see if this one still applies.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up
FileSize
14.88 KB

Re-rolled against latest HEAD.

This indeed qualifies as critical bug. Tagging to get into my list of issues to push.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
15.12 KB

This one will pass.

The concept needs work though.

mattyoung’s picture

.

Status: Needs review » Needs work

The last submitted patch failed testing.

q0rban’s picture

Subscribe

andypost’s picture

Subscribe, patch outdated

andypost’s picture

fizk’s picture

subscribe

Damien Tournoud’s picture

@andypost: those issues are pretty much unrelated.

chx’s picture

Assigned: chx » Unassigned
pwolanin’s picture

bump - this should still be a critical fix for D7

sun.core’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Normal
klonos’s picture

subscribing...

Enemy’s picture

Status: Needs work » Needs review

#9: dynamic_menu_links.patch queued for re-testing.

sun’s picture

Related contrib module: http://drupal.org/project/menu_token

andypost’s picture

andypost’s picture

Refactoring of menu in #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel will make this obsolete

kscheirer’s picture

So is this issue now obsolete?

Status: Needs review » Needs work

The last submitted patch, 34: drupal.menu-links-dynamic.34.patch, failed testing.

Fabianx’s picture

Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes

This deals with paths, which do not exist in D8 anymore => Back to 7.x