Separate Tabs (MENU_LOCAL_TASK) from hook_menu()

Tagging

generally speaking, this looks sane. i'm still a touch weirded out by the use of plugins for this, but that's ok.

the thing that's keeping me from RTBCing this is how the logic has been split. MenuLocalTaskManager::getTasksBuild() is the only public method that's ever actually called on this thing, and it seems rather singly-focused on doing the exact same thing as the rest of the logic in menu_local_tasks(). if there isn't another use case for calling that method, then i think that logic should be moved back in to menu_local_tasks(), at which point the other public methods on MenuLocalTaskManager will suddenly have a reason to be public. otherwise, we're effectively just dividing one logical task into two separate places - and to find the second place from menu_local_tasks(), you have to check system.services.yml to find the class that actually comes back from plugin.manager.system.menu_local_task.

from what i can tell, the only thing that would really need to change in moving it back out to the function is asking the container for the route provider directly. if that's the case, we can hit \Drupal for that.

i imagine that your response to this will be, "we have it this way so people can swap the logic," and/or "we're intending to get rid of the rest of menu_local_tasks() and have it just be this." if that's the case, i could probably be convinced to RTBC as-is, but for the former i'd like more details on whether or not such a use case is realistic, and for the latter i'd like to see a followup issue for that removal :)

Add a first basic unit test for the local task manager.

Added some of the followups to the summary.

@pwolanin added comments and opened a followup for removing the rest of menu_local_tasks() at #2032587: Clear menu_router code out of function menu_local_tasks() when all actions and tabs are converted to plugins. that meets my criteria. tim can knock it back if he's not OK :)

RTBC +1, this works for me.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Fixing some minors... Also, same thing as over at #2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths: We should not put the controller resolver resolved methods in the interface.

Also, setters should always return $this imho unless they can return something else that makes sense. The rest are just name changes, 180 character limit fixes and fixing copy&paste issues (local actions/local tasks).

Personally I prefer the interface one, as we do the same with for form interface as well and an interface is a nicer documentation then an exception but I won't block that.

Personally I prefer the interface one, as we do the same with for form interface as well and an interface is a nicer documentation then an exception but I won't block that.

Forms are weird and special because of FormAPI and the form state/form array we got there. I'd prefer properly calling the form builder methods there at some point too though. But currently, as you will see by looking at HtmlFormController we do a lot of weird things with $form and $form_state and the request at that point. We may be able to fix that later. I hope so at least.

No no no no no.

I pushed back on the other issue about exceptions vs interfaces, lets not have this debate again. We agreed to use interfaces, and the patch was green and RTBC.

The unit tests were stile trying to invoke the getTitle() / getPath() method on the interface mocks which don't have that anymore. Mocking the base class instead and testing for \BadMethodException when the method is missing.

These unit tests addition looks great!

Daniel, calm down. Every time you see a new unit test you completely freak out :D.

dawehner++

I withdraw my RTBC. This goes against the consensus we had.

Okay, we agreed on IRC that we will do the interface/no interface thing in a follow-up. Daniel is going to come back with a re-roll (I gotta run in a few, office closes).

This adds #26 + #21 without the code changes.

Follow up: #2035105: Figure out how to inject abritrary arguments into controllers/local action/local tasks whatever else plugins

#31: drupal-2004334-30.patch queued for re-testing.

Damn.

Going back to mocking the interface instead of the base class now that we got the methods on the interface again.

This changes looks great, but yeah they don't matter that much.

wth just happened since #20? can you say in englishish?

Since #20? @fubhy rewrote the patch with a new approach, added better docs, and the we agreed to return to the old approach.
So it should be almost the same as #20, but with better docs.

Someday we'll fix that bug.

wth just happened since #20? can you say in englishish?

We do not have the perfect solution for the getTitle/getPath methods yet and neither the patch from #17 nor the patch from #21 should be considered a final solution. It's not an easy problem for which we don't have a solution yet, so Daniel opened a follow-up to discuss that and we agreed to revert back to #17 as that's what we previously had consensus about.

In short, the question is: How do we 'inject' contexts into plugins. The problem basically is that due to these dynamic contexts the methods signature would be different for each plugin (if we assume that we want to forward the contexts as method arguments, which is what this patch does) which makes using an interface rather hard/weird because in order to match the interface all the additional, dynamic arguments of the method have to be declared as optional by setting a default value ($foo = NULL).

/**
 * Implements getTitle() from the interface but the interface does not have any arguments on the method.
 */
function getTitle($foo = NULL, $bar = NULL) {
  // Do something with $foo and $bar which, even though they are declared optional
  // (in order to comply with the interface) they are really not optional because we
  // need them for this method in any case.
}

Here is the follow-up: #2035105: Figure out how to inject abritrary arguments into controllers/local action/local tasks whatever else plugins

