Replace admin/people with a View

This is a duplicate/cross-over of #1823608: Admin views in core but I think it makes sense to break this down anyway. This issue is a good place to start too, as I think it may be the easiest one to get in.

Rerolled after recent changes to user.admin.inc and changed a couple of things (Hopefully they are right! :p)

• Changed permission of the view to 'administer users' as per page permissions before
• Changed default sort of table to descending
• Added reset button
• Turned on AJAX
• Re arranged filters slightly
• Changed items per page to 50

This is part of #1864980: [meta] Figure out how to integrate Views into core.

Just some small points.

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateBundle.phpundefined
--- a/core/modules/field/lib/Drupal/field/Tests/Views/ApiDataTest.php
+++ b/core/modules/field/lib/Drupal/field/Tests/Views/ApiDataTest.phpundefined

Seems to be out of scope here, but I like all that changes.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Plugin/views/field/TranslationLink.phpundefined
@@ -77,13 +62,21 @@ function render($values) {
+    if (module_invoke('translation_entity', 'translate_access', $entity)) {

I always tried to figure out what module_invoke gives you instead of directly calling the function.

+++ b/core/modules/translation_entity/translation_entity.views.incundefined
@@ -0,0 +1,55 @@
+  if ($module_handler->moduleExists('comment')) {

Is there a reason why we don't iterate over all entities, if yes a comment might be helpful.

+++ b/core/modules/translation_entity/translation_entity.views.incundefined
@@ -0,0 +1,55 @@
+        'click sortable' => FALSE,
...
+        'click sortable' => FALSE,
...
+        'click sortable' => FALSE,
...
+        'click sortable' => FALSE,

Instead we could also override the click_sort() method on the field handler.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Link.phpundefined
@@ -51,6 +51,10 @@ public function buildOptionsForm(&$form, &$form_state) {
+    if (user_access('administer users')) {
+      return TRUE;
+    }
...
     return user_access('access user profiles');

What about return user_access('administer users') || user_access('access user profiles');

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
@@ -0,0 +1,78 @@
+      $operations = module_invoke_all('user_operations', $form, $form_state['values']['action']);

We could use the module handler here.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -0,0 +1,66 @@
+    // @todo Validation errors are only shown on page refresh.

Is that a blocker to get it into core?

+++ b/core/modules/user/user.moduleundefined
@@ -2185,11 +2190,13 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
+  // Retrieve the accounts to be canceled from the temp store

No trailing dot.

+++ b/core/modules/user/user.views.incundefined
@@ -313,6 +313,15 @@ function user_views_data() {
+      'click sortable' => FALSE,

Yeah we could define that on the bulkFormBase

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -0,0 +1,82 @@
+    $this->assertIdentical(count($elements), count($this->container->get('module_handler')->invokeAll('user_operations')), 'All user operations are found.');

Shouldn't we also check for available operations?

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -0,0 +1,82 @@
+    $account = entity_load('user', $account->id());

What about using the storage controller instead of entity_load?

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
@@ -0,0 +1,84 @@
+      // If there are no valid accounts selected, return.
+      if (empty($accounts)) {
+        drupal_set_message(t('No updates to perform.'));
+        return;

When do we not have valid accounts like that? I guess safe is good. It seems we could almost remove that validation, and just go with this? But not a big point.

Otherwise, looks good!

Ahh, makes sense :) So if nothing is selected, We could just show that same message, instead of the validation?

If not, happy with this patch.

Talked with Tim about this. Let's just go with this as is.

Really excited to see this hit RTBC. However, looks like there's still some work..

On the second-to-last page of installer when I'm entering site info, email, etc., I'm getting this:

Debug:
'Missing handler: users user_bulk_form field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users name field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users status field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users_roles rid field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users created field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users access field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users edit_node field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users translation_link field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: views dropbutton field'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users created sort'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users_roles rid filter'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: role_permission permission filter'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users status filter'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: users uid_raw filter'
in views_get_handler() (line 975 of core/modules/views/views.module).
Debug:
'Missing handler: views area area'
in views_get_handler() (line 975 of core/modules/views/views.module).

and one more, on the last page:

Debug:
'Missing handler: users translation_link field'
in views_get_handler() (line 975 of core/modules/views/views.module).

I also get it on the actual admin/people page: https://dl.dropbox.com/u/10160/Screen%20Shot%202013-01-29%20at%207.47.55...

When attempting to add a user to the admin role I get:

Notice: Undefined index: add_role-administrator in Drupal\user\Plugin\views\field\UserBulkForm->views_form_submit() (line 62 of core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.php).

So, sadly, this still needs work. :(

Thanks for the work on this- it's exciting! Did you know about http://drupal.org/project/admin_views ?
Just making sure you're aware its out there, as it seems to work pretty well, although it's certainly not in core as you're looking to do here.

Thanks for linking it, but yeah we are certainly aware of it :)

This also feels really near.

+++ b/core/modules/user/config/views.view.people.ymlundefined
@@ -0,0 +1,253 @@
+      fields:
...
+      filters:
...
+        created:
...
+      empty:

All the plugin_id's are missing.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -0,0 +1,106 @@
+    $account = entity_load('user', $account->id(), TRUE);

I'm wondering whether we should use the entity plugin manager to get the storage controller directly, but yeah not sure what to do for now.

+++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_bulk_form.ymlundefined
@@ -0,0 +1,49 @@
+      fields:

Another round of missing plugin_id's

Added the missing plugin_id's. We should really find a way to fail testing if they're missing...

Well I don't think they should be checked at runtime, as it's just some piece of information to support translations.

We could though write a test which loads all test views and check it.

So the missing translation handler is the only thing that keeps us from progressing here?
i just tested the patch and https://dl.dropbox.com/u/10160/Screen%20Shot%202013-01-29%20at%207.47.55... is still there

Hm. Could we maybe watchdog() those at least? That seems like useful debugging information.

Is there an issue (probably major task?) for un-hacking user_admin_account()?

Well I've been talking for ages about handling missing handlers slightly better, which I have a few ideas around. Maybe I'll fire that issue up again....

Hm. Could we maybe watchdog() those at least? That seems like useful debugging information.

You would think, until your log is full of 5000000 of these messages.

Yeah, it could completely overrun your watchdog table :)

Thanks for working on this!

I performed an in-depth review and also manual user testing with the current patch. Here's what I found:

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDFunctionalTest.php
@@ -253,6 +253,7 @@ function testDelete() {
   function testBlockedUserLogin() {
+    module_enable(array('views'));

@@ -266,10 +267,10 @@ function testBlockedUserLogin() {
+      'action' => 'block',
+      'user_bulk_form[2]' => TRUE,
...
+    $this->drupalPost('admin/people', $edit, t('Apply'));

Instead of enabling Views, we should simply use the user edit form instead of the user listing to block the user in this OpenID test.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+          reset_button_label: Reset

The "Reset" button of the exposed filter form appears unconditionally, even when the view is not filtered in any way.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+          label: 'Bulk update'

When filtering in a way that produces no results, the bulk update options still appear.

They should not appear, since it does not make sense to provide update options for nothing.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+          label: 'Member for'
+          date_format: 'raw time ago'
...
+          label: 'Last access'
+          date_format: 'time ago'

These two columns seem to output date information in the same format, but why is one using a "raw" format (and WTF means "raw"? ;)) and not the other?

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+        edit_node:
+          id: edit_node
+          table: users
+          field: edit_node
+          exclude: '1'
+          text: Edit
+          plugin_id: user_link_edit

1) "node" ?

2) I'm missing a "Cancel account" link.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+      filters:
+        rid:
...
+        permission:
...
+        status:

I'm missing an exposed filter for user name and e-mail.

@damiankloip: Is this not the default view from admin_views?

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+            label: Roles

The "Roles" filter only allows to select a single role, so the label should not be plural.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+            label: Permission

When installing from scratch and going to the People listing, the root user / uid 1 appears.

When filtering by an arbitrary permission, the root user disappears, even though the user has that permission.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+          content: 'No people available.'
+          format: filtered_html

1) This text does not appear when the view is empty.

2) The cause for this is that the specified text format does not exist. In that case, check_markup() returns nothing.

3) We must not use a filter-processed text area in default configuration, unless the module also ships with the specified text format.

For admin_views, a new global plugin was added to Views 7.x-3.x, which allows to output text via filter_xss_admin().

This is what we need here and in all other default views. Was that not contained in the code base that was ported to D8 yet?

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+      path: admin/people/people

Any change we could rename the last path fragment to /list instead of /people while being there?

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+        type: 'default tab'
+        title: List
...
+        weight: '0'

The List tab appears last. ;)

