Problem/Motivation

Currently, you have to implement an empty class for any local task plugin. This is a bit weird, you are then just using the empty class for the annotation. It makes sense, similar to routes, to have YAML discovery for these, then static local tasks just get added into a $module.localtasks.yml file. This seems like better UX, as people can then actually create a plugin class for dynamic local tasks.

API changes

Annotations for local tasks go away.
Caching is removed and will be handled in a follow since the strategy is being developed in parallel. (related issue below)

#2050227: Add local action plugin deriver to use YAML discovery for static definitions
#2032303: Cache the result of local tasks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +Drupal wtf

+1

damiankloip’s picture

Status: Active » Needs review
FileSize
11.23 KB

Here's an initial patch for this. I have converted views local tasks and removed all the empty plugin classes.

The main thing that concerns me, is if we use a the standard derivative id pattern of "$base_plugin_id:$derivative_id" then the parent/root tab ids have to also reflect this, see the currently converted views_ui.local_tasks.yml file in the patch.

Status: Needs review » Needs work
Issue tags: -Drupal wtf

The last submitted patch, 2050919.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Drupal wtf

#2: 2050919.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/views_ui/views_ui.local_tasks.yml
@@ -0,0 +1,22 @@
+views_ui_settings_tab:
...
+  tab_root_id: local_task_static:views_ui_list_tab
+  tab_parent_id: local_task_static:views_ui_settings_tab
...
+views_ui_list_tab:

This is a huge WTF. I'm not sure if the derivative system will let us work around this though?

damiankloip’s picture

Yep, that's exactly what I was saying in #2.

I guess we could implement our own discovery decorator? or just have the static plugin if as 'static' (or something), so the ids could be 'static:views_ui_list_tab' maybe?

dawehner’s picture

I talked with neclimdul yesterday and to be honest he wasn't really convinced that local tasks as plugins are a good idea, so we didn't made any progress on that regard.

We could either implement our own (urgs) or allow the default one to be "configured".

neclimdul’s picture

Here's an alternative. Its a patch based on #2065571: Add YAML Plugin discovery. Its incomplete, notably missing translation and caching. This was mostly because of the @todo about caching already in the Manager which this bespoke Manager could more properly handle. Solving that @todo was a larger task though with bigger picture concerns that need to be discussed.

Status: Needs review » Needs work

The last submitted patch, yaml_discovery-local-tasks.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
13.21 KB

diffed against old branch by accident.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -20,7 +24,25 @@
+  protected $defaults = array(
+    // The ID.
+    'title' => '',
+    // The static title for the local task.
+    'id' => '',
+    'route_name' => '',
+    // The plugin ID of the root tab.
+    'tab_root_id' => '',
+    // The plugin ID of the parent tab (or NULL for the top-level tab).
+    'tab_parent_id' => NULL,
+    // The weight of the tab.
+    'weight' => NULL,
+    // The default link options.
+    'options' => array(),
+    // Default class for local task implementations.
+    'class' => 'Drupal\Core\Menu\LocalTaskDefault',
+  );

This can go on the LocalTask annotation class

neclimdul’s picture

LocalTask annotation goes away because we aren't using annotations at all. That's not how this system works.

Status: Needs review » Needs work

The last submitted patch, yaml_discovery-local-tasks.patch, failed testing.

neclimdul’s picture

apologies to tim for my last comment. it was correct but i needed a snickers. :)

Fix most of the failures. Root test is still failing and I don't see why($this->drupalGet('menu-local-task-test/tasks/view');). Also, added a test case for "dynamic" titles.

damiankloip’s picture

IMO we should really check that we can make this change instead of just the addition first.

dawehner’s picture

Issue tags: +API change

Adding an api change tag.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
593 bytes
19.36 KB

PEBKAC

Status: Needs review » Needs work

The last submitted patch, yaml_discovery-local-tasks-17.patch, failed testing.

neclimdul’s picture

I'm the worst. I uploaded the wrong patch but the right interdiff. Sorry for the noise.

neclimdul’s picture

Status: Needs work » Needs review
damiankloip’s picture

The patch is looking pretty good but should we not be working on the dependency first? i.e. #2065571: Add YAML Plugin discovery? As that will block this anyway.

