We still need to figure out #1981644: Figure out how to deal with 'title/title callback' and #2023795: REGRESSION: hook_local_actions doesn't use title callback, but here's a fix for a fun bug we've hacked around.

CommentFileSizeAuthor
#7 6-7-interdiff.txt3.73 KBalexpott
#7 menu-2027183-7.patch7.29 KBalexpott
#6 menu-2027183-6.patch6.44 KByesct
#6 interdiff-4-6.txt561 bytesyesct
#5 interdiff.txt2.02 KBtim.plunkett
#4 menu-2027183-4.patch6.43 KBtim.plunkett
#3 menu-2027183-3a.patch6.32 KBtim.plunkett
#3 menu-2027183-3b.patch6.44 KBtim.plunkett
#1 menu-2027183-1-FAIL.patch2.35 KBtim.plunkett
#1 menu-2027183-1-PASS.patch6.18 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new menu-2027183-1-PASS.patch6.18 KB
new menu-2027183-1-FAIL.patch2.35 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/includes/menu.incundefined
@@ -953,23 +954,38 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+    if (isset($path_bits[$index]) && preg_match('/{(?<placeholder>.*)}/', $path_bits[$index], $matches)) {

We should certainly explain this line or even lines.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Normal » Critical
StatusFileSize
new menu-2027183-3b.patch6.44 KB
new menu-2027183-3a.patch6.32 KB

Posting two patches, I can't remember why I made the one change. If both pass, it was leftover. If the one fails, I'll know why :)

Also, this is being coded around in patches like #1984702: Convert menu.module's page callbacks to Controllers and #111715: Convert node/content types into configuration, and was hacked around in the Field UI conversion.

But I'm calling this a pretty big regression, even if we DO want to kill title callback...

EDIT:
The difference for those curious is

@@ -777,9 +777,10 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
   $router_item['tab_parent_href'] = implode('/', $tab_parent_map);
   $router_item['options'] = array();
   if (!empty($router_item['route_name'])) {
+    $map = $link_map;

As seen in the first hunk of 3b

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new menu-2027183-4.patch6.43 KB

Oh, now I remember :) This is used by dynamic titles on local actions, but could be elsewhere. See interdiff (against the patch in 1 since I forgot one last time).

The hunk in field_ui.module explains the bug.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new interdiff.txt2.02 KB

Here's the missing interdiff.

yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff-4-6.txt561 bytes
new menu-2027183-6.patch6.44 KB

I read it closely and it seems reasonable to me.
Does this need to get looked at by someone else, with more knowledge in the area?
Only thing I found was one comment not a sentence with a weird ex: and lots of parentesis.
I fixed it.
But it turns out, that's copy and paste.
Still, hope it's not too disruptive, and made #2027351: tidy up grammar in doc copied around: (ex: array('node', '5')) to take care of the rest.
Since this thing I found is not a core gate blocker, if preferred, could just use the patch from #4.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new menu-2027183-7.patch7.29 KB
new 6-7-interdiff.txt3.73 KB 
+++ b/core/includes/menu.incundefined
@@ -953,23 +957,41 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+function menu_item_route_access(Route $route, $href, &$map) {
   $request = Request::create('/' . $href);
   $request->attributes->set('system_path', $href);
   // Attempt to match this path to provide a fully built request to the
   // access checker.
   try {
     $request->attributes->add(Drupal::service('router.dynamic')->matchRequest($request));
-    return Drupal::service('access_manager')->check($route, $request);
   }
   catch (NotFoundHttpException $e) {
     return FALSE;
   }
+
+  // Populate the map with any matching values from the request.
+  $path_bits = explode('/', trim($route->getPath(), '/'));
+  foreach ($map as $index => $map_item) {
+    $matches = array();
+    // Search for placeholders wrapped by curly braces. For example, a path
+    // 'foo/{bar}/baz' would return 'bar'.
+    if (isset($path_bits[$index]) && preg_match('/{(?<placeholder>.*)}/', $path_bits[$index], $matches)) {
+      // If that placeholder is present on the request attributes, replace the
+      // placeholder in the map with the value.
+      if ($request->attributes->has($matches['placeholder'])) {
+        $map[$index] = $request->attributes->get($matches['placeholder']);
+      }
+    }
+  }
+
+  return Drupal::service('access_manager')->check($route, $request);

It looks like menu_item_route_access is doing unrelated stuff on the map. I think this should be refactored into a separate function.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

For menu_item_populate_map() to work it HAS to be called after menu_item_route_access(), because it needs the matchRequest() part called. That seems very brittle to me, since it makes the new function useless on its own.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Furthermore, menu_item_route_access() has been refactored and rescoped several times, and isn't really an API function (I made it up).

It should really be called _menu_item_route_manipulation_du_jour(), but I doubt that would fly.

Instead of introducing a new function that is dependent on another function being called first, I'd rather see it combined as in #4, and the function renamed.

If we ever manage to combine _menu_translate() and _menu_link_translate(), we'll be able to move this function inline.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

So this patch improves and fixes stuff and on the longrun it will have to look different anyway.
Let's move forward and RTBC #6

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 0cb8f16 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.