The default tab should have a weight of -10.

Since #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage, you can simply omit the weight definition for default local tasks and they will automatically get a weight of -10.

+++ b/core/modules/user/config/views.view.people.yml
@@ -0,0 +1,274 @@
+id: people

Default configuration provided by modules should be properly namespaced.

This should at least be 'user_people', but even better would be 'user_admin_people'.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.php
@@ -0,0 +1,84 @@
+  public function views_form_validate(&$form, &$form_state) {
+    $selected = array_filter($form_state['values'][$this->options['id']]);
+    if (count($selected) == 0) {
+      form_set_error('', t('No users selected.'));
+    }
+  }

When the view is filtered in a way that produces no results, then this throws a PHP notice and a PHP warning:

Notice: Undefined index: user_bulk_form in Drupal\user\Plugin\views\field\UserBulkForm->views_form_validate() (line 36 of core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.php).

Warning: array_filter() expects parameter 1 to be array, null given in Drupal\user\Plugin\views\field\UserBulkForm->views_form_validate() (line 36 of core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.php).

No users selected.

These PHP warnings/notices should not happen, regardless of whether we'll remove the bulk form options when there are no results or not.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminTest.php
@@ -73,17 +70,17 @@ function testUserAdmin() {
-    $edit['accounts[' . $account->uid . ']'] = TRUE;
...
+    $edit['user_bulk_form[1]'] = TRUE;
...
-    $editunblock['accounts[' . $account->uid . ']'] = TRUE;
...
+    $editunblock['user_bulk_form[1]'] = TRUE;

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php
@@ -87,10 +88,10 @@ function testUserCancelUid1() {
-      'accounts[1]' => TRUE,
...
+      'user_bulk_form[0]' => TRUE,

@@ -407,14 +409,11 @@ function testMassUserCancelByAdmin() {
-    foreach ($users as $uid => $account) {
-      $edit['accounts[' . $uid . ']'] = TRUE;
...
+    for ($i = 0; $i <= 4; $i++) {
+      $edit['user_bulk_form[' . $i . ']'] = TRUE;

Hm.

The bulk form options should really contain the entity IDs as option names like previously.

This will have security consequences otherwise.

I do not really feel comfortable with having a mapping from arbitrary checkbox option integers to entity IDs in a form that performs critical operations. An arbitrary result row ID 15 can mean one thing in this particular second, but ID 15 can mean a completely different thing in the very next second. This means that you potentially perform bulk operations on unintended entities. The mapping should be explicit.

FWIW, the Views 7.x-3.x issue for filter_xss_admin() text area support was #1676608: Add an area handler for unfiltered text

Thanks @sun!

@sun: No, This is a direct port of the current user listing admin page I think. I did create ports of admin_views views for D8, but no one wanted them (sad face).

Also, yes, the unfiltered text plugin is still in D8 (Drupal\views\Plugin\views\area\TextCustom), I have been telling people to use this for any default views, See issues like #1938414: frontpage (/node) view should use unfiltered text for empty area handler.

I will take a look at the rest of this feedback now.

I have made it so that the bulk form options and buttons only appear if we have views results.

1) This text does not appear when the view is empty.

2) The cause for this is that the specified text format does not exist. In that case, check_markup() returns nothing.

3) We must not use a filter-processed text area in default configuration, unless the module also ships with the specified text format.

I will make a follow up to fix the empty area text, as #1934420: Allow area handlers to return a render array broke it, I think. It is not run through render(). I've also changed the handler to use the unfiltered text one, as mentioned in my last comment.

These two columns seem to output date information in the same format, but why is one using a "raw" format (and WTF means "raw"? ;)) and not the other?

raw time ago is just the 'ago' time with the word 'ago' :) Yep, naming could be up for debate.

