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

Comments

alexpott created an issue. See original summary.

alexpott’s picture

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2560897.3.test-only.patch7.19 KB
new 2560897.3.patch9.43 KB

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

dawehner’s picture

dawehner
Primary language German
commented

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

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
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 🇪🇺🌍
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
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
dawehner’s picture

dawehner
Primary language German
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
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.