c'mon d.o ...

This needs a reroll in the theme of #2031473-28: Convert menu local actions to plugins so that we can generate dynamic titles and paths, including the removal of ContainerFactoryPluginBase (#2033447: Remove obsolete ContainerFactoryPluginBase).
I'll try to get to this during lunch or this evening, but if someone wants to jump in before me, feel free.

Assign to myself.

+++ b/core/core.services.ymlundefined
@@ -169,6 +169,9 @@ services:
+  plugin.manager.local_task:

In the other, we had plugin.manager.menu.local_action, with the extra .menu.

+++ b/core/includes/menu.incundefined
@@ -2117,6 +2121,16 @@ function menu_local_tasks($level = 0) {
+      // The default manager class is \Drupal\Core\Menu\LocalTaskManager.

Is this really needed? I can't think of another time we've done this.

+++ b/core/lib/Drupal/Core/Annotation/Menu/LocalTask.phpundefined
@@ -0,0 +1,70 @@
+   * @var array

@var array|null (optional)

+++ b/core/lib/Drupal/Core/Annotation/Menu/LocalTask.phpundefined
@@ -0,0 +1,70 @@
+  /**
+   * The weight of the tab.
+   *
+   * @var int|NULL
+   */
+  public $weight;

This could probably have a default of 0 just for consistency.

+++ b/core/lib/Drupal/Core/Annotation/Menu/LocalTask.phpundefined
@@ -0,0 +1,70 @@
+   * @var int|NULL

@var int|null (optional)

+++ b/core/lib/Drupal/Core/Annotation/Menu/LocalTask.phpundefined
@@ -0,0 +1,70 @@
+   * @var array

@var array (optional)

+++ b/core/lib/Drupal/Core/Menu/LocalTaskInterface.phpundefined
@@ -0,0 +1,78 @@
+  /**
+   * Returns the localized title to be shown for this tab.
+   *
+   * Subclasses may add optional arguments like NodeInterface $node = NULL that
+   * will be supplied by the ControllerResolver.
+   */
+  public function getTitle();
...
+  /**
+   * Returns an internal Drupal path to use when creating the link for the tab.
+   *
+   * Subclasses may add optional arguments like NodeInterface $node = NULL that
+   * will be supplied by the ControllerResolver.
+   */
+  public function getPath();

Missing @return

+++ b/core/lib/Drupal/Core/Menu/LocalTaskInterface.phpundefined
@@ -0,0 +1,78 @@
+   * @see l().

no . at the end of @see

Thank you for the review.

This could probably have a default of 0 just for consistency.

There is a logic in the localtaskBase which sets the weight to -10, if the tab is basically the default tab. This is only applied if this value was not specified (null), but is not triggered when 0 is specified.

#48: local-task-2004334-48.patch queued for re-testing.

Thanks, the -10 part is a perfect explanation :)

This patch does not seem to apply. Asking for a re-test.

#48: local-task-2004334-48.patch queued for re-testing.

Manually tested, and the patch is identical.

Committed to 8.x. Thanks!

This needs a change notice.

Updating title.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -0,0 +1,227 @@
+      // @todo - optimize this lookup by compiling or caching.

Was this profiled?

Is there a follow-up for the @todo?

DefaultPluginManager gives you static caching for free, you have to do some extra work in the constructor for persistent caching (which I don't know that we want anyway).

(which I don't know that we want anyway).

That's why we should profile it ;)

$manager->clearCachedDefinitions()

Right now that clears the static cache, if we add persistent caching it will handle that as well.

Note that the MENU_LOCAL_TASK type did more than just present it as a local task (tab), it also manipulated the breadcrumb trail. In order to preserve that behavior each tab should have an entry of type MENU_VISIBLE_IN_BREADCRUMB left in hook_menu, at least until the breadcrumb code is made aware of the local task plugins.

Do we have an issue for this?

Reopening.

The change notice at https://drupal.org/node/2044515 is not clear at all.

Round about this point:

"These example tabs are simply using the base class - and all the needed definition information is encapsulated in the YAML:"

Each local task is represented by a plugin [how?]. Each plugin definition (found in the yml file [which yml file? routing.yml, or another one?]) contains three to five loal-task specific keys underneath the top-level key which is the unique plugin ID: [how?]

A few improvements: https://drupal.org/node/2044515/revisions/view/2835083/2838581

oh tags ...

I think we might use this, or make a meta to go through each module and convert their use of the hook to plugins.

there could be a problem though, See config_translation issue #2044737: Provide local tasks as plugins and core issue #2095325: using plugins to add tabs is not as capable as hook_menu()

The change record doesn't explain what the plugins are like -- what class are they? What plugin type?

> Note that you have full control over this behavior by implementing your own plugin class for a specific tab.

How do you do that?