When filtering by an arbitrary permission, the root user disappears, even though the user has that permission.

User 1 stays in the listing when I try to filter by permissions.

I have also not addressed the last point yet. I'm not sure exactly what you mean.

We will also need to make an issue for the reset button appearing when there are no filters.

I started #1960094: Change notice: Hide reset button when no exposed input has been added to do this.

We could also do with #1845836: Make the actions exposed in the views bulk operation handler configurable being committed for this too, so then the BulkFormBase can incorporate those changes too.

Changed the id of the view. @sun, I'm still not sure exactly what you mean in your last point, sorry!

Oops, forgot the file rename too.

Sorry, the issue changing human_name to label in views got committed, I forgot that.

I bumped #1845836: Make the actions exposed in the views bulk operation handler configurable to major, since it impacts this issue.

+++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_bulk_form.ymlundefined
@@ -0,0 +1,53 @@
+human_name: ''

label :(

+++ b/core/modules/user/user.moduleundefined
@@ -2081,18 +2080,24 @@ function user_user_operations_block($accounts) {
+function user_user_operations_cancel($accounts) {

Even if it's a callback function, shouldn't we document it? We could also type hint the $accounts.

+++ b/core/modules/views/views.moduleundefined
@@ -926,7 +926,6 @@ function views_get_handler($table, $field, $type, $override = NULL) {
   // Finally, use the 'broken' handler.
-  debug(t("Missing handler: @table @field @type", array('@table' => $table, '@field' => $field, '@type' => $type)));

Not sure whether you saw my suggestion in IRC, but I thought of a flag in the views config, which tells you that this handler is "optional".

I bumped #1845836: Make the actions exposed in the views bulk operation handler configurable to major, since it impacts this issue.

Just to be clear, both damian and I think that we should NOT limit the list of actions by default.

Yep, agreed, out of the box we shouldn't limit the available operation options. I think the optional flag is a good idea, we should talk about it some more in a follow up. It's more of a general problem to solve.

I still think that removing the help message is not something we should do, even temporary.

Ugh, the contextual link really sucks. I always hated that in admin generated screens - 99% of the time it will be irrelevant, we should figure out if we can make that nicer. Perhaps we can have a way to hide them, a setting?

The filters are a bit weird, is there any reason they are styled the way they are? There are a couple of issues, 1) The permissions filter doesn't have categories anymore, 2) there are two apply buttons, currently in core we just have the one 3) the alignment of these items is strange - is there a meta issue for this?

Bojhan, is what you are saying about contextual links related to #1877376: Change notice: Improve Views UI text for the contextual links display setting? The option was originally added in views 7.x-3.x for exactly this reason (to help out admin views).

I caught up with Bojhan in IRC; we were in agreement that the contextual links should be hidden.

@Bojhan, Having two buttons comes from the work we did for the bulk form, that this is based on. So I think for the moment this should do what the bulk form does, for consistency? Then if we decide we want to change this, we do this in the base class, so all bulk forms a the same. I think it was @sun originally that said it would be a good idea to have an apply button both top and bottom. I like this too.

I have just opened #1966424: Change notice: Allow Views handlers to be optional as a potential way to ship with optional handlers in default views.

Rerolled to include the admin links change.

Here is a patch with the solution from above referenced issue combined, if people want to test and not have the debug handler message.

Ugh, the contextual link really sucks. I always hated that in admin generated screens - 99% of the time it will be irrelevant, we should figure out if we can make that nicer. Perhaps we can have a way to hide them, a setting?

It seems to be the only direct indicator for users to know, that this is actually a view and they can improve it. This contextual link, will for sure, not be shown for normal users, as they will not have the right to change the view, but doesn't it actually help to have a trigger for the users?

@dawehner I dont think thats a very strong argument, I am sure the kind of user that wants to alter the way a listing like admin/people works will also scroll through the views listing once in his Drupal 8 lifetime. Given that we wil likely advertise that all displays in the admin have also been viewized when we release D8, I don't think this will cause discoverability problems.

Given that we wil likely advertise that all displays in the admin have also been viewized when we release D8, I don't think this will cause discoverability problems.

Well, it seems to be that many places will not be converted: menues, taxonomy etc. Well you are the UX guy, so you are probably right.

Is there a screenshot of what happens w/ contextual links and this view? I can't figure it out from the commentary. It sounds kind of like a bug, but I'm not sure?

I agree with bojhan now. Changing admin/people is not often done.

@webchick, its just do we want contextual links for this view or do we not. Not is the winner. This is not a bug.

Yeah, I tend to agree that offering contextual links on admin views is overkill. If you're already on the admin backend, you can find your way to the Views UI if you need to do tweaks.

Nice :)

Screenshots for the contextual link thing would still be really useful, so that the discussion isn't opaque to people who didn't participate in an IRC discussion. :)

#66: 1851086-66.patch queued for re-testing.

So, LOL. The last dozen comments are about this:

I went to heroic lengths to get this screenshot. May future generations not suffer through the same.

I've tried to convert user_cancel*confirm forms in #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route and found that better to properly fix their ugly code to limit the scope of this issue to views.
also this issue postponed clean-up in #1938884: Replace the fallback user listing with a list controller
so both added as dependent to summary

Tested on a fresh install, works as expected. I don't think we need the details elements arounds filters and ops as we've had before, but I think it'd be nice to keep some separation there between the filters and operations. Also, aligned filters looked much nicer. Will filters styling and filters/VBO separation be follow-up tasks for core themes? If so, +1 for RTBC.

Thanks for the clarifications and fixes.

We should get the majority of improvements from admin_views into this default view though. Unlike the stone-age core listing that we're replacing, the users view in admin_views was continuously improved through actual user feedback over multiple years.

Yay! :) Unfortunately there's a few issues. :(

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.phpundefined
   @@ -51,41 +51,48 @@ public function views_form(&$form, &$form_state) {
   +    // Only add the bulk form options and buttons if there are results.
   +    if (!empty($this->view->result)) {
   

   Does this fix the array hooray bug? Edit: No, I guess that's #1960014: Empty area handlers need to be rendered for tables.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.phpundefined
   @@ -51,41 +51,48 @@ public function views_form(&$form, &$form_state) {
   +          // We are not able to determine a main "title" for each row, so we can
   +          // only output a generic label.
   +          '#title' => t('Update this item'),
   

   Mmmmmmh this looks like an accessibility violation to me.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.phpundefined
   @@ -51,41 +51,48 @@ public function views_form(&$form, &$form_state) {
   +        '#weight' => -100,
   

   Did we pick -100 at random? :)

4. +++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
   @@ -0,0 +1,81 @@
   +/**
   + * Defines a user operations bulk form element.
   + *
   + * @PluginID("user_bulk_form")
   + */
   +class UserBulkForm extends BulkFormBase {
   

   <3 the new annotation.

5. +++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
   @@ -0,0 +1,81 @@
   +      // If there are no valid accounts selected, return.
   +      if (empty($accounts)) {
   +        drupal_set_message(t('No updates to perform.'));
   +        return;
   +      }
   

   Shouldn't this be a yellow warning dsm() rather than a green info one? How do we get to this condition through the validation?

6. +++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
   @@ -0,0 +1,81 @@
   +        else {
   +          drupal_set_message(t('The update has been performed.'));
   +        }
   

   Should we provide a more specific message than this, like "X accounts were blocked"?

7. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
   @@ -66,6 +66,7 @@ function testUserCancelWithoutPermission() {
      function testUserCancelUid1() {
   +    module_enable(array('views'));
   
   @@ -390,6 +391,7 @@ function testUserWithoutEmailCancelByAdmin() {
      function testMassUserCancelByAdmin() {
   +    module_enable(array('views'));
   
   +++ b/core/modules/user/user.admin.incundefined
   @@ -6,283 +6,11 @@
   -function user_admin($callback_arg = '') {
   

   Are we still retaining the legacy form as planned? If so, does this mean it no longer has test coverage?

   Edit: Or we aren't retaining it anymore?

   Edit: I just tested the page manually and the admin people page has no listing of users in Minimal, unlike what we discussed in December. (See attached screenshots below this list.) That's a pretty big deal, and not what Dries signed off on, so we need to discuss this further.

8. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
   @@ -87,10 +88,10 @@ function testUserCancelUid1() {
   +      'user_bulk_form[0]' => TRUE,
   

   What's element 0?

9. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
   @@ -407,14 +409,11 @@ function testMassUserCancelByAdmin() {
        // Cancel user accounts, including own one.
   

   Is this comment still true?

10. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
    @@ -407,14 +409,11 @@ function testMassUserCancelByAdmin() {
    +    $edit['action'] = 'cancel';
    +    for ($i = 0; $i <= 4; $i++) {
    +      $edit['user_bulk_form[' . $i . ']'] = TRUE;
    

    Hm, this seems questionable. We're hardcoding integers here for the UIDs? Or the view just outputs serial labels for the bulk action field and then interprets which is which on submission?

11. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateTest.phpundefined
    @@ -14,6 +14,11 @@
     class UserCreateTest extends WebTestBase {
     
    +  /**
    +   * Modules to enable.
    +   */
    +  public static $modules = array('views');
    

    Does this test really need the admin view? Wouldn't it be better to decouple it from Views?

12. +++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
    @@ -0,0 +1,106 @@
    +    $this->assertIdentical(count($elements), count($this->container->get('module_handler')->invokeAll('user_operations')), 'All user operations are found.');
    

    I believe we can use \Drupal here; TestBase::prepareEnvironment() sets \Drupal to use the test container.

13. +++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
    @@ -0,0 +1,106 @@
    +    $this->drupalPost(NULL, $edit, t('Apply'));
    +    // @todo Validation errors are only shown on page refresh.
    +    $this->drupalGet('test-user-bulk-form');
    +    $this->assertText(t('No users selected.'));
    

    This sounds problematic; has it been resolved yet?

14. +++ b/core/modules/user/user.admin.incundefined
    @@ -6,283 +6,11 @@
    +function user_admin_create() {
    +  $account = entity_create('user', array());
    +  return entity_get_form($account, 'register');
    

    If we're adding new page callbacks, should we be using D8 routing instead? Or was that omitted here to restrict the scope?

15. +++ b/core/modules/user/user.moduleundefined
    @@ -989,10 +980,18 @@ function user_menu() {
    +  $items['admin/people/cancel'] = array(
    +    'title' => 'Add user',
    +    'page callback' => 'drupal_get_form',
    +    'page arguments' => array('user_multiple_cancel_confirm'),
    +    'access arguments' => array('administer users'),
    +    'file' => 'user.admin.inc',
    +  );
    

    I'm pretty sure the menu item title here is a copy and paste mistake?

I tested the patch manually with Views disabled, and here's what happens.

Tagging for accessibility review, since I'm not sure "Update this item" is going to be a sufficient label for the checkbox, and we should probably do full accessibility testing of the form anyway. Thanks guys!

Regarding xjm's comment above, I'd like to add to her awesome review by adding;

2. Yes it is an accessibility violation. I'd like to see it be much more specific. I appreciate that the patch didn't actually change this code, but surely there is a way for us to update that to be better?
5 & 6. Both of these should be more specific. What updates? How many? Why not?

I went over this patch as well as #1864980: [meta] Figure out how to integrate Views into core, #1946526: Clean up Minimal profile modules, #1823608: Admin views in core, and the discussion we had in London with @Dries, @webchick, and @effulgentsia. Dries' decision was:

1. In Standard (at least), our admin/people view is installed by default, since it is a really big win for site builders, module developers, etc. for this to be a view.
2. We need to fix any usability and accessibility regressions before committing this view.
3. Instead of deleting the old page callbacks for the user module, we mark them deprecated.
4. We retain the existing test coverage for the deprecated listing since we will have little indication otherwise if it gets broken.
5. It sounds like Dries is not necessarily opposed to moving the deprecated code to another module (like we discussed in London), but that he would prefer that we have a followup discussion for that instead of considering it for this patch. (Edit: Note that this point is me interpreting what he said, as opposed to something he said explicitly.)

There are two disadvantages to this approach:

1. Maintenance of the deprecated code. Dries is comfortable with the risk of the deprecated code not being well-maintained so long as we retain the test coverage as well.
2. We need to convert these callbacks to the routing system then. :( That's the one part I'm really not fond of, but let's file an issue for it as part of #1971384: [META] Convert page callbacks to controllers. It's not actually that hard, and we can crowdsource it.

...We reintroduce the non-Views listing of users, complete with pager and dropbuttons to edit and delete

But, we remove the bulk operations. It is just a listing. Anyone who wants bulk operations can turn this into tableselect and write it themselves, or install Views. ...

So, we are practically wontfixing this issue here(?). That's not what I got from #87.

Yeah, sorry. I though you were talking about the direction of this whole issue - not what needs to happen for the non-views fallback. My bad.

I think better to split the issue on parts:
1) Simplify user-list
2) decouple user_cancel_multiple form form single user and operations
3) merge with "operation api" if any

Talked with Tim at IRC, let's make in and unfreez other issues that stack on that

The goal was generally not to have to touch these old listing pages at all, so they'd just remain a fallback for people without Views and we could decide what to do with them in D9 or whatever. I don't really understand why we monkeyed with hook_user_operations() in this patch, and after talking to Tim on IRC I *still* don't really understand that, other than the answer "because it's already done." ;) Having never been a fan of hook_user/node_operations() in the first place though (to put it lightly), I don't really shed any tears about them becoming more sense-making (in the future though, probably best to discuss these things in their own issues).

Anyway, I think the compromise of keeping the old listing pages around without bulk operations is fairly ok, and a much better situation than the screenshots in #85. But we need to restore the test coverage to the fallback page because, especially if Minimal starts enabling Views module by default, it's highly unlikely we're going to catch regressions in our day-to-day work.

Haven't reviewed the patch in depth to comment on anything else, but needs work for test restoration.

Doing what I said I was going to do. :P

Also, a lot of my feedback from #85 still hasn't been addressed, as far as I can tell.

Also this.

My questions with the feedback in #85 got addressed so I will try to work on that.

Comment #2, #3 adress code which is already in core, so they work fine and maybe should get a general follow up.

#4 That's great, indeed!

#5 I had a look at the test and yes, it does test for this message itself, but this is just based upon a bug in
the sql query plugin of views. It uses !empty() to determinate whether to load the entity of the row. In the test case we try to block the anonymous user, so the loading fails. I think it's fine to remove the message and
rely totally on the validation message.

#6 Adapted that.

#7 was adressed already

#8,9,10:

What's element 0?

It's the first selected row. I guess you would like it better to have the entity ID as selection available.
Let's please do that in another issue as well. This has always been part of the core bulk operations.

#11
Removed that, as the test works with the fallback listing as well.

#12

I believe we can use \Drupal here; TestBase::prepareEnvironment() sets \Drupal to use the test container.

A note for other people: We discussed that and came to the conclusion to use $this->container if it's available.

#13

This sounds problematic; has it been resolved yet?

No, but that's why it's still a @todo. As far as I understand it, that's not a problem of UserBulkForm, but of BulkFormBase. MHH

Regarding #14: Tim tried to reduce the amount of changes in the patch. I'm happy to have another follow up for that. In general it would be better a controller using an entity listing controller.

Regarding "update this item". The generic BulkFormBase don't even know about entities, so we can't provide something more useful. In constrast though, in UserBulkForm we know that there is an entity involved,
so let's describe it: 'Update the user %name'

I added a new test for the user admin listing page. I hope I tested everything.t

PS: I tried to get the empty text to display, but I doubt it's possible to use simpletest when you have no user at all :)

Updated issue summary.

Good catch!

Awesome work @dawehner!

Let's add followup issues for the things listed in #100:

Comment #2, #3 adress code which is already in core, so they work fine and maybe should get a general follow up.

#8,9,10:

What's element 0?
It's the first selected row. I guess you would like it better to have the entity ID as selection available.
Let's please do that in another issue as well. This has always been part of the core bulk operations.

#13

This sounds problematic; has it been resolved yet?

No, but that's why it's still a @todo. As far as I understand it, that's not a problem of UserBulkForm, but of BulkFormBase. MHH

Regarding #14: Tim tried to reduce the amount of changes in the patch. I'm happy to have another follow up for that. In general it would be better a controller using an entity listing controller.

I'll try to re-review this patch later this week.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
@@ -0,0 +1,96 @@
+   * {@inheritdoc}
+   *
+   * Provide a more useful title to improve the accessibility.

this inheritdoc is not gonna work.either use inheritdoc alone or do it old style, with Overrides..

------

More important..when i install locally, i get
'Missing handler: users user_bulk_form field'

In the configure screen after batch...in simplytest.me, even more weird, the first installation i try, batch hangs at the begin, like for ever....

#104: drupal-1851086-104.patch queued for re-testing.

this inheritdoc is not gonna work.either use inheritdoc alone or do it old style, with Overrides..

I do think this is okay: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria... ... so if this is a problem the api module should support that as well.

I will have a look at the error message.

More important..when i install locally, i get
'Missing handler: users user_bulk_form field'

You might need to enable the actions module if you applied the patch to an existing install.

Tested in minimal. It mostly seems to work fine now, but the default tab for the fallback listing is missing:

Other than that, I still need to test Standard, as well as the pager for the Minimal listing.

I'm also seeing the same problem when I try to install Standard on simplytest.me. Trying locally.

I installed Standard locally with the patch already applied and it worked great. I can't reproduce the handler error @ParisLiakos mentions, but I vaguely remember encountering it when I applied the patch after installing and didn't turn on the action module.

Only one small problem:

And, we still need those followup issues filed. Also, please update the summary to describe the current status/approach. Tim did a nice job scoping the admin content issue via its title. ;)

Not updating the summary yet

• Let's add the people's tab back
  
• Fixed the contextual link

The defaults option was not turned off for show_admin_links.

patch -p1 still applied, so here is a rerole.

We also now need to incorporate the improvements from #1932068: People Admin Table Missing Labels.

I don't think we need an adaption:

<div class="form-item form-type-checkbox form-item-user-bulk-form-0">
 <label class="element-invisible" for="edit-user-bulk-form-0">Update the user <em class="placeholder">admin</em></label> <input type="checkbox" id="edit-user-bulk-form-0" name="user_bulk_form[0]" value="1" class="form-checkbox">
</div>

Unassign myself if this maybe blocks someone from reviewing the patch.

#118: drupal-1851086-118.patch queued for re-testing.

Hm, something funky is still going on with the menu items. On a clean install with #118 applied:

Oh, re: #120, that looks great.

Hmm, and there's a weird bug with status messages. When I add a role to an account, I get a message that the account was blocked.

• Injected the module handler into the UserBulkForm
• Added a proper message (compared to before. I'm not sure whether this the proper way but some helpful message is certainly required)

It would be better if possible that the filter "Active" would be renamed to "Status" with the optioncs "Active" and "Blocked"

Adding a role now gives the correct text.

#125: drupal-1851086-125.patch queued for re-testing.

Just checked the patch with simplytest.me and I could block mysqlf user/1 by using the views submit and got excluded from the system.

Maybe there is already an submit handler and this is a bug or some submithandler should be added to the view.

I certainly can reproduce this behavior in current HEAD.

Re: #130 and #131, see #472074: Don't let the all administrator accounts get blocked. Also, I don't think #1998192: Allow the boolean labels of exposed filters to be configurable blocks this issue.

So, based on @dawehner's changes in #125, the bit with the status message is an existing bug with the user actions. We discussed this and it's actually trying to do something richer than HEAD, which just says "The update has been performed." In this case improving the update message is out of scope and can be a followup, and it will be much cleaner if we do #1846172: Replace the actions API. So, I filed #2004202: Improve status messages for user actions.

When I apply the patch from #125 on a clean clone and go to admin/people I get the following error:

Error message

LogicException: Unable to parse the controller name "user_admin". in Drupal\Core\Controller\ControllerResolver->createController() (line 91 of /Applications/MAMP/htdocs/d8-patch-2/core/lib/Drupal/Core/Controller/ControllerResolver.php).
The website has encountered an error. Please try again later.

(I was going to work on #133)

#125: drupal-1851086-125.patch queued for re-testing.

@xjm asked us to do the thing in #133, we couldn't test it because the patch in #125 was broken so this is our best guess. @bshaffer and @YesCT did all this work, I'm just uploading two things.

I was able to roll back 8.x to May 18th, when the patch was originally made, and applied the interdiff in #137. I was able to verify the change in the message was reverted as expected.

The error in #135 is caused by applying the patch after Drupal is already installed. The behavior with the current page is that admin/people returns an access denied, but admin/people/list works.

Note that the failures in #137 (and the bug with admin/people returning access denied) is caused by #1800998: Use route system instead of hook_menu() in Views.

+++ b/core/modules/user/config/views.view.user_admin_people.ymlundefined
@@ -0,0 +1,281 @@
+      path: admin/people/list

Should not this be admin/people?

+++ b/core/modules/user/config/views.view.user_admin_people.ymlundefined
index f1d1f37..0288ce4 100644
--- a/core/modules/user/lib/Drupal/user/Plugin/Action/AddRoleUser.php

--- a/core/modules/user/lib/Drupal/user/Plugin/Action/AddRoleUser.php
+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/AddRoleUser.phpundefined

+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/AddRoleUser.phpundefined
+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/AddRoleUser.phpundefined
@@ -28,12 +28,9 @@ class AddRoleUser extends ChangeUserRoleBase {

@@ -28,12 +28,9 @@ class AddRoleUser extends ChangeUserRoleBase {
   public function execute($account = NULL) {
     $rid = $this->configuration['rid'];
     // Skip adding the role to the user if they already have it.
-    if ($account !== FALSE && !isset($account->roles[$rid])) {
-      $roles = $account->roles + array($rid => $rid);
-      // For efficiency manually save the original account before applying
-      // any changes.
+    if (array_search($rid, $account->getBCEntity()->roles) === FALSE) {
       $account->original = clone $account;
-      $account->roles = $roles;
+      $account->roles[] = $rid;
       $account->save();
     }
...
diff --git a/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php b/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php

diff --git a/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php b/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php
index a2b2616..2c200e6 100644

index a2b2616..2c200e6 100644
--- a/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php

--- a/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.php
+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.phpundefined

+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.phpundefined
+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/RemoveRoleUser.phpundefined
@@ -28,12 +28,10 @@ class RemoveRoleUser extends ChangeUserRoleBase {

@@ -28,12 +28,10 @@ class RemoveRoleUser extends ChangeUserRoleBase {
   public function execute($account = NULL) {
     $rid = $this->configuration['rid'];
     // Skip removing the role from the user if they already don't have it.
-    if ($account !== FALSE && isset($account->roles[$rid])) {
-      $roles = array_diff($account->roles, array($rid => $rid));
-      // For efficiency manually save the original account before applying
-      // any changes.
+    $key = array_search($rid, $account->getBCEntity()->roles);
+    if ($key !== FALSE) {
       $account->original = clone $account;
-      $account->roles = $roles;
+      unset($account->roles[$key]);
       $account->save();
     }

Let's see who wins here: #2015721: Adding a role to a user throws a php error (*hint* ... test coverage :) )

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminListingTest.phpundefined
@@ -0,0 +1,103 @@
+    $account->roles[$rid_1] = $rid_1;
+    $account->roles[$rid_2] = $rid_2;

Roles are just a list of RIDs now.

+++ b/core/modules/user/user.admin.incundefined
@@ -261,25 +89,11 @@ function user_admin_account() {
 /**
- * Submit the user administration update form.
+ * Page callback: Generates user create administration form.
...
+function user_admin_create() {
+  $account = entity_create('user', array());
+  return entity_get_form($account, 'register');

+++ b/core/modules/user/user.moduleundefined
@@ -930,21 +930,12 @@ function user_menu() {
+    'page callback' => 'user_admin_account',

@@ -984,8 +975,9 @@ function user_menu() {
+    'page callback' => 'user_admin_create',

It just feels wrong to still add page callbacks.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -37,11 +37,11 @@ public function testBulkForm() {
-    $this->assertIdentical(count($elements), count($this->container->get('module_handler')->invokeAll('user_operations')), 'All user operations are found.');
+    $this->assertIdentical(count($elements), 5, 'All user operations are found.');

I try to understand this change. Doesn't it make sense to compare these two? The mount of available actions might change with time.

+++ b/core/modules/system/system.installundefined
@@ -1917,14 +1917,19 @@ function system_update_8045() {
+  $config_to_import = array(
+    'views.view.user_admin_people' => 'user',
+    'views.view.frontpage' => 'node',
...
+  if ($front_page == 'node') {
...
+    $config_to_import['views.view.frontpage'] = 'node';

This would always import the frontpage view, even the frontpage was not /node.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -35,15 +35,11 @@ public static function getInfo() {
-    $this->assertIdentical(count($elements), 5, 'All user operations are found.');

We could bring this back but just count the amount of actual actions there should be available and assert against that?

+++ b/core/modules/system/system.installundefined
@@ -1917,14 +1917,19 @@ function system_update_8045() {
+  $front_page = config('system.site')->get('page.front') ?: 'node';
+  if ($front_page == 'node') {
     // This imports the node frontpage view.
-    $module = 'node';
-    $config_name = 'views.view.frontpage';
+    $config_to_import['views.view.frontpage'] = 'node';
+  }

So we will only import the view if the frontpage is /node. That seems strange - so if I had changed my front page (alot of sites) I would never even see that view in my listings :(

+++ b/core/modules/user/config/views.view.user_admin_people.ymlundefined
@@ -0,0 +1,281 @@
+      exposed_form:
+        type: basic
+        options:
+          submit_button: Filter
+          reset_button: '1'
+          reset_button_label: Reset
+      pager:
+        type: full
+        options:

I think we should re export the view after #1938654: Export all properties of all views handlers and plugins went in. As we are now missing lots of default options that we want in the export. E.g. for query:

query:
  type: views_query
  options:
    disable_sql_rewrite: '0'
    distinct: '0'
    slave: '0'
    query_comment: ''
    query_tags: {  }

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/UserBulkForm.phpundefined
@@ -0,0 +1,64 @@
+    // Filter the actions to only include those for the 'user' entity type.
+    $this->actions = array_filter($this->actions, function ($action) {
+      return $action->getType() == 'user';
+    });

Don't we only want to filter all actions like this when we are in the UI?

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.phpundefined
@@ -35,15 +35,11 @@ public static function getInfo() {
-    $this->assertIdentical(count($elements), 5, 'All user operations are found.');

We could bring this back but just count the amount of actual actions there should be available and assert against that?

Don't we only want to filter all actions like this when we are in the UI?

Sorry, totally forget that point.

EDIT: And the first point about the test. I thought we were still overriding getBulkOptions(), but we aren't anymore. It's covered in the base class.

The only updates are to the yaml file, and that is done programmatically - all looks good.

I think this is ready......

Oh, why the permissions aren't grouped like on admin/people/permissions? :(

Okay, will create a new one I created #2018569: Group permissions in the new views handler (BTW thanks again for your USB cable at DrupalCon Munich :D).

+++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.phpundefined
@@ -163,4 +174,11 @@ public function views_form_submit(&$form, &$form_state) {
+  public function click_sortable() {
+    return FALSE;
+  }

This is now clickSortable()... fix during commit...

Committed e5774f5 and pushed to 8.x. Thanks!

I opened this issue as a follow up #2020167: Add a name and email search field to the admin/people view

another followup #2020265: Bring back contextual links for admin views

going to write the API change notification.

(THIS is the issue I was looking for.)

Summary

• People admin list (admin/people) is now a view by default.
• A page callback is used when views is disabled.
• Bulk operations are only available with views.

update tags (normalize to "Needs change notification")

Seems like a good short change notice.

Thanks ibullock! The change notice also needs to specifically document the API functions that were changed/removed.

Setting to needs work as per #172.

Proposed change notification:
https://drupal.org/node/1895160#comment-7783017

Since the change notice is handled in #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations I'm going to go ahead and mark this one fixed.

Change notification added:
https://drupal.org/node/2084727

156: people-1851086-156.patch queued for re-testing.