Problem/Motivation

Simple logic bug in calculate dependencies. When no roles are used in the filter, there are no dependencies.

Steps to reproduce:

  1. From /admin/structure/views/add, create a view showing "Users" and click "Save and edit"
  2. Add the User -> Roles filter (description text is "Roles that a user belongs to" as there are two User -> Roles filters...)
  3. In the "Configure filter" dialog, select "Is one of" and select "Administrator" under "Options", click Apply
  4. Edit the filter settings again, this time select either "Only has the 'authenticated user' role" or "Has roles in addition to 'authenticated user'" for empty and not-empty, respectively
  5. Save the filter settings and save the view
  6. Navigate to /admin/config/development/configuration/single/export and pull up the config for the view created in step #1

The export will show

dependencies:
  config:
    - user.role.administrator

which is incorrect. Note that unselecting the Administrator role will remove the config dependency.

Proposed resolution

Do not return any dependencies for empty/not-empty operators.

Remaining tasks

Add tests.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +Novice
FileSize
673 bytes
kristiaanvandeneynde’s picture

Status: Needs review » Needs work

Code looks good! Needs work for not having a test.

You should be able to pull it off rather easily by adding to \Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest::testDependencies()

benjifisher’s picture

Issue tags: +Baltimore2017
brendon’s picture

While attempting to provide tests I was unable to reproduce the issue. I was unable to find the empty/not empty operator in the filter. Could you add steps to reproduce?

mikeker’s picture

Issue summary: View changes

Updated the issue summary with steps to reproduce.

erozqba’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
1.18 KB
Sam152’s picture

Test only patch to verify the test catches the bug.

Status: Needs review » Needs work

The last submitted patch, 8: 2868841-7--test-only.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review

Test looks good. +1 to RTBC.

erozqba’s picture

Add a test for not empty too.

mikeker’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I've verified the patch in #11 using the STR in the issue summary and the config dependency is removed as expected. We've got a patch, we've got tests, I say we're RTBC.

kristiaanvandeneynde’s picture

Yup, looks good. Nice work!

  • catch committed 0653d6b on 8.4.x
    Issue #2868841 by erozqba, Sam152, kristiaanvandeneynde, mikeker: \...

  • catch committed f5f7573 on 8.3.x
    Issue #2868841 by erozqba, Sam152, kristiaanvandeneynde, mikeker: \...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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