For instance, the latest patch over there does not override the provider key, as this could be set on behalf of another module.

I will ask one of the core maintainers to give their opinion on this today.

As a side note, you should not criticise the work others do so much, it's not constructive :/ The derivative pattern was added (yes, it's not ideal) to have the minimum changes to the API (just additions).

pwolanin’s picture

Status: Needs review » Needs work

So I think the pattern used with local tasks should be the right one here also - we want a convenient YAML derivative provider, but we need to allow for modules (e.g. Views) to provide other derivatives. Or maybe I'm missing the flow.

in any case, I think this needs a re-roll since the standalone YAML Plugin discovery was committed.

damiankloip’s picture

Local actions?

Well, first we still need confirmation that this API change is allowed. The current patch will render all current plugins for local tasks broken. Which is why local actions currently has the derivative YAML pattern and not this one.

It is more of an issue here though, as local tasks reference other local tasks.

This is why I added a discovery decorator over in #2065571: Add YAML Plugin discovery, as a contingency plan. That got removed whilst I was asleep though.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.83 KB

Here is a reroll anyway.

catch’s picture

Adding YAML discovery is fine, removing annotation discovery I don't see any reason to do that.

The only reason I can think of would be if the directory scans from both YAML + annotation discovery were too expensive on cache misses but no-one's brought that up on here (or any other real reason for removing it that I can see in this issue).

neclimdul’s picture

We're removing annotation discovery from local tasks because we're not using it. Why would we keep it?

neclimdul’s picture

Another reason we'd want to lean toward yaml only that catch wanted documented is that mixing the two means we have to scan directories for annotations and a separate set of directories for yaml files. 2 scans is less good.

damiankloip’s picture

Title: Add local task plugin deriver to use YAML discovery for static definitions » Replace local task plugin discovery with YamlDiscovery

This reflects better what we want to do?

pwolanin’s picture

@neclimdul - whatever we do, we need to have a simple way for modules like Views or Search to add local tasks dynamically. I'm not sure how the derivative decorator interacts with this YAML discovery?

neclimdul’s picture

@pwolanin is there not a test for this in core already? It should be handled by the derivative decorator being used by all other plugins system.

pwolanin’s picture

@neclimdul - had some discussion with EclipseGC about this to clarify my understanding - seems indeed like it should be handled, but it would be nice to demonstrate it.

pwolanin’s picture

I'm confused as to why are you creating a new LocalTaskDefault and not just using LocalTaskBase?

also, why comment out the alter hook?

+    // @todo caching/compiling, translations and altering.
+    // $this->alterInfo($module_handler, 'local_tasks');
neclimdul’s picture

re LocalTaskDefault I can't decide if it was because I was moving the static discovery from before or if its because Base is designed to be extended by all tasks and default is explicitly the default. I kinda like the way it is but if there's a case for removing it I'm open to hearing the argument.

re: todo because caching, altering, and translations are closely connected I was leaving it up to the follow up to get it right. We can't just uncomment it because we're not inheriting the DefaultPluginManager but we can put some sort of logic in if its needed.

pwolanin’s picture

From IRC - you can skip calling the parent constructor and still extend DefaultPluginManager. Here's a quick re-roll to does that and removes 2 unneeded classes.

Something that seems to be missing from the committed YAML discovery is required key checking and typecasting (e.g. make sure a parameter is an array)?

damiankloip’s picture

Since when does discovery do that?

Possibly a required keys thing could work. I'm not sure about any type casting though. Why would we even want to do that? (I don't see the point of us doing it in the other issue before). If people have not used an array when they should have, they will get an error soon enough anyway. We don't want to do too much babysitting IMO. If something gets cast into an array, that could make DX worse, i.e. harder for a Dev to debug, or thinking that not using an array is ok.

pwolanin’s picture

damiankloip - I'm looking at the existing code for local actions disscovery via YAML

damiankloip’s picture

That's not a plugin discovery class though, that is just custom code to that derivative class.

neclimdul’s picture

re: skipping the constructor, I don't really like that. I don't know what the concern here is. Is there a problem, bug or concern?

