Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
3.25 KB

Started some basic work.

Gábor Hojtsy’s picture

Status: Active » Needs review

I'm really interested by this because #2044737: Provide local tasks as plugins needs a *very* similar solution and that seems to be required at this point to get into core. So any core changes needed to make this work would be good to get in ASAP. Looks like the core change proposed here is more like code reorganization to achieve loading all routes at once instead of one-by-one, so not required to make this work, so we may be safe there.

Looking at how would we provide the annotations from the derivative though.

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-1.patch, failed testing.

dawehner’s picture

There is an extra issue regarding the multiple-load: #2032311: Load all routes for local tasks via getRoutesByNames()

Gábor Hojtsy’s picture

Looks like it should not be included here then? :)

dawehner’s picture

FileSize
4.47 KB
4.34 KB

Okay made some progress, so you could potentially look somehow how it will work, but to be honest, as always, the views example is way too complex.

Some problems I ran into:

  • Figure out the task parent/task root is not trivial in general, when it comes as route
  • Based on the parent route you need the task plugin (maybe we should change the API here), but we don't have the information yet, because we are in the context of defining the task plugins.
dawehner’s picture

Issue tags: +VDC
FileSize
7.84 KB
6.84 KB

Some work on it, though still not near to be reviewed.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit, +MenuSystemRevamp
FileSize
22.52 KB

Luckily views comes with an existing webtest for local tasks, so it was just a matter of waiting for them to run and write some unit tests in the meantime.

dawehner’s picture

FileSize
23.03 KB
4.11 KB

A couple small nitpicks here and there.

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-9.patch, failed testing.

Berdir’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
@@ -0,0 +1,179 @@
+            if ($parent_task = $this->getTaskFromRoute($name, $local_tasks)) {
+              $local_tasks['views_view:' . $plugin_id]['tab_root_id'] = $parent_task;
+            }

$parent_task is an array here I think, that does not end well.

