Convert tracker_page() to a Controller

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

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:)

Addressing the issues raised in #4.

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.

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

Fixed the code, so that the test passes now.

+++ 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

Oh you are totally right.

I fixed also some other small points.

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.

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

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.

I'll try this one

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()

+++ 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)

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.

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

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?

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.

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

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.

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.

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

forgot a couple use statements in the local task class.

fix tags

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.

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

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?

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.

This looks good so far.

+++ 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.

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.

Like this makes it much simpler.

related patch: #2105123: Add a currentUser() method to plugin base classes that need it

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

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.

Thanks!

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!