Convert this page callback to a new-style Controller

Part of #1971384: [META] Convert page callbacks to controllers

Files: 
CommentFileSizeAuthor
#56 interdiff.txt897 bytesdawehner
#56 tracker-1972990-56.patch12.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]
#55 tracker_page_controller-1972990-55.patch13.83 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]
#55 1972990-53-55.increment.txt636 bytespwolanin
#53 tracker_page_controller-1972990-53.patch13.83 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]
#53 1972990-51-53.increment.txt2.22 KBpwolanin
#51 page-callback-2091691-41.patch14.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,013 pass(es).
[ View ]
#51 interdiff.txt5.75 KBtim.plunkett
#48 tracker_page_controller-1972990-48.patch10.63 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,144 pass(es).
[ View ]
#48 1972990-46-48.increment.txt2.38 KBpwolanin
#46 tracker_page_controller-1972990-46.patch10.56 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]
#46 1972990-44-46.increment.txt4.32 KBpwolanin
#44 tracker_page_controller-1972990-44.patch10.22 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,032 pass(es).
[ View ]
#44 1972990-42-44.increment.txt6.05 KBpwolanin
#42 tracker_page_controller-1972990-42.patch9.96 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,178 pass(es).
[ View ]
#42 1972990-40-42.increment.txt1.53 KBpwolanin
#40 tracker_page_controller-1972990-40.patch8.76 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,831 pass(es).
[ View ]
#40 1972990-38-40.increment.txt656 bytespwolanin
#38 tracker_page_controller-1972990-38.patch8.66 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,058 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
#31 tracker_page_controller-1972990-31.patch20.94 KBAjitS
FAILED: [[SimpleTest]]: [MySQL] 57,848 pass(es), 38 fail(s), and 9 exception(s).
[ View ]
#24 tracker_page_controller-1972990-24.patch20.43 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 55,782 pass(es), 26 fail(s), and 17 exception(s).
[ View ]
#22 tracker_page_controller-1972990-21.patch20.3 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 55,646 pass(es), 34 fail(s), and 11 exception(s).
[ View ]
#14 tracker_page_controller-1972990-14.patch20.07 KBDean Reilly
FAILED: [[SimpleTest]]: [MySQL] 55,339 pass(es), 2 fail(s), and 17 exception(s).
[ View ]
#14 interdiff.txt842 bytesDean Reilly
#13 tracker_page_controller-1972990-13.patch19.99 KBDean Reilly
FAILED: [[SimpleTest]]: [MySQL] 55,887 pass(es), 59 fail(s), and 60 exception(s).
[ View ]
#13 interdiff.txt7.82 KBDean Reilly
#11 drupal-1972990-11.patch14.99 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,305 pass(es), 7 fail(s), and 3 exception(s).
[ View ]
#11 interdiff.txt1.53 KBdawehner
#9 drupal-1972990-9.patch8.73 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,394 pass(es).
[ View ]
#9 interdiff.txt1.44 KBdawehner
#8 tracker_page_controller-1972990-8.patch8.67 KBdokumori
FAILED: [[SimpleTest]]: [MySQL] 55,142 pass(es), 27 fail(s), and 9 exception(s).
[ View ]
#5 tracker_page_controller-1972990-5.patch8.7 KBDean Reilly
FAILED: [[SimpleTest]]: [MySQL] 55,358 pass(es), 26 fail(s), and 43 exception(s).
[ View ]
#5 interdiff.txt3.65 KBDean Reilly
#1 tracker_page_controller-1972990-1.patch7.75 KBJberges
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]

Comments

Issue summary:View changes

Added parent issue

Issue summary:View changes

Added instructions link in correct format

StatusFileSize
new7.75 KB
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]

First patch.

tracker_page() cannot be removed from tracker.pages.inc because is used in other callbacks. I will be working on those other callbacks, but first I want to check if this patch is OK.

Status:Active» Needs review

Just a minor remark before your submit the next patch: TrackerController.php contains extra space in the end. Otherwise, patch looks great.

Status:Needs review» Needs work

thanks for picking this up:)

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,160 @@
+   * Injects TrackManager Service.

