Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()

The meta issue is major, we don't need this to be as well.

I'll take this one

So I'm really not sure where to go with this one. For example, I don't even know where MENU_LOCAL_ACTION will go when we rip it out, or what interface/class to implement/extend. Sorry...

I just spent a good amount of time with local actions thanks to #1938960: _menu_translate() doesn't care about new routes, going to take a look

Views UI is the only hook_menu() whose MENU_LOCAL_ACTIONs are routes that reference other paths that are routes. More conversions will have to follow.

To do this, I had to clean up _menu_router_merge_route().
It has been tweaked over time, and currently takes a $path and discards it directly.
Also, the entire point of the function is to populate the path from a route name.
So I've renamed it and repurposed it for just that.

This contains #1938960: _menu_translate() doesn't care about new routes.

It's so sad, I've worked so hard to kill off info hooks this cycle, yet this would mark the third new info hook I'm responsible for...

I'll mull over the naming while I write a test.

Okay, made the switch to 'appears_on', and here's a test.

I can only see nitpicks here

+++ b/core/includes/menu.incundefined
@@ -2215,6 +2215,19 @@ function menu_secondary_local_tasks() {
+  foreach (module_invoke_all('local_actions_info') as $route_name => $route_info) {

module_invoke_all deprecated?

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -416,10 +416,36 @@ function menu_test_menu() {
+ * Implments hook_local_actions_info().

Implements

Good catch!
Converted picture and aggregator since there were move conversions committed.

Postponed on http://drupal.org/project/issues/search/drupal?issue_tags=MENU_LOCAL_ACTION

+++ b/core/includes/menu.incundefined
@@ -2215,6 +2215,19 @@ function menu_secondary_local_tasks() {
 function menu_local_actions() {
   $links = menu_local_tasks();
+  $router_item = menu_get_item();
+  foreach (Drupal::moduleHandler()->invokeAll('local_actions_info') as $route_name => $route_info) {
+    if (in_array($router_item['route_name'], $route_info['appears_on'])) {
+      $route_path = _menu_router_translate_route($route_name);
+      $links['actions'][$route_path] = array(
+        '#theme' => 'menu_local_action',
+        '#link' => array(
+          'title' => $route_info['title'],
+          'href' => $route_path,
+        ),
+      );
+    }
+  }
   return $links['actions'];
 }

There is just a single place there this function is used so we maybe can move this into some kind of object already?

Okay. MENU_LOCAL_ACTION does two things.

1. When on the parent page, the action link is displayed
2. When on the action page, the tabs of the sibling pages are displayed

This issue is just about the former. It does not let us remove the hook_menu() definition.

But to better differentiate these two things, we *can* start converting now, and just have an identical bitmask menu type with a different name.

Introducing MENU_SIBLING_LOCAL_TASK.
Let's see how it fares.
---
@dawehner, I'm not sure what that would look like, especially with the menu_get_item() and menu_local_tasks() calls.

Here is my reasoning for introducing a duplicated constant (MENU_SIBLING_LOCAL_TASK and MENU_LOCAL_ACTION have the same value):

We are attempting to replace the display of menu local actions on their parent page (the functionality of menu_local_actions())

We are NOT attempting to rewrite _menu_router_build(), where it checks hook_menu() in order to build the breadcrumbs and decide what tabs to show on the action's page itself.

Once this issue is in, and all existing MENU_LOCAL_ACTIONs are converted, we will still be left with a hook_menu() entry for that. Yet it will not be for anything action-related, just about its behavior as a pseudo-local-task. Hence MENU_SIBLING_LOCAL_TASK.

The added benefit will be knowing which actions remain (since they will have the old constant).

Fixed the docs, and added another conversion.

+++ b/core/includes/menu.incundefined
@@ -2215,6 +2223,19 @@ function menu_secondary_local_tasks() {
+  foreach (Drupal::moduleHandler()->invokeAll('local_actions_info') as $route_name => $route_info) {
+    if (in_array($router_item['route_name'], $route_info['appears_on'])) {

Maybe we should come later here, as this maybe not scale if there are a lot of local actions defined.

+++ b/core/includes/menu.incundefined
@@ -2215,6 +2223,19 @@ function menu_secondary_local_tasks() {
+      $route_path = _menu_router_translate_route($route_name);
+      $links['actions'][$route_path] = array(
+        '#theme' => 'menu_local_action',

maybe it would be nice to have some access checking on there ...

+++ b/core/includes/menu.incundefined
@@ -2215,6 +2223,19 @@ function menu_secondary_local_tasks() {
+          'title' => $route_info['title'],

... We want to be able to translate the title ...

+++ b/core/modules/picture/picture.moduleundefined
@@ -48,6 +48,20 @@ function picture_permission() {
+    'picture_mapping_page_add' => array(

Let's move the route_name to the definition, as it's for example more consistent to the way hook_menu looks at the moment.

Fixed all of those but the first, where I added a @todo to consider adding static caching later.

+++ b/core/includes/theme.incundefined
@@ -2900,7 +2900,7 @@ function template_preprocess_page(&$variables) {
+  $variables['action_links']      = menu_get_local_actions();

I will not bitch about silly spaces!

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -432,13 +432,13 @@ function menu_test_menu() {
- * Implements hook_local_actions_info().
+ * Implements hook_local_actions().

Good idea!

#26: local-action-1908756-26.patch queued for re-testing.

Committed 00fdcd3 and pushed to 8.x. Thanks!

+++ b/core/modules/system/tests/modules/menu_test/menu_test.module
@@ -416,10 +416,37 @@ function menu_test_menu() {
+ * Implements hook_local_actions().

Any specific reason why this was changed from hook_local_action_info()? It seems the latter is a standard throughout core, that would make sense following here, IMO.

We're actually in the midst of killing info hooks. In the above patches it was named that, I was asked to rename it at Drupalcon.

Hmm... still seems strange to not apply an existing pattern just because the whole concept is sort of deprecated-ish. Not a big deal, either way, though, so I'll shut up.

Opened #2006624: Implement LocalAction plugins for previously converted routes

Added https://drupal.org/node/2007444

Change notice looks good.