neclimdul’s picture

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -24,13 +24,14 @@
-class LocalTaskManager extends PluginManagerBase {
+class LocalTaskManager extends DefaultPluginManager {

I didn't want to have this discussion quite yet but since it's come up several times on IRC as well and it is a change I'm making in the patch so I should have done it earlier. This isn't pointed at anyone I just want to lay it all out here so everyone sees it.

TL;DNR We're taking unnecessary technical debt.

First, conceptually extending the default plugin manager doesn't make sense to me. Really I think its weird that we ever do extend it because it sort of breaks our semantics(base vs default) but that is a different argument. In this case we are not using the default methods for plugin management. We have different methods for discovery addressed in this issue and different methods for caching which affects all the rest of the DefaultPluginManager code minus maybe the cache clearing method. The alterInfo function pwolanin added it back for is so trivial I even wonder what the point is.

Now the academic argument. I think we're taking on technical debt that doesn't serve us well. The DefaultPluginManager is built to be the one true core plugin implementation. The core idea is that we want the implementations extending it to be tightly coupled as a way of being able to maintain them in a single location because other managers are basically only configuring it for each implementation.

The problem is the concerns of this plugin system are fundamentally different. We've got both a different discovery system, and different cache logic and probably additional concerns. We should remove the training wheels and really write the bespoke manager implementation we need and maintain exactly that not a monkey patch of another class we're not really using.

neclimdul’s picture

Updated patch with some discussion from IRC.
Summary
1) Because every task isn't making a class, we're really making a default not a base implementation. So instead of removing the default remove the base and move the implementation.
2) Not actually from IRC but what I'd prefer to see happen based on my comment in #39. For now just store the moduleHandler and handle altering as part of cache follow up or as its own follow up.

Also, because it won't be clear from the interdiff, I updated the todo from the base implementation when I moved it.

I was also lazy worked on my local branch and manually applied pwolanin's changes since I was changing some of them. Because of that the interdiff is off of #19/#24 and you'll see a lot of the same changes. Sorry.

Status: Needs review » Needs work

The last submitted patch, drupal8.menu-system.2050919-40.patch, failed testing.

neclimdul’s picture

Failed copying use statements...

neclimdul’s picture

Status: Needs work » Needs review
dawehner’s picture

FileSize
17.16 KB

After quite some talking with chx and pwolanin we just want to use the decorator approach and reduce the amount of changes of the local task manager as much as possible.

An interdiff to #42 does not make that much sense as I started from 0. Well I copied quite some changes for the tests, so thank you for that!

pwolanin’s picture

Status: Needs review » Needs work

I think if we are backing off of using primary YAML discovery we should align this with local actions and generate derivatives based on the YAML, instead of adding this extra decorator on top?

In any case, the 2 implementations should be in alignment.

dawehner’s picture

Status: Needs work » Needs review

Let's wait until #2076681: Add a YAML discovery decorator with tests is in.

neclimdul’s picture

Status: Needs review » Needs work

Seriously, no. You're making things more complicated at some false sense of "bigger" changes. Don't.

dawehner’s picture

Seriously, no. You're making things more complicated at some false sense of "bigger" changes. Don't.

So you don't like that we build up an alternative chain of constructors?

Great, now we finally found a solution of which all beside one person agrees with, yeah :)

neclimdul’s picture

Status: Needs work » Needs review

woops.

neclimdul’s picture

Maybe I'm not understanding what you did because there wasn't an interdiff and I don't have time to compare the patches line by line tonight but there is no possible reason I can think of to move yaml to a decorator. that is by the very definition of how plugins work the job of the discover object.

neclimdul’s picture

It seems the real concern here is that people want to continue inheriting from the DefaultPluginManager. I do still disagree but we don't need a decorator for that. I'm out of time for the evening but I'll re-roll 2 patches so we can see what's going on. Also, since this is a concern we want to discuss lets move the cache issue forward so we can include the cache complexities here instead of the @todo.

pwolanin’s picture

@neclimdul - no, I think the real concern is that custom implementations should use annotation rather than yaml.

neclimdul’s picture

Ok, so if we want to have definitions live in 2 places, have 2 directory scans and have an extra decorator to support it I'm not going to stop this. I do think it is the wrong decision though.

pwolanin’s picture

