User password field is empty in Views

I think that is better to remove this field from views, because of:

1. access control handler for the user entity type allows editing the password, but not viewing it. See /core/modules/user/src/UserAccessControlHandler.php:128 (This is the main reason why this field is empty in views results).
2. D7 version of view doesn't have this field in the user field list.

Please review and test. Thanks.

Thanks, as this is a bug report, we need a new test to assert the behaviour (i.e. that this field is not available to views).

We can use \Drupal\Tests\user\Kernel\Views\UserKernelTestBase as a base class and the test would be something like so:

public function testUserPasswordFieldNotAvailableToViews() {
  $this->assertArrayNotHasKey('pass', \Drupal::service('views.views_data')->get('users_field_data'));
}

Thanks!

@larowlan Thanks for your advice. I agree with you.

I have updated the patch. Please test and review. Thanks.

Updated patch, because the previous one failed to apply.

This is a nice bug fix in terms of views - I don't think providing a view that displays password hashes is a great use case for views. We need the issue summary to updated with the solution we've chosen - ie. to remove the field from views.

1. There is a big question about upgrade paths. Atm you can configure a view to have this field. It doesn't work - apparently - but what do we if a user has configured this - do we allow the Views broken handler to kick in - or do we provide an upgrade path? One could argue the view is currently borken and therefore an upgrade path is not needed.

2. +++ b/core/modules/user/src/UserViewsData.php
   index 0d21221841..2f1e987693 100644
   --- a/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
   
   --- a/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
   +++ b/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
   
   +++ b/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
   +++ b/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
   @@ -50,4 +50,11 @@ public function testFieldPermission() {
   
   @@ -50,4 +50,11 @@ public function testFieldPermission() {
        }
      }
    
   +  /**
   +   * Tests if views data object doesn't contain pass field.
   +   */
   +  public function testUserPasswordFieldNotAvailableToViews() {
   +    $this->assertArrayNotHasKey('pass', $this->viewsData->get('users_field_data'));
   +  }
   +
   

   This test is about \Drupal\user\Plugin\views\field\Permissions which is not the as UserViewsData.

   Perhaps this belongs in its own test. Also this test is only asserting for the absence of something which makes the test likely to produce false positives. I think we need at least a positive assertion that the user entity has a field called 'pass' and then the corresponding assert that this field is not in views data.

3. +++ b/core/modules/user/tests/src/Kernel/Views/UserKernelTestBase.php
   @@ -38,6 +38,13 @@
   +  /**
   +   * The views data service.
   +   *
   +   * @var \Drupal\views\ViewsData
   +   */
   +  protected $viewsData;
   
   @@ -48,6 +55,8 @@ protected function setUp($import_test_views = TRUE) {
   +
   +    $this->viewsData = $this->container->get('views.views_data');
   

   Let's not add a service as a class property to a TestBase class for a single testAssertion.

@alexpott Thanks for your full review.
1. About issue summary - ok, I will update it after updating the patch.
2,3 It's clear, I agree with you.

I'm going to update the patch shortly.

I updated the patch and issue summary according to the @alexpott feedback. Please test and review.

Updated patch, because the previous one failed to apply.

+++ b/core/modules/user/tests/src/Kernel/Views/UserViewsDataTest.php
@@ -0,0 +1,57 @@
+    if ($this->assertArrayHasKey('pass', $field_definitions)) {
+      $this->assertArrayNotHasKey('pass', $this->viewsData->get('users_field_data'));
+    }

Assertions in PHPUnit do not have return values. They either pass, or they throw an exception and abort the test in which the failed assertion was executed. In this case, the second assertion is not evaluated. You can verify this by running the test, which will output "OK (1 test, 1 assertion)" (note that it only counts one assertion instead of 2). Since you expect both assertions to pass, just execute them sequentually and remove the if-statement.

However, if I do that, the second assertions fails with:

1) Drupal\Tests\user\Kernel\Views\UserViewsDataTest::testUserPasswordFieldNotAvailableToViews
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "action" entity type does not exist.

