Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Les Lim’s picture

Assigned: Unassigned » Les Lim

it's MINE now.

Les Lim’s picture

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

admin/people
admin/people/people
admin/people/create
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;
}
Crell’s picture

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.

Les Lim’s picture

Status: Active » Needs review
FileSize
37.11 KB

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.

Crell’s picture

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.

tim.plunkett’s picture

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

Crell’s picture

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

tim.plunkett’s picture

Status: Needs review » Postponed

Ah. Yeah. Meant to do this.

andypost’s picture

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

tim.plunkett’s picture

Title: Convert user_admin to a new-style Controller » Convert user_admin_account to a new-style Controller
Status: Postponed » Needs review
FileSize
10.74 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
391 bytes
10.78 KB

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

dawehner’s picture

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

dawehner’s picture

FileSize
4.51 KB
11.02 KB

Here is the EQ.

Status: Needs review » Needs work

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

dawehner’s picture

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

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

tim.plunkett’s picture

Status: Needs work » Postponed

Postponed on that issue

xjm’s picture

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.

tim.plunkett’s picture

Title: Convert user_admin_account to a new-style Controller » Replace the fallback user listing with a list controller
Status: Postponed » Needs work
Issue tags: -SprintWeekend2013
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
11.76 KB

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

jibran’s picture

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.

Xano’s picture

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?

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
12.17 KB
2.71 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

Xano’s picture

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

Xano’s picture

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

alexpott’s picture

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

Patch no longer applies.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.03 KB

Rerolled.

jibran’s picture

Issue tags: -Needs reroll

back to RTBC.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

:/

jibran’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

This patch still applies.

Xano’s picture

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

Xano’s picture

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

webchick’s picture

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.