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
User roles views argument definition specifies that roles_target_id is numeric, but it's machine name (string).
$data['user__roles']['roles_target_id'] = array(
'title' => $this->t('Roles'),
'help' => $this->t('Roles that a user belongs to.'),
'field' => array(
'id' => 'user_roles',
'no group by' => TRUE,
),
'filter' => array(
'id' => 'user_roles',
'allow empty' => TRUE,
),
'argument' => array(
'id' => 'user__roles_rid',
'name table' => 'role',
'name field' => 'name',
'empty field name' => $this->t('No role'),
'zero is null' => TRUE,
<strong>'numeric' => TRUE,</strong>
),
);
Proposed resolution
Remove numeric definition, so the argument will be considered a string.
Remaining tasks
- Create a patch
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | incorrect_definition_of-2799201-23.patch | 499 bytes | robybala |
| #16 | multiple-roles-handlers.png | 49.26 KB | Lendude |
| #12 | incorrect_definition_of-2799201-12.patch | 1.59 KB | Taran2L |
| #12 | interdiff-2-12.txt | 1.34 KB | Taran2L |
Comments
Comment #2
Taran2LComment #3
dawehnerMaybe some testcoverage would be nice here? This could have caught the issue before ...
Comment #4
Taran2LI believe test is related issue #2595025: None validator incorrectly handles numeric arguments covers exactly what you are asking for.
Do you think creating a test/assertion that checks the plugin definition is a good idea?
Comment #5
dawehnerWell, then let's postpone this issue on the other one?
Comment #6
Taran2LI can work on this issue further, because it's very small can be easily fixed. I'm willing to help, just show me the direction.
I'm just asking: are there any tests (you are aware of) that tests views plugin definition?
If yes - I will update patch. If no - should we start writing such kind of tests? Should we create a meta issue to add tests for all the plugin definitions?
Thanks!
Comment #7
Taran2LSeems like general entity tests for views data exist only: see
EntityViewsDataTest. There are no tests for particular entity types' views data. I think either create a separate issue(s) for adding those tests or leave it as is.Comment #8
matslats CreditAttribution: matslats commentedThought I would point out that roles currently appear twice by the end of Drupal\user\UserViewsData::getViewsData();
One is defined in the parent, taken ultimately from the entity definition. Then the users__roles table is explicitly added in Drupal\user\UserViewsData::getViewsData();
Comment #9
Chi CreditAttribution: Chi commentedComment #10
dawehnerWe need at least tests here.
Comment #12
Taran2LNew patch with tests included. Please review
Comment #13
Taran2LComment #15
Taran2LActually it does pass tests. Some test bot glitch I guess ...
Setting back to Needs review
Comment #16
LendudeLooking into this, I would say that
$data['user__roles']['roles_target_id']just needs to be taken out as a whole. As @matslats pointed out in #8, we currently have two Roles contextual filters, neither of which works!We should only need the one provided by
EntityViewsDataI would think, but that currently runs into #2837843: EntityViewsData::mapFieldDefinition incorrectly maps entity reference fields which might be a duplicate of #2846614: Incorrect field name is used in views integration for multi-value base fields.Comment #21
robybala CreditAttribution: robybala commentedPatch updated for 8.5.x
Comment #22
robybala CreditAttribution: robybala commentedComment #23
robybala CreditAttribution: robybala commentedComment #27
neclimdulseems this as fixed in #3132145: Views contextual filter: "allow multiple" doesn't work for user roles filter.