/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeManager.php:150
/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeManager.php:269
/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeManager.php:208
/var/www/html/core/modules/views/views.views.inc:158
/var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php:392
/var/www/html/core/modules/views/src/ViewsData.php:245
/var/www/html/core/modules/views/src/ViewsData.php:162
/var/www/html/core/modules/user/tests/src/Kernel/Views/UserViewsDataTest.php:53

So the test still needs some tweaking.

Working on this from DrupalCon Amsterdam!

I've taken a look at this, and apparently the action_entity_type_build() function from the action module is being called from views.views.inc:views_views_data(), and fails because the action module isn't enabled in the test.

I updated the main test as per #16, and tried just adding the module to the UserViewsDataTest::$modules array, without much luck (got an error from inside the action_entity_type_build() function), but this change in views.views.inc seems to work and hopefully even makes sense.

Also, I'm not sure if this thing should be split into its own issue on the "views" project.

Thanks for working on this @cambraca!

+++ b/core/modules/views/views.views.inc
@@ -155,9 +155,11 @@ function views_views_data() {
-    $actions = array_filter(\Drupal::entityTypeManager()->getStorage('action')->loadMultiple(), function (ActionConfigEntityInterface $action) use ($entity_type) {
-      return $action->getType() == $entity_type;
-    });
+    if (\Drupal::moduleHandler()->moduleExists('action')) {
+      $actions = array_filter(\Drupal::entityTypeManager()->getStorage('action')->loadMultiple(), function (ActionConfigEntityInterface $action) use ($entity_type) {
+        return $action->getType() == $entity_type;
+      });
+    }

The action entity type is not provided by the action module (which is why adding it to UserViewsDataTest::$modules did not fix your issue), but rather by the system module. Views has drupal:system as an indirect dependency, however in a KernelTest you have to explicitly specify the dependencies you need. Try adding 'system' to UserViewsDataTest::$modules instead of altering views, this should get rid of the test failure.

Working on this from DrupalCon Amsterdam!

Adding tags

Thanks for the guidance! You're absolutely right. In my defense, it is a bit confusing ("action" module vs. entity type).

Here's a new patch, much closer to the one from #14. The test now runs fine on my local.

Would it make sense for the "views" module to have "system" as a dependency?

Would it make sense for the "views" module to have "system" as a dependency?

Technically, this already is the case: views has a dependency on drupal:filter, drupal:filter has a dependency on drupal:user, drupal:user has a dependency on drupal:system. So you cannot enable views on a site without also installing the filter+user+system. KernelTestBase ignores a module's dependencies during test though, which is why you have to explictly specify it.

Needs review for the patch in #20.

+++ b/core/modules/user/src/UserViewsData.php
@@ -249,6 +249,10 @@ public function getViewsData() {
+    // Unset pass field, because access control handler for the user entity
+    // type allows editing the password, but not viewing it.

Comments should be complete sentences, so "Unset the pass field, because the access control handler ...". I would also put quotes around "pass". If you alter the comment, make sure you don't exceed the 80 chars per line limit. See https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... for more information on coding standards.

KernelTestBase ignores a module's dependencies during test

I learned yet another thing today!

Here's a new patch with the changes to the comment.

There is the question of the upgrade path in #9. Prior to this patch, adding the pass field to a view does not do anything, no information is shown and no indication is given that the field does not work. After this patch, the field handler is replaced by the "broken/missing handler". I think this is the right approach.

Since we are removing a Views field, I think it's best to have a change record explaining why it was removed.

Agree that no upgrade path is needed nor useful, it is better to show broken/missing handler - it was broken before anyway.

Committed 999dbef and pushed to 9.1.x. Thanks!

Going to ask release managers about backporting to 8.9.x and 9.0.x

I had to hot fix the test for the void return so here's a patch for 9.0.x... #24 is good for 8.9.x.

I'm going to go ahead and set this issue back to fixed against 9.1.x since 9.0.x is nearing the end of its active support; it does change the error behavior and could have side effects for contrib we didn't think of.