Problem/Motivation

In order to save code and other reasons the listing of all people will be a view, if views is installed.

Proposed resolution

The idea is to provide a very basic listing (without any bulk operations available) if views is not available. This fallback
will be overridden by a proper view with filtering, bulk operations etc.

#1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route
#1938884: Replace the fallback user listing with a list controller

Also, because of #1851082: Add a base bulk form handler class , this temporarily makes the actions module required.

Files: 
CommentFileSizeAuthor
#156 people-1851086-156.patch59.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch people-1851086-156.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#156 interdiff.txt21.35 KBtim.plunkett
#153 admin-people-1851086-153.patch44.11 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,510 pass(es).
[ View ]
#153 interdiff.txt1.59 KBtim.plunkett
#151 people-1851086-151.patch44.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,428 pass(es).
[ View ]
#151 interdiff.txt3.45 KBtim.plunkett
#149 user-1851086-149.patch53.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,181 pass(es), 41 fail(s), and 1 exception(s).
[ View ]
#149 interdiff.txt13.8 KBtim.plunkett
#147 people-1851086-147.patch53.94 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,164 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#147 interdiff.txt7.15 KBtim.plunkett
#144 admin-people-1851086-144.patch43.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,334 pass(es), 13 fail(s), and 4 exception(s).
[ View ]
#144 interdiff.txt2.9 KBtim.plunkett
#142 user-1851086-142.patch43.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 32 fail(s), and 8 exception(s).
[ View ]
#142 interdiff.txt7.43 KBtim.plunkett
#137 drupal-1851086-137.patch49.56 KBalimac
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#137 interdiff-125-137.txt3.33 KBalimac
#135 Screen Shot 2013-05-25 at 2.51.14 PM.png65.81 KBalimac
#127 added_role.png36.59 KBKars-T
#126 active.png40.12 KBKars-T
#125 drupal-1851086-125.patch50.2 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,951 pass(es), 42 fail(s), and 1 exception(s).
[ View ]
#125 interdiff.txt7.24 KBdawehner
#124 did_not_block_anyone.png89.79 KBxjm
#122 cancel_what_now.png158.03 KBxjm
#118 drupal-1851086-118.patch47.64 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,677 pass(es).
[ View ]
#116 1851086-114.patch47.65 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#116 interdiff-1851086-114.patch534 bytesdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#114 drupal-1851086-114.patch47.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#114 interdiff.txt2.41 KBdawehner
#112 contextual_links.png24.32 KBxjm
#110 missing_default_tab.png41.19 KBxjm
#104 drupal-1851086-104.patch47.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]
#104 interdiff.txt508 bytesdawehner
#102 interdiff.txt612 bytesdawehner
#102 drupal-1851086-102.patch47.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,454 pass(es).
[ View ]
#100 drupal-1851086-100.patch44.01 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,703 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#100 interdiff.txt4.25 KBdawehner
#88 vdc-1851086-88.patch43.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,754 pass(es).
[ View ]
#88 interdiff.txt4.06 KBtim.plunkett
#88 admin-people.png37.62 KBtim.plunkett
#85 minimal_1.png82.61 KBxjm
#85 minimal_2.png51.78 KBxjm
#82 admin-people-1851086-82.patch45.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,385 pass(es).
[ View ]
#81 admin-people-1851086-81.patch45.56 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,587 pass(es).
[ View ]
#78 one_little_pencil.png54.09 KBxjm
#67 1851086-67-do-not-test.patch47.87 KBdamiankloip
#66 1851086-66.patch45.58 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,294 pass(es), 70 fail(s), and 15 exception(s).
[ View ]
#66 interdiff-1851086-66.txt479 bytesdamiankloip
#63 1851086-63.patch45.55 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,171 pass(es).
[ View ]
#63 interdiff-1851086-63.txt492 bytesdamiankloip
#61 1851086-61.patch46.03 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,200 pass(es).
[ View ]
#61 interdiff.txt1.93 KBdamiankloip
#57 1851086-57.patch45.98 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,966 pass(es).
[ View ]
#57 interdiff.txt504 bytesdamiankloip
#55 1851086-55.patch45.98 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#53 1851086-53.patch45.95 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#53 interdiff.txt417 bytesdamiankloip
#49 1851086-49.patch45.95 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 53,920 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#49 interdiff.txt8.72 KBdamiankloip
#39 vdc-1851086-39.patch42.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,330 pass(es).
[ View ]
#39 interdiff.txt492 bytestim.plunkett
#36 interdiff.txt536 bytestim.plunkett
#36 user-1851086-36.patch42.23 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,096 pass(es).
[ View ]
#34 user-1851086-34.patch42.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#31 vdc-1851086-31.patch43.75 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,403 pass(es).
[ View ]
#31 interdiff.txt4.94 KBtim.plunkett
#29 vdc-1851086-29.patch42.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,046 pass(es).
[ View ]
#29 interdiff.txt1.02 KBtim.plunkett
#27 vdc-1851086-27-FAIL.patch42.97 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,134 pass(es), 48 fail(s), and 3 exception(s).
[ View ]
#27 vdc-1851086-27-PASS.patch42.95 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,122 pass(es), 47 fail(s), and 1 exception(s).
[ View ]
#27 interdiff.txt4.15 KBtim.plunkett
#17 user-1851086-17.patch41.46 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,729 pass(es).
[ View ]
#17 interdiff.txt2.22 KBtim.plunkett
#16 user-1851086-15.patch43.8 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,715 pass(es).
[ View ]
#14 user-1851086-14.patch56.14 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,774 pass(es), 48 fail(s), and 1 exception(s).
[ View ]
#14 interdiff.txt3.22 KBtim.plunkett
#11 user-1851086-11.patch48.87 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,664 pass(es).
[ View ]
#10 user-1851086-10.patch48.63 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,689 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 user-1851086-8.patch42.88 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,431 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#6 user-1851086-6.patch31.95 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,719 pass(es), 35 fail(s), and 7 exception(s).
[ View ]
#3 1851086-3.patch28.43 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 48,749 pass(es), 39 fail(s), and 18 exception(s).
[ View ]
replace-user-admin-with-views.patch26.37 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,762 pass(es), 39 fail(s), and 18 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, replace-user-admin-with-views.patch, failed testing.

