Problem/Motivation

Much like \Drupal\views\Plugin\views\filter\Bundle calculates the dependencies for its required bundle entities,
Drupal\user\Plugin\views\filter\Roles should calculate the user roles as part of their config depdencies.

Proposed resolution

  • Implement calculateDependencies() in Roles.php
  • There, load the roles, and call getConfigDependencyKey and getConfigDependencyName
  • Add some test coverage to ensure that this happens.

Remaining tasks

User interface changes

API changes

Wanna help?

This issue is a follow-up for #2372855: Add content & config entity dependencies to views. There, we made all Views plugins calculate which dependencies they need. This is important when deploying a view; when it's deployed, we want to make sure that all things it needs to work correctly, actually exist. Until before that issue, we didn't check that, which could lead to broken sites.
Unfortunately, we forgot to update at least one Views plugin: Drupal\user\Plugin\views\filter\Roles.
You can look at the changes made to Bundle.php in #2372855-84: Add content & config entity dependencies to views as an example on how to update the Roles plugin: implement a calculateDependencies() method, and inject any services you need.
When that's done, please also verify that all other Views plugins do calculate their dependencies.

Files: 
CommentFileSizeAuthor
#19 2407107-interdiff-16-19.txt1.97 KBjan.stoeckler
#19 2407107-19-Roles-calculateDependencies.patch4.32 KBjan.stoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,080 pass(es). View
#16 2407107-16-Roles-calculateDependencies.patch4.3 KBjan.stoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,000 pass(es). View

Comments

Wim Leers’s picture

Issue tags: +D8 cacheability, +VDC
Wim Leers’s picture

Title: Drupal\user\Plugin\views\filter\Roles should implement » Drupal\user\Plugin\views\filter\Roles should implement calculateDependencies()
dawehner’s picture

This is a good issue for someone on a sprint during the weekend.

Wim Leers’s picture

Indeed!

xjm’s picture

Category: Task » Bug report
Priority: Major » Critical
Issue tags: +Configuration system

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?

dawehner’s picture

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?

The filter itself just uses the string, so the filtering will continue to work, and so depending on the data, return nothing or the data which are in the tables with the corresponding values. We should write all filters that way, that they don't need the objects IMHO.

xjm’s picture

Issue tags: +D8 upgrade path

Thanks @dawehner!

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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

dawehner’s picture

@Wim Leers
I was planning to give this to someone on a sprint. This is a task a lot of people, with some instructions, can work one.
This has the advantage that the person might also be able to go through all the other handlers checking whether we missed some.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
Issue tags: +SprintWeekend2015Queue

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

dawehner’s picture

That is helpful, thank you for updating the IS wim.

jan.stoeckler’s picture

Assigned: Unassigned » jan.stoeckler

I'll try this one.

Wim Leers’s picture

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

jan.stoeckler’s picture

Issue tags: +SprintWeekend2015
FileSize
4.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,000 pass(es). View

OK, let's see how that goes.

jan.stoeckler’s picture

Status: Active » Needs review

Sorry, status change.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -SprintWeekend2015Queue

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.

jan.stoeckler’s picture

FileSize
4.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,080 pass(es). View
1.97 KB

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

jan.stoeckler’s picture

Status: Needs work » Needs review

Sorry, again.

Berdir’s picture

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.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 0a38107 on 8.0.x
    Issue #2407107 by jan.stoeckler: Drupal\user\Plugin\views\filter\Roles...
Wim Leers’s picture

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

dawehner’s picture

That's simple, consistent, logical.

For phpunit tests I totally think we should use the full power. If simpletest doesn't support that, so what.

Wim Leers’s picture

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

Status: Fixed » Closed (fixed)

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