Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686

Meta issue: #1971384: [META] Convert page callbacks to controllers

Files: 
CommentFileSizeAuthor
#30 user-1938884-30.patch12.03 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,087 pass(es).
[ View ]
#24 interdiff.txt2.71 KBXano
#24 drupal_1938884_24.patch12.17 KBXano
PASSED: [[SimpleTest]]: [MySQL] 59,447 pass(es).
[ View ]
#21 user-1938884-21.patch11.76 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,432 pass(es).
[ View ]
#14 drupal-1938884-14.patch11.02 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,377 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#14 interdiff.txt4.51 KBdawehner
#12 user-1938884-12.patch10.78 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 41 fail(s), and 1 exception(s).
[ View ]
#12 interdiff.txt391 bytestim.plunkett
#10 user-1938884-10.patch10.74 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#4 1938884-convert-user_admin-4.patch37.11 KBLes Lim
PASSED: [[SimpleTest]]: [MySQL] 53,029 pass(es).
[ View ]

Comments

Assigned:Unassigned» Les Lim

it's MINE now.

Some reference info: currently, user_admin() is the page callback for the following hook_menu() routes:

admin/people
admin/people/people
admin/people/create

<?php
function user_admin($callback_arg = '') {
 
$op = isset($_POST['op']) ? $_POST['op'] : $callback_arg;
  switch (
$op) {
    case
t('Create new account'):
    case
'create':
     
$account = entity_create('user', array());
     
$build['user_register'] = entity_get_form($account, 'register');
      break;
    default:
      if (!empty(
$_POST['accounts']) && isset($_POST['operation']) && ($_POST['operation'] == 'cancel')) {
       
$build['user_multiple_cancel_confirm'] = drupal_get_form('user_multiple_cancel_confirm');
      }
      else {
       
$build['user_filter_form'] = drupal_get_form('user_filter_form');
       
$build['user_admin_account'] = drupal_get_form('user_admin_account');
      }
  }
  return
$build;
}
?>

Wow, that's ridiculous. :-)

That sounds like that should turn into 3 methods on the same class. I don't know what's up with that $_POST['op'] stuff, though. That doesn't seem right.

Status:Active» Needs review
StatusFileSize
new37.11 KB
PASSED: [[SimpleTest]]: [MySQL] 53,029 pass(es).
[ View ]

I split off the "create user" part, which now uses the already existing "register" method. No problem there.

The other part (the one checking for $_POST['operation'] == 'cancel') looks like a hacky implementation of a multi-step form. I think the reason why it's not a proper multi-step form in the first place is because there are actually two separately rendered forms on the "admin/people" page - one for setting filters, and separate form for selecting users for bulk operations.

I left that old logic in the new-style Controller as-is. I could probably re-implement it as a multi-step form, if it would be worthwhile.

Assigned:Les Lim» tim.plunkett

I'm going to ask Tim to weigh in here, since there looks to be some weirdness happening with forms here.

Thanks for assigning to me. A lot, or all, of that code is changed/removed in #1851086: Replace admin/people with a View.
I'll post more tonight

Ah, should we postpone this issue then, and won't-fix it if that one lands?

Status:Needs review» Postponed

Ah. Yeah. Meant to do this.

Related issue #1946466-9: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route
I think better make account cancellation forms conversion in separate issue

Title:Convert user_admin to a new-style ControllerConvert user_admin_account to a new-style Controller
Status:Postponed» Needs review
StatusFileSize
new10.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

This was all changed with the views conversion, here's what's left.

Status:Needs review» Needs work

The last submitted patch, user-1938884-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new391 bytes
new10.78 KB
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 41 fail(s), and 1 exception(s).
[ View ]

When Views and User register a route with the same fit, it seems to choose by alphabetical order.

+++ b/core/modules/user/lib/Drupal/user/Controller/UserAdmin.phpundefined
@@ -0,0 +1,154 @@
+   * Constructs a new UserAdmin.

This should be rather "Constructs a new UserAdmin object".

