Blocks #2560751: Remove all usages SafeMarkup::checkPlain() from Views titleQuery()

Problem/Motivation

Can't add a contextual filter for a user's roles.

Proposed resolution

Fix and text code

Remaining tasks

Do it
Review
Commit

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#3 2560897.3.patch9.43 KBalexpott
#3 2560897.3.test-only.patch7.19 KBalexpott
#2 2560897.2.patch2.24 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2560897.2.patch2.24 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2560897.3.test-only.patch7.19 KB
2560897.3.patch9.43 KB

Added a test. The test only patch is the interdiff.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I'm curious ... why does the HandlerAllTest.php not find that problem?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented

The patch works by a manual test and the fix seems to be correct. Anyway it seems like the HandlerAllTest doesn't test RolesRid at all and that is why it didn't fail. Maybe we should figure out why it's so?

The last submitted patch, 3: 2560897.3.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@dawehner because it never tests user__roles - it is not a base table...

    foreach ($this->container->get('views.views_data')->get() as $base_table => $info) {
      if (!isset($info['table']['base'])) {
        continue;
      }
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Test looks good for me, the follow up is in #2560949: Expland HandlerAllTest to find broken stuff

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/user/src/Tests/Views/RolesRidArgumentTest.php
@@ -0,0 +1,38 @@
+   * Tests the generated title of an user: roles argument.

Fixing on commit: 'a user'.

Committed/pushed to 8.0.x, thanks!

  • catch committed f3040e7 on 8.0.x 
    Issue #2560897 by alexpott: Drupal\user\Plugin\views\argument\RolesRid...

  • catch committed ca9f585 on 8.0.x 
    Revert "Issue #2560897 by alexpott: Drupal\user\Plugin\views\argument\...
  • catch committed d36af97 on 8.0.x
    Issue #2560897 by alexpott: Drupal\user\Plugin\views\argument\RolesRid...

Status: Fixed » Closed (fixed)

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