No need to have a comment there, just {inheritdoc}

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,160 @@
+    return new static();
...
+  public function __construct() {
+  }
...
+      $query = db_select('tracker_user', 't')
...
+      $query = db_select('tracker_node', 't', array('target' => 'slave'))
...
+      $nodes = entity_load_multiple('node', $nids);

lets get rid of db_select and entity_load_multiple
To do so we need to inject the database and plugin manager service and store them to $this->database and $this->entityManager

you can check the patch here #1963410-19: Convert aggregator_form_opml to a FormInterface implementation and routing definition to get an idea from a similara conversion:)

Status:Needs work» Needs review
Issue tags:+LONDON_2013_APRIL
StatusFileSize
new3.65 KB
new8.7 KB
FAILED: [[SimpleTest]]: [MySQL] 55,358 pass(es), 26 fail(s), and 43 exception(s).
[ View ]

Addressing the issues raised in #4.

Status:Needs review» Needs work

The last submitted patch, tracker_page_controller-1972990-5.patch, failed testing.

Thanks for converting the tracker page, but you could also remove the old code :)

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,191 @@
+* Controller routines for book routes.

... That's for tracker :)

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,191 @@
+   * @param \Drupal\Core\Database\Connection; $database

There shouldn't be a ";" at the end.

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,191 @@
+   *   The database object.

Let's name is database connection object.

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,191 @@
+          'author' => array('data' => theme('username', array('account' => $node))),

Let's use a follow up to use a build array here.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -34,8 +34,7 @@ function tracker_help($path, $arg) {
     'file' => 'tracker.pages.inc',

No need for the file anymore.

StatusFileSize
new8.67 KB
FAILED: [[SimpleTest]]: [MySQL] 55,142 pass(es), 27 fail(s), and 9 exception(s).
[ View ]

Made most of the changes suggested by @dawehner as well as some other minor changes. (The fourth suggestion re array has not been implemented.)

This still doesn't pass the tests - some issues in a select query of trackerPage in TrackerController.php

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
new8.73 KB
PASSED: [[SimpleTest]]: [MySQL] 55,394 pass(es).
[ View ]

Fixed the code, so that the test passes now.

Status:Needs review» Needs work

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,193 @@
+<?php
+/**
+* @file
+* Contains \Drupal\tracker\Controller\TrackerController.
+*/

needs new line and docblock some indentation

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,193 @@
+/**
+* Controller routines for tracker routes.
+*/

indentation too

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,193 @@
+?>