Also, getting a real mess with local tasks. Somehow view.files.page_1 ends up as a local task of my page (Where I'm trying to add local tasks to a view), which as added benefit need arguments, but that other view is not providing those arguments, so it blows up.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.55 KB
6.13 KB

$parent_task is an array here I think, that does not end well.

Okay, this is a clear sign that the method is named bad. Fixed that.

Also, getting a real mess with local tasks. Somehow view.files.page_1 ends up as a local task of my page (Where I'm trying to add local tasks to a view), which as added benefit need arguments, but that other view is not providing those arguments, so it blows up.

Here is a bit more work which let's the three tabs (content/files/comments appear again). The general problem is that the local task system is 100% incompatible with the old,
so if something is converted all of the other tasks have to be converted as well.

It would be cool if you could just point me to the repo where TMGMT D8 is developed, so I will have a look.

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.85 KB
1.53 KB

This fixes at least the comment tests.

The other one seemed to be a random failure.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -10,6 +10,7 @@
     use Drupal\Core\Access\AccessManager;
     use Drupal\Core\Cache\CacheBackendInterface;
    +use Drupal\Core\Cache\NullBackend;
     use Drupal\Core\Controller\ControllerResolverInterface;
     use Drupal\Core\Extension\ModuleHandlerInterface;
     use Drupal\Core\Language\LanguageManager;
    @@ -116,6 +117,7 @@ public function __construct(ControllerResolverInterface $controller_resolver, Re
    
    @@ -116,6 +117,7 @@ public function __construct(ControllerResolverInterface $controller_resolver, Re
         $this->accessManager = $access_manager;
         $this->alterInfo($module_handler, 'local_tasks');
         $this->setCacheBackend($cache, $language_manager, 'local_task_plugins', array('local_task' => TRUE));
    +    $this->cacheBackend = new NullBackend('local_tasks');
       }
     
    

    Debug left over.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Menu/LocalTask/ViewsLocalTask.php
    @@ -0,0 +1,16 @@
    +/**
    + * Defines a local task for views with configured local tasks.
    + */
    +class ViewsLocalTask extends LocalTaskDefault {
    +}
    

    What do you need a class for if it doesn't do anything? :)

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -294,17 +294,20 @@ public function executeHookMenu($callbacks) {
             case 'tab':
    -          $items[$path]['type'] = MENU_LOCAL_TASK;
    +          $items[$path]['type'] = MENU_CALLBACK;
               break;
             case 'default tab':
    -          $items[$path]['type'] = MENU_DEFAULT_LOCAL_TASK;
    +          $items[$path]['type'] = MENU_CALLBACK;
               break;
    

    Is this also necessary for the contextual links @todo below?

dawehner’s picture

FileSize
2.56 KB
27.84 KB

What do you need a class for if it doesn't do anything? :)

You are totally right. This is kind of a leftover when we did not used yml files for discovery.

Is this also necessary for the contextual links @todo below?

It is a bit hard to untackle the code in the current state. What do you think about moving the logic to the same place as the contextual links at the moment?

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-16.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -10,6 +10,7 @@
    +use Drupal\Core\Cache\NullBackend;
    

    I guess you want this to go too?

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -0,0 +1,64 @@
    +    return new static($configuration, $plugin_id, $plugin_definition,
    +      $container->get('database')
    

    Should they all move onto their own line? this looks weird.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -0,0 +1,64 @@
    +} ¶
    

    :)

  4. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,180 @@
    +    $view_route_names = $this->state->get('views.view_route_names');
    

    Are we sure we will always have this state set? So the route processing will be first.

  5. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,180 @@
    +      /** @var $executable \Drupal\views\ViewExecutable */
    

    //

  6. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,180 @@
    +          $this->derivatives[$plugin_id]['tab_root_id'] = 'views_view:' . $plugin_id;
    

    Where does this marry up to? (ignorance with local tasks)

  7. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,180 @@
    +    $local_task_id = FALSE;
    

    Should return NULL if there is no string?

  8. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,180 @@
    +    return views_get_applicable_views('uses_hook_menu');
    

    We can use the Views:: alternative now.

dawehner’s picture

FileSize
7.78 KB
27.13 KB

Should they all move onto their own line? this looks weird.

Just approved the weirdness on the unapproved comments.

:)

This seems to be a bug in my editor.

Are we sure we will always have this state set? So the route processing will be first.

If we don't rebuild the routes we are fucked anyway, as routes might not exist at all yet, but I don't see that as a problem at all.
Local tasks are just requested when you render the page. Before that you always would have to find out the active route, which requires the route system to be rebuild or active.

Thank you for the review.

//

Well the full idea behind that line is to make it nice for static checkers and IDEs. Technical a single line comment is always ignored, so for example annotations also don't work on them.

Where does this marry up to? (ignorance with local tasks)

All the local tasks with the same tab_root_id appear at the same time. So if you are at node/{node} the local task manager finds the route 'node.view' and finds all local tasks
which are in the same "tab-group" with the current route.

Should return NULL if there is no string?

I don't give a shit about it.

We can use the Views:: alternative now.

Yeah, sadly this does not help with unit tests at all.

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
27.56 KB

Just moved back how executeHookMenu worked when the test passed.

Status: Needs review » Needs work

The last submitted patch, vdc-2032309-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
28.14 KB

Doh!

damiankloip’s picture

  1. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Derivative/ViewsLocalTaskTest.php
    @@ -0,0 +1,339 @@
    +   * Set applicable views result.
    

    Sets

  2. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/Derivative/ViewsLocalTaskTest.php
    @@ -0,0 +1,339 @@
    +  protected function getApplicableMenuViews() {
    

    Needs a docblock

Both small, sorry. Apart from that, all the other points i had before are sorted.

dawehner’s picture

FileSize
832 bytes
28.18 KB

There we go.

damiankloip’s picture

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

FileSize
1.02 KB
27.57 KB

Even it is already perfect, this can be perfecter!

damiankloip’s picture

Nice clean up!

RTBC++

Berdir’s picture

27: localtasks-2032309-27.patch queued for re-testing.

Berdir’s picture

Wondering, can we clean this up further now that the contextual links etc. stuff went in? Might also require a re-roll now?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: localtasks-2032309-27.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
27.52 KB

Wondering, can we clean this up further now that the contextual links etc. stuff went in? Might also require a re-roll now?

Not in this issue, please!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, then let's get this in ASAP :)

amateescu’s picture

FileSize
1.05 KB
27.5 KB

I was working on #2111823: Convert field_ui / Entity local tasks to YAML definitions which has the same getPluginIdFromRoute() method and made some improvements to the doxygen block which I'd like to bring here as well.

Berdir’s picture

34: vdc-2032309-34.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: vdc-2032309-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.55 KB

Reroll.

tim.plunkett’s picture

Awesome tests!

  1. +++ b/core/modules/comment/comment.module
    @@ -210,18 +210,6 @@ function comment_menu() {
    -    'title callback' => 'comment_count_unpublished',
    
    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -0,0 +1,67 @@
    +  public function getTitle() {
    +    $count = $this->database->query('SELECT COUNT(cid) FROM {comment} WHERE status = :status', array(
    +      ':status' => COMMENT_NOT_PUBLISHED,
    +    ))->fetchField();
    +    return $this->t('Unapproved comments (@count)', array('@count' => $count));
    +  }
    

    I think comment_count_unpublished() should be moved to CommentManager or something... But if not, at least delete the old function.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,181 @@
    +  protected function getPluginIdFromRoute($route_name, &$local_tasks) {
    

    Field UI has this method, and content/config_translation have the same thing but called getTaskFromRoute(). Can we move this to a base class? Or at least a @todo.

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -314,17 +314,20 @@ public function executeHookMenu($callbacks) {
    -          $items[$path]['type'] = MENU_LOCAL_TASK;
    +          $items[$path]['type'] = MENU_CALLBACK;
    

    I think I had a dedicated issue for this (I might be wrong) but let's close that if so.

dawehner’s picture

FileSize
33.76 KB
12.62 KB

I think comment_count_unpublished() should be moved to CommentManager or something... But if not, at least delete the old function.

Let's keep comment_count_unpublished() as this is really out of scope of this issue.

Field UI has this method, and content/config_translation have the same thing but called getTaskFromRoute(). Can we move this to a base class? Or at least a @todo.

Let's do it.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Wonderful!

dawehner’s picture

Priority: Normal » Major

No way is that normal.

dawehner’s picture

Issue tags: +beta blocker

aka.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDerivativeBase.php
    @@ -0,0 +1,40 @@
    +    $local_task_id = NULL;
    

    Couldn't this return $plugin_id directly if found instead of assigning to null + break etc. The function will return null if nothing is found anyway.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -0,0 +1,24 @@
    + * Provides a local task that shows the amount of unapproved comments
    

    Missing full stop.

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiLocalTask.php
    @@ -200,29 +200,6 @@ public function alterLocalTasks(&$local_tasks) {
    -   * Finds the local task ID of a route given the route name.
    

    eww. Nice to see all of these go.

  4. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,158 @@
    +      /** @var $executable \Drupal\views\ViewExecutable */
    

    Is this a new coding standard?

  5. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,158 @@
    +   * Alters tab_root_id and tab_parent_id into the views local tasks.
    

    Doesn't hook_local_tasks_alter() run every request? This looks potentially very expensive if so.

  6. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,158 @@
    +        $path = $executable->display_handler->getPath();
    

    Could this be stored with the view somehow instead?

  7. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsLocalTask.php
    @@ -0,0 +1,158 @@
    +    return Views::getApplicableViews('uses_hook_menu');
    

    Why the helper for this?

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
33.69 KB

Couldn't this return $plugin_id directly if found instead of assigning to null + break etc. The function will return null if nothing is found anyway.

Sure, this works too. The patch was just moving a bit of code around, but it is fine, if we can improve it at the same time.

Is this a new coding standard?

There are various places in core where this is done already to get support for static tools as well as IDEs.

Doesn't hook_local_tasks_alter() run every request? This looks potentially very expensive if so.

There are two hooks at the moment:

  • menu_local_tasks_alter: This runs right before providing the local tasks in menu_local_tasks() and so on every request
  • local_task_plugins_alter: This runs once for the list of local task plugins and is fully cached

So this is not a problem, hey!

Could this be stored with the view somehow instead?

I try to figure out what you meant with that. The idea here is to get the configured path from the user, which is stored in the display. There is also $executable->getPath() which returns the same in this case, though also contains runtime overrides, which means that it is not really semantical, what we want to do.

Why the helper for this?

Well, let's allow the code to speak here: This is not a simple functionality behind there.

  public static function getApplicableViews($type) {
    // Get all display plugins which provides the type.
    $display_plugins = static::pluginManager('display')->getDefinitions();
    $ids = array();
    foreach ($display_plugins as $id => $definition) {
      if (!empty($definition[$type])) {
        $ids[$id] = $id;
      }
    }

    $entity_ids = \Drupal::service('entity.query')->get('view')
      ->condition('status', TRUE)
      ->condition("display.*.display_plugin", $ids, 'IN')
      ->execute();

    $result = array();
    foreach (\Drupal::entityManager()->getStorageController('view')->loadMultiple($entity_ids) as $view) {
      // Check each display to see if it meets the criteria and is enabled.
      $executable = $view->getExecutable();
      $executable->initDisplay();
      foreach ($executable->displayHandlers as $id => $handler) {
        if (!empty($handler->definition[$type]) && $handler->isEnabled()) {
          $result[] = array($executable, $id);
        }
      }
    }

    return $result;
  }
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think that adequately addresses catch's concerns.

webchick’s picture

Assigned: Unassigned » catch

Let's find out! :)

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work

There are various places in core where this is done already to get support for static tools as well as IDEs.

Would be good to add it to http://drupal.org/coding-standards.

menu_local_tasks_alter: This runs right before providing the local tasks in menu_local_tasks() and so on every request
local_task_plugins_alter: This runs once for the list of local task plugins and is fully cached
So this is not a problem, hey!

The hook that's implemented is hook_menu_local_tasks_alter(), so I think it is a proble? If it's expensive/unnecessary to run that every request, which it looks like it is.

Well, let's allow the code to speak here: This is not a simple functionality behind there.

I didn't mean getApplicableViews() itself, I meant the one line wrapper around getApplicableViews() that just saves passing the argument to it.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

@dawehner misspoke, the two hooks are hook_local_tasks_alter (for plugins) and hook_menu_local_tasks_alter (for runtime). views_local_tasks_alter is the plugin one. Field UI, Config Translation, and Content Translation use exactly this pattern.

getApplicableMenuViews() is there for mocking, since we can't swap out the Views:: call. This is like any of our wrapper methods for services.

class TestViewsLocalTask extends ViewsLocalTask {

  /**
   * Sets applicable views result.
   */
  public function setApplicableMenuViews($result) {
    $this->result = $result;
  }

  /**
   * {@inheritdoc}
   */
  protected function getApplicableMenuViews() {
    return $this->result;
  }

}
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

andypost’s picture

Status: Fixed » Closed (fixed)

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