damiankloip’s picture

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.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new28.43 KB
FAILED: [[SimpleTest]]: [MySQL] 48,749 pass(es), 39 fail(s), and 18 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1851086-3.patch, failed testing.

xjm’s picture

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Needs work» Needs review
Issue tags:+VDC
StatusFileSize
new31.95 KB
FAILED: [[SimpleTest]]: [MySQL] 50,719 pass(es), 35 fail(s), and 7 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, user-1851086-6.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new42.88 KB
FAILED: [[SimpleTest]]: [MySQL] 49,431 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Also includes patch from #1893800-18: Something is very, very wrong with update.php / upgrade tests... demons suspected because that breaks the upgrade path.

Status:Needs review» Needs work

The last submitted patch, user-1851086-8.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new48.63 KB
FAILED: [[SimpleTest]]: [MySQL] 49,689 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Okay, added translation link, will abstract that out for the dedicated issue.

tim.plunkett’s picture

StatusFileSize
new48.87 KB
PASSED: [[SimpleTest]]: [MySQL] 49,664 pass(es).
[ View ]

Whoops, that was missing some stuff.

dawehner’s picture

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

tim.plunkett’s picture

Status:Needs review» Needs work

Half of that feedback is for #1896268: Add Views integration for translation_entity, which has changed since anyway.

tim.plunkett’s picture

Category:feature» task
Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new3.22 KB
new56.14 KB
FAILED: [[SimpleTest]]: [MySQL] 49,774 pass(es), 48 fail(s), and 1 exception(s).
[ View ]

Rerolled, still includes the user bulk form and the entity translation, both of which are RTBC.
Dries also stated that these conversions are major tasks.

Status:Needs review» Needs work

The last submitted patch, user-1851086-14.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new43.8 KB
PASSED: [[SimpleTest]]: [MySQL] 49,715 pass(es).
[ View ]

This issue is now absorbing #1894972: Provide a bulk form for user operations.

Test coverage needed: Trying to perform actions on the anonymous user. Will do tonight/tomorrow.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
StatusFileSize
new2.22 KB
new41.46 KB
PASSED: [[SimpleTest]]: [MySQL] 49,729 pass(es).
[ View ]

Okay, here's the fix and test coverage webchick asked for in the other issue.