+++ b/core/modules/user/lib/Drupal/user/Controller/UserAdmin.phpundefined
@@ -0,0 +1,154 @@
+  public function userList() {

Missing @return

+++ b/core/modules/user/lib/Drupal/user/Controller/UserAdmin.phpundefined
@@ -0,0 +1,154 @@
+    $query = $this->connection->select('users', 'u');
+    $query->condition('u.uid', 0, '<>');
+
+    $count_query = clone $query;
+    $count_query->addExpression('COUNT(u.uid)');
+
+    $query = $query
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+      ->extend('Drupal\Core\Database\Query\TableSortExtender');
+    $query
+      ->fields('u', array('uid'))
+      ->limit(50)
+      ->orderByHeader($header)
+      ->setCountQuery($count_query);
+    $accounts = $this->storageController->load(array_keys($query->execute()->fetchAllAssoc('uid')));

What blocks us to use an EQ?

StatusFileSize
new4.51 KB
new11.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,377 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

Here is the EQ.

Status:Needs review» Needs work

The last submitted patch, drupal-1938884-14.patch, failed testing.

I'm working on #2027115: Allow views to override existing routing items which really is needed to fix this problems.

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

Status:Needs work» Postponed

Postponed on that issue

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.

Title:Convert user_admin_account to a new-style ControllerReplace the fallback user listing with a list controller
Status:Postponed» Needs work
Issue tags:-#SprintWeekend

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new11.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,432 pass(es).
[ View ]

Didn't realize it was assigned to me still :)

Status:Needs review» Reviewed & tested by the community

It is all good RTBC. Just a minor issue.

+++ b/core/modules/user/lib/Drupal/user/Controller/UserListController.php
@@ -0,0 +1,139 @@
+      'username' => array('data' => $this->t('Username'), 'field' => 'name', 'specifier' => 'name'),
+      'status' => array('data' => $this->t('Status'), 'field' => 'status', 'specifier' => 'status', 'class' => array(RESPONSIVE_PRIORITY_LOW)),
+      'roles' => array('data' => $this->t('Roles'), 'class' => array(RESPONSIVE_PRIORITY_LOW)),
+      'member_for' => array('data' => $this->t('Member for'), 'field' => 'created', 'specifier' => 'created', 'sort' => 'desc', 'class' => array(RESPONSIVE_PRIORITY_LOW)),
+      'access' => array('data' => $this->t('Last access'), 'field' => 'access', 'specifier' => 'access', 'class' => array(RESPONSIVE_PRIORITY_LOW)),

Please can we make it mutli line.

Assigned:Unassigned» Xano
Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/Controller/UserListController.php
    @@ -0,0 +1,139 @@
    + * @todo.

    Nuff said :D

  2. +++ b/core/modules/user/lib/Drupal/user/Controller/UserListController.php
    @@ -0,0 +1,139 @@
    +    $build['accounts']['#empty'] = $this->t('No people available.');

    Is this even possible, or did you just add it for good practice?

Assigned:Xano» Unassigned
Status:Needs work» Needs review
StatusFileSize
new12.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,447 pass(es).
[ View ]
new2.71 KB

Status:Needs review» Reviewed & tested by the community

Xano wanted to open a follow up for user_role_names() so this looks fine now.

See #2111341: Introduce EntityManager::options() to build options lists for a generic replacement for user_role_names()

#24: drupal_1938884_24.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new12.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,087 pass(es).
[ View ]

Rerolled.

Issue tags:-Needs reroll

back to RTBC.

Status:Needs review» Reviewed & tested by the community

:/

Issue summary:View changes

Updated issue summary.

This patch still applies.

30: user-1938884-30.patch queued for re-testing.

30: user-1938884-30.patch queued for re-testing.

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs followup

+++ b/core/modules/user/lib/Drupal/user/Controller/UserListController.php
@@ -0,0 +1,166 @@
+use Drupal\Core\Entity\EntityControllerInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Entity\EntityListController;
+use Drupal\Core\Entity\EntityStorageControllerInterface;
+use Drupal\Core\Entity\Query\QueryFactory;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
...
+
+  /**
+   * The entity query factory.
+   *
+   * @var \Drupal\Core\Entity\Query\QueryFactory
+   */
+  protected $queryFactory;
+
+  /**
+   * Constructs a new UserListController object.
+   *
+   * @param string $entity_type
+   *   The type of entity to be listed.
+   * @param array $entity_info
+   *   An array of entity info for the entity type.
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage
+   *   The entity storage controller class.
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler to invoke hooks on.
+   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
+   *   The entity query factory.
+   */
+  public function __construct($entity_type, array $entity_info, EntityStorageControllerInterface $storage, ModuleHandlerInterface $module_handler, QueryFactory $query_factory) {
+    parent::__construct($entity_type, $entity_info, $storage, $module_handler);
+    $this->queryFactory = $query_factory;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
+    return new static(
+      $entity_type,
+      $entity_info,
+      $container->get('entity.manager')->getStorageController($entity_type),
+      $container->get('module_handler'),
+      $container->get('entity.query')
+    );
+  }
+

Not for this issue, but that's a *shocking* amount of boilerplate code to accomplish this. The diffstat between reams and reams of custom code and re-using a nice common base library is almost equal. :( I would've expected EntityListController to make a lot of this transparent to implementers.

Could we get a follow-up to discuss how we might make the DX of EntityListControllers better? (If one does not exist already?)

At any rate though, yay for using said common base libraries instead of custom code!

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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