Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Simple logic bug in calculate dependencies. When no roles are used in the filter, there are no dependencies.
Steps to reproduce:
- From /admin/structure/views/add, create a view showing "Users" and click "Save and edit"
- Add the User -> Roles filter (description text is "Roles that a user belongs to" as there are two User -> Roles filters...)
- In the "Configure filter" dialog, select "Is one of" and select "Administrator" under "Options", click Apply
- 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
- Save the filter settings and save the view
- 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | interdiff-2868841-7-11.txt | 1.02 KB | erozqba |
| #11 | 2868841-11.patch | 2.7 KB | erozqba |
| #8 | 2868841-7--test-only.patch | 1.33 KB | Sam152 |
| #7 | interdiff-2868841-2-7.txt | 1.18 KB | erozqba |
| #7 | 2868841-7.patch | 1.99 KB | erozqba |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
kristiaanvandeneyndeCode 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()Comment #4
benjifisherComment #5
brendon CreditAttribution: brendon commentedWhile 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?
Comment #6
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated the issue summary with steps to reproduce.
Comment #7
erozqba CreditAttribution: erozqba commentedComment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest only patch to verify the test catches the bug.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest looks good. +1 to RTBC.
Comment #11
erozqba CreditAttribution: erozqba commentedAdd a test for not empty too.
Comment #12
mikeker CreditAttribution: mikeker as a volunteer commentedI'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.
Comment #13
kristiaanvandeneyndeYup, looks good. Nice work!
Comment #16
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #18
Wim LeersSomebody who worked on this may be able to resolve the conflict in #2846614: Incorrect field name is used in views integration for multi-value base fields that this introduced.