I also minimized the changes made to ApiDataTest (left out of the interdiff, it's 90% a revert of what the patch does).

dawehner’s picture

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

damiankloip’s picture

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

tim.plunkett’s picture

I don't know if it's worth to add explicit checks for the individual user operations, that will vary if more modules eventually implement this

I don't think it's worth skipping entity_load(), doing the reset() is rather tedious...

That check for no valid accounts is to handle if someone builds a view that shows the Anonymous user, and tries to perform an operation on them (which @webchick tried in the other issue).

damiankloip’s picture

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.

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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. :(

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

The first set of those notices are because this is the first time a view is enabled during the installer. The second notice is known.
They're both a symptom of #1822048: Introduce a generic fallback plugin mechanism.

The third part should have had test coverage :(

SchwebDesign’s picture

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.

dawehner’s picture

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new4.15 KB
new42.95 KB
FAILED: [[SimpleTest]]: [MySQL] 50,122 pass(es), 47 fail(s), and 1 exception(s).
[ View ]
new42.97 KB
FAILED: [[SimpleTest]]: [MySQL] 50,134 pass(es), 48 fail(s), and 3 exception(s).
[ View ]

Indeed, there was no test coverage anywhere for adding/removing roles.

Status:Needs review» Needs work

The last submitted patch, vdc-1851086-27-PASS.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
new42.95 KB
PASSED: [[SimpleTest]]: [MySQL] 50,046 pass(es).
[ View ]
dawehner’s picture

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

tim.plunkett’s picture

StatusFileSize
new4.94 KB
new43.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,403 pass(es).
[ View ]

Yeah, I'm not swapping out entity_load just yet, I think that's overkill right now.
Added the missing plugin_id's. We should really find a way to fail testing if they're missing...

dawehner’s picture

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.

tim.plunkett’s picture

Opened #1915686: Ensure that all default views have 'plugin_id' for their handlers as a follow-up.
This will conflict with #1806334: Replace the node listing at /node with a view, but only because the have identical fixes to ApiDataTest::testViewsData(), but that is trivial to reroll.

tim.plunkett’s picture

StatusFileSize
new42.23 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, user-1851086-34.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new42.23 KB
PASSED: [[SimpleTest]]: [MySQL] 53,096 pass(es).
[ View ]
new536 bytes

Oh bother.

ParisLiakos’s picture

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

tim.plunkett’s picture

Yes. They're still there.

Currently, instead of doing something correctly in its own module, there are translation_entity.module hacks located directly in user_admin_account().
In no way should those be there, and its 100% what is blocking us right now.

I have no idea how to unhack this from the Views side of things.

  1. Remove the functionality and comment out the tests, making it translation_entity's problem to do it correctly
  2. Suppress the debug() statements

I vote for 2.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
StatusFileSize
new492 bytes
new42.71 KB
PASSED: [[SimpleTest]]: [MySQL] 53,330 pass(es).
[ View ]

This is what I would do.

webchick’s picture

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

damiankloip’s picture

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

xjm’s picture

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.

damiankloip’s picture

Yeah, it could completely overrun your watchdog table :)

sun’s picture

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.

sun’s picture

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

xjm’s picture

Thanks @sun!

tim.plunkett’s picture

Status:Needs review» Needs work

This is up for grabs.

damiankloip’s picture

Assigned:Unassigned» damiankloip

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

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new8.72 KB
new45.95 KB
FAILED: [[SimpleTest]]: [MySQL] 53,920 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

damiankloip’s picture

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

Status:Needs review» Needs work

The last submitted patch, 1851086-49.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
damiankloip’s picture

StatusFileSize
new417 bytes
new45.95 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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!

Status:Needs review» Needs work

The last submitted patch, 1851086-53.patch, failed testing.

damiankloip’s picture

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review
StatusFileSize
new45.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Oops, forgot the file rename too.

Status:Needs review» Needs work

The last submitted patch, 1851086-55.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new504 bytes
new45.98 KB
PASSED: [[SimpleTest]]: [MySQL] 53,966 pass(es).
[ View ]

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

xjm’s picture

dawehner’s picture

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

dawehner’s picture

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.

damiankloip’s picture

StatusFileSize
new1.93 KB
new46.03 KB
PASSED: [[SimpleTest]]: [MySQL] 54,200 pass(es).
[ View ]

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.

dawehner’s picture

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

damiankloip’s picture

StatusFileSize
new492 bytes
new45.55 KB
PASSED: [[SimpleTest]]: [MySQL] 54,171 pass(es).
[ View ]
Bojhan’s picture

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?

damiankloip’s picture

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

damiankloip’s picture

StatusFileSize
new479 bytes
new45.58 KB
FAILED: [[SimpleTest]]: [MySQL] 54,294 pass(es), 70 fail(s), and 15 exception(s).
[ View ]

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.

damiankloip’s picture

StatusFileSize
new47.87 KB

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

dawehner’s picture

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?

Bojhan’s picture

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

dawehner’s picture

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.

webchick’s picture

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?

dawehner’s picture

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

damiankloip’s picture

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

webchick’s picture

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.

damiankloip’s picture

Nice :)

xjm’s picture

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

xjm’s picture

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

xjm’s picture

StatusFileSize
new54.09 KB

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

Show edit pencil and 'edit view' link on hover, or not?

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

Status:Needs review» Needs work

The last submitted patch, 1851086-66.patch, failed testing.

andypost’s picture

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new45.56 KB
PASSED: [[SimpleTest]]: [MySQL] 54,587 pass(es).
[ View ]

#1966424: Change notice: Allow Views handlers to be optional went in, this now has no open dependencies.

The contextual link is hidden.

There are no more debug errors for the translation.

YAY.

tim.plunkett’s picture

StatusFileSize
new45.3 KB
PASSED: [[SimpleTest]]: [MySQL] 54,385 pass(es).
[ View ]

Replaced Implements/Overrides with {@inheritdoc}

steveoliver’s picture

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.

sun’s picture

Status:Needs review» Reviewed & tested by the community

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.

xjm’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+needs accessibility review
StatusFileSize
new51.78 KB
new82.61 KB

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.

No user accounts are listed on admin/people. There is also no tab to get back to the main page once you click on roles or permissions. Finally, the second link that says 'add user' on the blank page is actually a cancel user link....And when you click the second 'add user' link, it displays a warning because no user accounts were there to cancel, because none are listed on the page.

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!

cbiggins’s picture

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?

xjm’s picture

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.
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new37.62 KB
new4.06 KB
new43.25 KB
PASSED: [[SimpleTest]]: [MySQL] 54,754 pass(es).
[ View ]

Restoring the full functionality of admin/people is a bit too much, IMO. The improvements to the bulk operations is the only progress we've made toward #1839516: Introduce entity operation providers, and we would have to effectively start over with this issue.

So here is a compromise:
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.

The added benefit is that we don't reintroduce the crazy code that blocks the two routing issues linked in the OP.

Here is a screenshot of admin/people without the view:
admin-people.png

klonos’s picture

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

tim.plunkett’s picture

I meant for the nonViews listing, it would have no bulk operations. As you can see by the interdiff, nothing has been changed about the view.

klonos’s picture

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.

andypost’s picture

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

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.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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

xjm’s picture

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

xjm’s picture

Also this.

tim.plunkett’s picture

I think in IRC I told someone I would work on this but I forgot to actually assign it.

But I just can't find the mental energy to keep this issue going. So I'm "unassigning" myself. Sorry @dawehner and @damiankloip.

dawehner’s picture

Assigned:Unassigned» dawehner

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.25 KB
new44.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,703 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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'

Status:Needs review» Needs work

The last submitted patch, drupal-1851086-100.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new47.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,454 pass(es).
[ View ]
new612 bytes

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

dawehner’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

+++ b/core/modules/user/config/views.view.user_admin_people.ymlundefined
@@ -0,0 +1,279 @@
+      hide_admin_links: '1'

This apparently show_admin_links: 0 now? #1877376: Change notice: Improve Views UI text for the contextual links display setting

dawehner’s picture

StatusFileSize
new508 bytes
new47.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]

