Drupal\user\Plugin\views\filter\Roles should implement calculateDependencies()

Indeed!

This is definitely a bug, and probably critical as the config dependencies will be missing. Since filtering a view by a role is a way of restricting access, this might also have security implications. What happens currently if you deploy a view filtered by role without that role? Is there an exception, or does the filter fail silently?

Thanks @dawehner!

I'll take this one on later today, unless somebody beats me to it (@dawehner, if you're already on it, let me know).

Fair point; added instructions instead, tagged for the sprint.

I'll try this one.

Awesome; thank you! Let me know if you're stuck, I'll try to help :)

OK, let's see how that goes.

Sorry, status change.

AWESOME work! Thank you very much!

1. +++ b/core/modules/user/src/Plugin/views/filter/Roles.php
   @@ -18,6 +20,42 @@
   +   * Constructs a Roles.
   

   s/Roles/Roles object/

2. +++ b/core/modules/user/src/Plugin/views/filter/Roles.php
   @@ -33,4 +71,16 @@ function operators() {
   +      $role_entity = $this->roleStorage->load($role_id);
   

   s/$role_entity/$role/ to make it slightly nicer? :)

3. +++ b/core/modules/user/src/Tests/Views/HandlerFilterRolesTest.php
   @@ -0,0 +1,71 @@
   + * @group user
   

   Let's also add @group views.

4. +++ b/core/modules/user/src/Tests/Views/HandlerFilterRolesTest.php
   @@ -0,0 +1,71 @@
   + * @see \Drupal\user\Plugin\views\filter\Roless
   

   s/Roless/Roles/

   :)

5. +++ b/core/modules/user/src/Tests/Views/HandlerFilterRolesTest.php
   @@ -0,0 +1,71 @@
   +   * Tests the roles filter handler.
   +   */
   +  public function testFilterRoles() {
   

   THANK YOU for writing a test! :)

   But the docs/function name are a bit misleading: they suggest it tests the actual filter, but it only tests the calculated dependencies. So I suggest calling it testFilterDependencies() or something like that.

OK, second attempt. I omitted #3 as I was told only one group should be specified. Will add the second one if absolutely essential.

Sorry, again.

On, three, I agree that there should only be one @group, we can't handle more than one in Simpletest (UI) at the moment. @Wim, see my last comment in #2301481: Mark test groups as belonging to modules in UI.

Yes, adding a second group would be pretty non-standard. We have a pretty clear 1-1 relationship between @group and module/component name. I'd rather not muddy those waters. Also all the test for the user views handlers also specify @user only (which proves the point), so...

Patch looks good and Wim's review is fixed and nothing major was brought up by Wim, so I feel comfortable marking this RTBC :-)

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0a38107 and pushed to 8.0.x. Thanks!

When that's done, please also verify that all other Views plugins do calculate their dependencies.

I don't think that should happen in this issue. We could open an issue to do that.

#21/#22: that's total news to me; I've been told for a long time that I should associate all relevant @groups. I think dawehner taught me that, and insisted on that in his patch reviews. PHPUnit supports it just fine in any case. I agree with Berdir's proposal at #2301481-13: Mark test groups as belonging to modules in UI:

if multiple groups, then the first should be the one it is supposed to be shown in the current UI

That's simple, consistent, logical.

#26: agreed. But just having "put the extension's name as the first @group" does not limit PHPUnit's power :) That's what I like in Berdir's proposal: best of both worlds.