should not be there

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -34,10 +34,8 @@ function tracker_help($path, $arg) {
-    'page callback' => 'tracker_page',

is tracker_page() still needed? i dotn see it removed

Status:Needs work» Needs review
StatusFileSize
new1.53 KB
new14.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,305 pass(es), 7 fail(s), and 3 exception(s).
[ View ]

Oh you are totally right.

I fixed also some other small points.

Status:Needs review» Needs work

The last submitted patch, drupal-1972990-11.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.82 KB
new19.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,887 pass(es), 59 fail(s), and 60 exception(s).
[ View ]

The other menu items also use the tracker_page() function. Here's a patch which starts on converting them but still needs some work doing. Specifically:

  • The access controllers use arg(). Seems like this could be improved.
  • /tracker seems to be matching against tracker/%user_uid_optional and causing an error because there's only one path element.
  • Seem to be getting some odd error messages in the log:

    Recoverable fatal error: Argument 2 passed to Drupal\tracker\Controller\TrackerController::__construct() must be an instance of Drupal\tracker\Controller\EntityManager, instance of Drupal\Core\Entity\EntityManager given, called in /var/www/drupal8/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.php on line 38 and defined in Drupal\tracker\Controller\TrackerController->__construct() (line 49 of /var/www/drupal8/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.php).
    Recoverable fatal error: Argument 1 passed to Drupal\tracker\Controller\TrackerController::__construct() must be an instance of Drupal\tracker\Controller\Connection, instance of Drupal\Core\Database\Driver\mysql\Connection given, called in /var/www/drupal8/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.php on line 38 and defined in Drupal\tracker\Controller\TrackerController->__construct() (line 49 of /var/www/drupal8/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.php).

I'm still getting up to speed with the WSCCI stuff so any help around this would be great.

StatusFileSize
new842 bytes
new20.07 KB
FAILED: [[SimpleTest]]: [MySQL] 55,339 pass(es), 2 fail(s), and 17 exception(s).
[ View ]

Ahh, looking at the interdiff, I see what caused point 3. Fixed in this patch.

Status:Needs review» Needs work

The last submitted patch, tracker_page_controller-1972990-14.patch, failed testing.

Issue tags:-LONDON_2013_APRIL

Issue needs a summary from comment #13, and a re-roll.

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/OwnTrackingInformation.phpundefined
@@ -0,0 +1,32 @@
+class OwnTrackingInformation implements AccessCheckInterface {

Maybe someone else has a better idea for the access checker

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/OwnTrackingInformation.phpundefined
@@ -0,0 +1,32 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().
+++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTab.phpundefined
@@ -0,0 +1,32 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

As you need to get on the code anyway: Let's better use {@inheritdoc}

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTab.phpundefined
@@ -0,0 +1,32 @@
+    return ($account = user_load(arg(1), TRUE)) && $account->access('view') && user_access('access content');

It should use an injected entity Manager to load the user.

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,200 @@
+  public function trackerPage($account = NULL, $set_title = FALSE) {

I guess the reason why it doesn't work is that you don't have the same name: account vs. user.

+++ b/core/modules/tracker/tracker.routing.ymlundefined
@@ -0,0 +1,20 @@
+    _content: '\Drupal\tracker\Controller\TrackerController::trackerPage'
...
+    _content: '\Drupal\tracker\Controller\TrackerController::trackerPage'
...
+    _content: '\Drupal\tracker\Controller\TrackerController::trackerPage'

I really like that we can reuse the same controller all the time.

+++ b/core/modules/tracker/tracker.routing.ymlundefined
@@ -0,0 +1,20 @@
\ No newline at end of file

There should be no newline.

Assigned:Unassigned» dbcollies

Assigned:dbcollies» Unassigned

Assigned:Unassigned» pwolanin

I'll try this one

Status:Needs work» Needs review

This makes some improvements, but there is a fatal error on /tracker due to the removal of _to_arg functions. I think this should be considered blocked on #2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu()

StatusFileSize
new20.3 KB
FAILED: [[SimpleTest]]: [MySQL] 55,646 pass(es), 34 fail(s), and 11 exception(s).
[ View ]

Status:Needs review» Needs work

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.phpundefined
@@ -0,0 +1,33 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

The coding standard tells about using {@inheritdoc} instead + in the other files.

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.phpundefined
@@ -0,0 +1,33 @@
+  }
+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,199 @@
+  }

Nitpicking: There is an empty line needed after the pre-last "}"

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.phpundefined
@@ -0,0 +1,33 @@
+ * Contains Drupal\tracker\Access\ViewOwnTrackerAccessCheck.

Contains "\"

+++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerController.phpundefined
@@ -0,0 +1,199 @@
+  /**
+   * The entity manager.
+   *
+   * @var \Drupal\Core\Entity\EntityManager
+   */
+  protected $entityManager;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('database')
+    );
+  }
+
+  /**
+   * Constructs a TrackerController object.
+   *
+   * @param \Drupal\Core\Database\Connection $database
+   *   The database connection object.
+   */
+  public function __construct(Connection $database) {
+    $this->database = $database;

This should get also the actual entity manager injected unless we want magic to happen.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -34,33 +34,23 @@ function tracker_help($path, $arg) {
+  $items['tracker/1'] = array(

Seems to be a temporary fix :)

+++ b/core/modules/tracker/tracker.routing.ymlundefined
@@ -0,0 +1,27 @@
+tracker_users_recent_content:
...
+tracker_user_tab:

I guess we should have the same cardinality (users vs. user)

Status:Needs work» Needs review
StatusFileSize
new20.43 KB
FAILED: [[SimpleTest]]: [MySQL] 55,782 pass(es), 26 fail(s), and 17 exception(s).
[ View ]

tracker_user_tab shoudl be singular I think - the s in the other is indicating possessive 's I think?

Adds back the missing entity controller.

Status:Needs review» Needs work

The last submitted patch, tracker_page_controller-1972990-24.patch, failed testing.

Issue tags:+Needs reroll

This no longer applies, will need a reroll to move things further.

Issue tags:-Needs reroll

Don't we actually want to be these pages to be a view?

@dhwehner - would make sense to be to be a View - however, I'm not sure Views has even been able to handle the kind of "to_arg" functionality in question?

Component:action.module» tracker.module

Okay as far as I understand to_arg is basically what we now call parameter conversion/upcasting so convert an ID to an actual object.

Why would views need this? If something is filtered then it is always filtered by ID. For the page title it just calls user_load and then use the label.

Status:Needs work» Needs review
StatusFileSize
new20.94 KB
FAILED: [[SimpleTest]]: [MySQL] 57,848 pass(es), 38 fail(s), and 9 exception(s).
[ View ]

Patch at #24 re-rolled for testing.
Thank you heddn for mentoring me on this.

Status:Needs review» Needs work

The last submitted patch, tracker_page_controller-1972990-31.patch, failed testing.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Issue tags:+Needs reroll

old patch needs massive reroll.

Don't bother rerolling per #27. If someone wants to work on this, start from scratch writing a view.

Let me try implementing now that we have all the routes in.

working on this today - making a class per route as we discussed earlier in the week.

Status:Needs work» Needs review
StatusFileSize
new8.66 KB
FAILED: [[SimpleTest]]: [MySQL] 59,058 pass(es), 0 fail(s), and 36 exception(s).
[ View ]

Prior patch didn't apply, so here's a new version with quick conversion of the controllers.

Status:Needs review» Needs work

The last submitted patch, tracker_page_controller-1972990-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new656 bytes
new8.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,831 pass(es).
[ View ]

forgot a couple use statements in the local task class.

Issue tags:-Needs reroll

fix tags

StatusFileSize
new1.53 KB
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,178 pass(es).
[ View ]

opps, we should just delete tracker_menu()

  1. +++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    +class UserTrackerAccessCheck implements AccessCheckInterface {
    ...
    +  /**
    +   * Implements AccessCheckInterface::applies().
    +   */
    +  public function applies(Route $route) {
    +    return array_key_exists('_access_tracker_user_tab', $route->getRequirements());
    +  }
    +
    +++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    +class ViewOwnTrackerAccessCheck implements AccessCheckInterface {
    ...
    +  /**
    +   * Implements AccessCheckInterface::applies().
    +   */
    +  public function applies(Route $route) {
    +    return array_key_exists('_access_tracker_own_information', $route->getRequirements());
    +  }

    Let's better implement StaticAccessCheckInterface

  2. +++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    +   * Implements AccessCheckInterface::access().
    +++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    + * Access check for user tracker routes.

    Let's also use @inheritdoc

  3. +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerPage.php
    @@ -0,0 +1,17 @@
    +class TrackerPage extends ControllerBase {
    +  public function getContent() {
    +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerUserRecent.php
    @@ -0,0 +1,18 @@
    +class TrackerUserRecent extends ControllerBase {
    +  public function getContent(UserInterface $user) {
    +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerUserTab.php
    @@ -0,0 +1,24 @@
    +class TrackerUserTab extends ControllerBase {
    +  public function getContent(UserInterface $user) {
    ...
    +  public function getTitle(UserInterface $user) {
    +++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/Menu/UserTrackerTab.php
    @@ -0,0 +1,57 @@
    +class UserTrackerTab extends LocalTaskDefault {

    We could use some documentation here at there.

  4. +++ b/core/modules/tracker/tracker.local_tasks.yml
    @@ -0,0 +1,10 @@
    +tracker_page_tab:
    ...
    +tracker_users_recent_tab:
    ...
    +  class: '\Drupal\tracker\Plugin\Menu\UserTrackerTab'

    Let's use tracker.page_tab for now as we do convert all the other instances anyway.

StatusFileSize
new6.05 KB
new10.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,032 pass(es).
[ View ]

ok, made those changes.

  1. +++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    +    $account = $request->attributes->get('user');
    +    $user = \Drupal::currentUser();
    +++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php
    @@ -0,0 +1,36 @@
    +    $account = $request->attributes->get('user');
    +    $user = \Drupal::currentUser();

    These are backwards, I'm pretty sure we'll be passing in AccountInterface $account eventually, and 'user' is UserInterface... just swap the variable names

  2. +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerPage.php
    @@ -0,0 +1,24 @@
    +  /**
    +   * Controller for tracker.page route.
    +   */

    Weird indent

  3. +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerPage.php
    @@ -0,0 +1,24 @@
    +class TrackerPage extends ControllerBase {
    +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerUserRecent.php
    @@ -0,0 +1,25 @@
    +class TrackerUserRecent extends ControllerBase {
    +++ b/core/modules/tracker/lib/Drupal/tracker/Controller/TrackerUserTab.php
    @@ -0,0 +1,29 @@
    +class TrackerUserTab extends ControllerBase {

    I don't see a reason to not combine these for now... doesn't matter though

StatusFileSize
new4.32 KB
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]

I was trying to start aligning with the class-per-page approach (obviously does not matter much for this path).

Beside from this this is looking pretty good.

+++ b/core/modules/tracker/lib/Drupal/tracker/Access/UserTrackerAccessCheck.php
@@ -0,0 +1,38 @@
+    return $account->hasPermission('access content') && $user->access('view', $account);
+++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php
@@ -0,0 +1,38 @@
+    return $account->isAuthenticated() && ($user->id() == $account->id()) && $account->hasPermission('access content');

Is there a reason we don't add the 'access content' bits to the route definition and choose the 'ALL' access mode?

StatusFileSize
new2.38 KB
new10.63 KB
PASSED: [[SimpleTest]]: [MySQL] 59,144 pass(es).
[ View ]

ok, moved the permission check back to the route. Also, we should check that the $user is not empty, since case we get a Request where it's not present.

Status:Needs review» Reviewed & tested by the community

This looks good so far.

Status:Reviewed & tested by the community» Needs review

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/Menu/UserTrackerTab.php
@@ -0,0 +1,60 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getRouteParameters(Request $request) {
+    return array('user' => $this->currentUserId);
+  }

Since this is the only functional piece of code here, I'm mystified why we need...

+++ b/core/modules/tracker/lib/Drupal/tracker/Plugin/Menu/UserTrackerTab.php
@@ -0,0 +1,60 @@
+  /**
+   * {@inheritdoc}
+   *
+   * @param int $current_user_id
+   *   Ths User ID for the current user.
+   */
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, TranslationInterface $string_translation, RouteProviderInterface $route_provider, $current_user_id) {
+    $this->currentUserId = $current_user_id;
+    parent::__construct($configuration, $plugin_id, $plugin_definition, $string_translation, $route_provider);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
+    return new static(
+      $configuration,
+      $plugin_id,
+      $plugin_definition,
+      $container->get('string_translation'),
+      $container->get('router.route_provider'),
+      $container->get('current_user')->id()
+    );
+  }

...all of these lines of code to do it.

Do we need a LocalTaskDefaultBase? create() should basically never appear in "user-space" code unless they're doing something very out of the ordinary.

Priority:Normal» Critical
StatusFileSize
new5.75 KB
new14.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,013 pass(es).
[ View ]

We have a LocalTaskDefault. Unfortunately when you override the parent's create(), you have to pass all of the same stuff through.

The only added parts here are $this->currentUserId = $current_user_id; and $container->get('current_user')->id()

But, we can simplify the need for that by making the parent class use a local getter for the route provider (a la FormBase/ControllerBase). This is even better since the main reason to override this class is to take over getRouteParameters, which is the only method that uses the route provider.

The $configuration, $plugin_id, $plugin_definition stuff is not avoidable right now.

That looks like a move in the right diretion, but we discussed in IRC adding a currentUser() method to the default local task and a @todo to add it to the base plugin class. That way we wouldn't need any constructor injection (which webchick apparently hates).

Oh course, I'm fine this way too.

StatusFileSize
new2.22 KB
new13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]

Like this makes it much simpler.

StatusFileSize
new636 bytes
new13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]

Silly me - that method doesn't need to be public

StatusFileSize
new12.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]
new897 bytes

There is no reason for the additional access checker, as this is just entity access.

I don't think the interdiff is complete.

Oh well, the interdiff is missing the removed file.

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Fixed

Other than the fact that this patch sorely identifies how badly we need to clean up the DX around access checkers, this looks good to me.

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Removing link