Good catch!

xjm’s picture

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.

ParisLiakos’s picture

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

ParisLiakos’s picture

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

dawehner’s picture

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.

xjm’s picture

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.

xjm’s picture

Status:Needs review» Needs work
StatusFileSize
new41.19 KB

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

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

xjm’s picture

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

xjm’s picture

StatusFileSize
new24.32 KB

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:

The contextual links are still shown.

xjm’s picture

Issue tags:+Needs followup

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. ;)

xjm’s picture

Issue summary:View changes

Not updating the summary yet

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new47.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  • Let's add the people's tab back
    people.png
  • Fixed the contextual link

Status:Needs review» Needs work

The last submitted patch, drupal-1851086-114.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new534 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new47.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1851086-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The defaults option was not turned off for show_admin_links.

Status:Needs review» Needs work

The last submitted patch, interdiff-1851086-114.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new47.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,677 pass(es).
[ View ]

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

xjm’s picture

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

dawehner’s picture

Assigned:dawehner» Unassigned

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.

xjm’s picture

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

xjm’s picture

StatusFileSize
new158.03 KB

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

xjm’s picture

Oh, re: #120, that looks great.

xjm’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests
StatusFileSize
new89.79 KB

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.

did_not_block_anyone.png

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.24 KB
new50.2 KB
FAILED: [[SimpleTest]]: [MySQL] 55,951 pass(es), 42 fail(s), and 1 exception(s).
[ View ]
  • 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)
Kars-T’s picture