So... I know it seems the direction is moving around but I think we are all struggling to see if we are making the "right" decision in terms of setting a model for other parts of core and contrib. At least *I'm* struggling.

The advantage of using annotations for custom implementations is that the meta-data is directly on the class so e.g. it could be duplicated as a unit. Is this worth the potential confusing of there being 2 ways devs might need to approach the same general task?

Using some version of yaml discovery (instead of derivates) for the plugins using the default class means we have nicer IDs and an easier DX in the 80%+ case. But I think we'll need to do the kind of checking like #11 that Tim didn't like. Does having the annotation class means that default values are filled in? I'm not sure I see how that would work. At the least, they not that they are validated, so I think we need something like this list from #11 regardless.

Based on discussion w/ msonnabaum, the right place to put the validation might be in a factory class?

pwolanin’s picture

Maybe we can setup a hangout or some other way to have a real-time discussion?

I'm going to re-roll a patch that looks more like #34 and #42 (back to pure YAML discovery without a decorator) and add a custom factory class which is where the validation will live.

I think it's very helpful to keep the hook trivially in place by using the DefaultPluginManager since I'm guessing a module like Views would prefer to do that instead of having to set up a derivatives class via referencing it in YAML?

I think that remains an advantage of keeping annotation discovery too - that a derivative class can be located where needed and be totally self-contained?

pwolanin’s picture

Issue tags: +Needs reroll

turn out it needs a serious re-roll anyhow for changes including #2032303: Cache the result of local tasks

pwolanin’s picture

Issue tags: -Needs reroll
FileSize
29.4 KB

Here's a new one that adds a LocalTaskFactory, and translates the title (which was missing before). The title translation is actually a good reason perhaps to stick with one discovery - in the annotated version it was already translated, so we'd have a mix of translated and untranslated strings.

Note - I created the patch with git diff --no-renames, so it can be applied using patch as well.

pwolanin’s picture

Here's the same patch with the rename.

pwolanin’s picture

So, further discussion in IRC with msonnabaum and others, suggests that setting defaults would ideally be in the YAML discovery since the annotation-based discovery already sets defaults and it would be good for them to work in parallel.

However, the rest of core already uses the $defaults array from the PluginManagerBase. So... let's go back to that and then we don't need the custom factory right now.

So, this ends up back very close to neclimdul's patch in #42 except that it continues to extend DefaultPluginManager so we don't have to re-implement the alter hook and caching, validates that a route name is present, and translates the plugin title inside the getTitle() method.

This one has the rename in the diff, for easier comparison to neclimdul's

Status: Needs review » Needs work

