Correcting UserViewsFieldAccessTest

Awesome, thanks for creating the spin-off @vaplas!

In the other issue, we decided to remove the changes to the FileManagedUnitTestBase class because we could not know who could have extended that. Shouldn't we be doing the same here and fix the base class in the UID 1 patch?

We could update UserViewsFieldAccessTest to recreate the user and load the admin role from the parent to add the proper permissions. Maybe it's overkill and we should just leave the test base as is. I'm not sure.

How about we stick with the patch in #2 and see what the core committers think of it?

Okay, RTBC for #2 then. Let's see what core committers have to say about the base class change. Impact should be minimal to no as it adds the right permission for the base class to still function, but you never know.

1. +++ b/core/modules/user/tests/src/Kernel/Views/UserViewsFieldAccessTest.php
   @@ -46,20 +52,50 @@ public function testUserFields() {
   +    $this->assertFieldAccess('user', 'name', 'test user');
   

   is this correct? I thought user names were only available for those with the 'view user profiles' permission? Apologies if that is already granted but not present in the diff.

2. +++ b/core/modules/user/tests/src/Kernel/Views/UserViewsFieldAccessTest.php
   @@ -46,20 +52,50 @@ public function testUserFields() {
   +    // $this->assertFieldAccess('user', 'mail', 'druplicon@drop.org');
   +    // $this->assertFieldAccess('user', 'created', \Drupal::service('date.formatter')->format(123456));
   +    // $this->assertFieldAccess('user', 'changed', \Drupal::service('date.formatter')->format(REQUEST_TIME));
   ...
   +    // $this->assertFieldAccess('user', 'mail', 'druplicon@drop.org');
   

   Let's punt this to the followup (existing followup)

3. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldFieldAccessTestBase.php
   @@ -40,9 +40,14 @@ protected function setUp($import_test_views = TRUE) {
   +    User::create(['uid' => 1, 'name' => 'user1'])->save();
   

   yeah I'm with @kristiaanvandeneynde, lets not do this in the base class as it may lead to unexpected issues

@larowlan in #12:

1. Everyone can view it as it's the label. See below, taken from UserAccessControlHandler
   

   case 'name':
           // Allow view access to anyone with access to the entity. Anonymous
           // users should be able to access the username field during the
           // registration process, otherwise the username and email constraints
           // are not checked.
           if ($operation == 'view' || /* snip */ ) {
             return AccessResult::allowed()->cachePerPermissions();
           }

2. Okay so which part do we punt? All of it, including the lines that were there (commented out) but got rearranged? Trying to not throw out the rearranged lines is near impossible because they never should have been in the low-access test in the first place. They belong in the admin-access test.
3. Done

This test might fail as I threw out the 'access content' permission because I don't think we need it in the user test. We might need to add said permission in the node tests in the parent issue that strips UID 1 of its power. I'll add a comment in the parent issue if this one goes green.

The patch also contains the commented out lines still. I'm not sure which ones to remove (as stated in 2.)

Unrelated failure

2018 bump. This is the first domino in a series that might lead to getting #540008: Add a container parameter that can remove the special behavior of UID#1 committed.

Reroll the patch. Please review.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

+    Role::create([
+      'id' => 'user_admin',
+      'label' => 'User role',
+      'permissions' => ['administer users'],
+    ])->save();

Instead of creating roles and users this way think it should use UserCreationTrait