StatusFileSize
new40.12 KB

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

Status "Filter"

Kars-T’s picture

StatusFileSize
new36.59 KB

Adding a role now gives the correct text.

added_role.png

tim.plunkett’s picture

kerasai’s picture

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

Kars-T’s picture

Status:Needs review» Needs work

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.

dawehner’s picture

Status:Needs work» Needs review

I certainly can reproduce this behavior in current HEAD.

xjm’s picture

xjm’s picture

StatusFileSize
new4.8 KB

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.

update_performed.png

xjm’s picture

Status:Needs review» Needs work
alimac’s picture

StatusFileSize
new65.81 KB

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.

Screen Shot 2013-05-25 at 2.51.14 PM.png

(I was going to work on #133)

dawehner’s picture

Status:Needs work» Needs review

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

alimac’s picture

StatusFileSize
new3.33 KB
new49.56 KB
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

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

bshaffer’s picture

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.

Status:Needs review» Needs work

The last submitted patch, drupal-1851086-137.patch, failed testing.

xjm’s picture

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.

xjm’s picture

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.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new7.43 KB
new43.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 32 fail(s), and 8 exception(s).
[ View ]

TypedData is unfun.

Status:Needs review» Needs work

The last submitted patch, user-1851086-142.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
new43.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,334 pass(es), 13 fail(s), and 4 exception(s).
[ View ]

admin/content went in!
Here are a couple test tweaks that were just sloppy bugs from the reroll.

dawehner’s picture

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

tim.plunkett’s picture

Status:Needs review» Needs work

I'll kill the page callbacks, and reroll on top of #2015721: Adding a role to a user throws a php error

We need it to be admin/people/list since its a default task, it will actually be on admin/people.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new7.15 KB
new53.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,164 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Work in progress.

Status:Needs review» Needs work

The last submitted patch, people-1851086-147.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -Needs issue summary update
StatusFileSize
new13.8 KB
new53.22 KB
FAILED: [[SimpleTest]]: [MySQL] 56,181 pass(es), 41 fail(s), and 1 exception(s).
[ View ]

Okay, modernized a bunch of stuff, this should be good.

Status:Needs review» Needs work

The last submitted patch, user-1851086-149.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new3.45 KB
new44.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,428 pass(es).
[ View ]

Oh, right, Views can't override routes yet: #2004300: Extend the router provider interface so that Views can override routes
Let's just stick to the easy parts.

The interdiff is against #147, ignore #149

dawehner’s picture

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

tim.plunkett’s picture

StatusFileSize
new1.59 KB
new44.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,510 pass(es).
[ View ]

1) I agree
2) Oops, that should have been 'content'.

damiankloip’s picture

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

damiankloip’s picture

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.

tim.plunkett’s picture

StatusFileSize
new21.35 KB
new59.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch people-1851086-156.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I'm not changing the logic of system_update_8046() with regard to frontpage, that is pre-existing. I'm just adding in new views.

This re-exports the view, good catch.

Ignoring your other points from #154 as per #155 :)

Yay 15k of extra YAML!

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

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

I think this is ready......

tim.plunkett’s picture

yannickoo’s picture

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

tim.plunkett’s picture

I would guess an issue with the existing views handler. Please file an issue, thanks!

yannickoo’s picture

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

alexpott’s picture

Title:Replace admin/people with a View» Change notice: Replace admin/people with a View
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
+++ 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!

irinaz’s picture

Status:Active» Needs review
ParisLiakos’s picture

Status:Needs review» Active
ibullock’s picture

Assigned:Unassigned» ibullock

going to write the API change notification.

(THIS is the issue I was looking for.)

ibullock’s picture

Status:Active» Needs review

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.
scor’s picture

Issue tags:+Needs change record

update tags (normalize to "Needs change notification")

ibullock’s picture

Assigned:ibullock» Unassigned
dawehner’s picture

Seems like a good short change notice.

xjm’s picture

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

pfrenssen’s picture

Status:Needs review» Needs work

Setting to needs work as per #172.

webwarrior’s picture

catch’s picture

Title:Change notice: Replace admin/people with a View» Replace admin/people with a View
Priority:Critical» Normal
Status:Needs work» Fixed

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.

webwarrior’s picture

Issue tags:-Needs change record

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

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

Anonymous’s picture

Issue summary:View changes

changed issue summary

liuk71’s picture

Status:Closed (fixed)» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 156: people-1851086-156.patch, failed testing.

dawehner’s picture

Issue summary:View changes
Status:Needs work» Closed (fixed)

no reason to open it again.