The last submitted patch, local_task-2050919-59.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -87,6 +106,17 @@ public function __construct(ControllerResolverInterface $controller_resolver, Re
    +  public function processDefinition(&$definition, $plugin_id) {
    +    $definition += $this->defaults;
    

    Why not just call parent?
    This isn't a problem now, but if it becomes a multidimensional array, this will stop working.

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -85,16 +85,17 @@ public function getRouteName() {
    +    return $this->t->translate($this->pluginDefinition['title']);
    

    $this->t->translate will not work, we need $this->t(). That was decided already, see #2019679: Support for Drupal 8 $this->t()

  3. +++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
    @@ -8,19 +8,16 @@
    +    return 'Dynamic title';
    

    Wouldn't this need $this->t()?

pwolanin’s picture

@tim.plunkett I think this->t() vs this->t->translate() doesn't matter since I think the original test will need to be parsed out of the YAML?

It would only matter in the case of fixed strings in the class, but I guess we should provide a $this->t() for that use case?

Patch adds that method and fixes the test fail, and addresses the other comments.

changes:

-    $definition += $this->defaults;
+    parent::processDefinition($definition, $plugin_id);
tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -23,7 +23,7 @@ class LocalTaskDefault extends PluginBase implements LocalTaskInterface, Contain
        * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface
        */
    -  protected $t;
    +  protected $stringTranslation;
    
    @@ -55,7 +55,7 @@ class LocalTaskDefault extends PluginBase implements LocalTaskInterface, Contain
       public function __construct(array $configuration, $plugin_id, array $plugin_definition, TranslatorInterface $string_translation, UrlGeneratorInterface $generator) {
         // This is available for subclasses that need to translate a dynamic title.
    -    $this->t = $string_translation;
    +    $this->stringTranslation = $string_translation;
    

    This is the wrong interface, it should be Drupal\Core\StringTranslation\TranslationInterface;

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -74,6 +74,29 @@ public static function create(ContainerInterface $container, array $configuratio
       /**
    +   * Translates a string to the current language or to a given language using
    +   * the string translation service.
    +   *
    +   * @param string $string
    +   *   A string containing the English string to translate.
    +   * @param array $args
    +   *   An associative array of replacements to make after translation. Based
    +   *   on the first character of the key, the value is escaped and/or themed.
    +   *   See \Drupal\Core\Utility\String::format() for details.
    +   * @param array $options
    +   *   An associative array of additional options, with the following elements:
    +   *   - 'langcode': The language code to translate to a language other than
    +   *      what is used to display the page.
    +   *   - 'context': The context the source string belongs to.
    +   *
    +   * @return string
    +   *   The translated string.
    +   */
    +  protected function t($string, array $args = array(), array $options = array()) {
    +    return $this->stringTranslation->translate($string, $args, $options);
    +  }
    

    Per #2018411: Figure out a nice DX when working with injected translation, this should be

      /**
       * Translates a string to the current language or to a given language.
       *
       * See the t() documentation for details.
       */
      protected function t($string, array $args = array(), array $options = array()) {
        return $this->stringTranslation->translate($string, $args, $options);
      }
pwolanin’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think this is fine from my perspective!

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1003 bytes
23.64 KB
+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -72,8 +94,10 @@ class LocalTaskManager extends DefaultPluginManager {
+    $this->discovery = new YamlDiscovery('local_tasks', array_values($module_handler->getModuleDirectories()));

Sorry to change status, but I'm not sure why array_values() is being used here?! It doesn't really make sense. You're definition will look like:

Array
(
    [title] => Settings
    [tab_root_id] => views_ui_list_tab
    [tab_parent_id] => 
    [weight] => 0
    [options] => Array
        (
        )

    [class] => Drupal\Core\Menu\LocalTaskDefault
    [route_name] => views_ui.settings.basic
    [provider] => 38
    [id] => views_ui_settings_tab
)

The local task definitions are not getting tested, only in unit tests that mock everything, so this wouldn't get picked up. This could be a follow up? I don't mind.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.45 KB
24.42 KB

Added a test to ensure the provider key.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

@damiankloip - thanks for catching that bug. I think with the added test coverage this is solid.

damiankloip’s picture

Was there some other reason array_values() was used in the first place (to fix anything else)? Seems like we have most things covered now though.

pwolanin’s picture

It was in the 1st patch by neclimdul in #8, so I think it was just a mistake or oversight that no one noticed and keep re-using.

damiankloip’s picture

OK, great. Looks good then!

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

Found two things that are missing. patch in a few.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
25.73 KB
2.7 KB

Peter caught me and corrected the only big concern I had. Just some doc and use cleanups. Added back the defaults that went missing so they are documented.

neclimdul’s picture

"can you improve the doc name for route? i.e. that it's required and is used to generate the link for the tab?" - pwolanin

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

thanks for the extra docs

pwolanin’s picture

dawehner’s picture

Priority: Normal » Critical

Given that this blocks #2076283: Allow local task plugins on paths with a dynamic value (like a node) and #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters, which itself blocks every advanced conversion like node/{node}/edit and field UI this feels critical, sorry.

pwolanin’s picture

catch’s picture

Title: Replace local task plugin discovery with YamlDiscovery » Change notice: Replace local task plugin discovery with YamlDiscovery
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Looks great. Committed/pushed to 8.x.

Will need a change notice.

pwolanin’s picture

Title: Change notice: Replace local task plugin discovery with YamlDiscovery » Replace local task plugin discovery with YamlDiscovery
Status: Active » Fixed
Issue tags: -Needs change record

Added a short new change notice at : https://drupal.org/node/2085285

and updated the original one at: https://drupal.org/node/2044515

Status: Fixed » Closed (fixed)

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

plach’s picture

plach’s picture

Issue summary: View changes

Update to clarify new direction of issue.