Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Problem/Motivation

SafeMarkup::checkPlain() is unnecessary.

Proposed resolution

Remove usage from titleQuery - auto escape will do it for us.

Remaining tasks

Do it
Review
Commit

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#10 interdiff.txt485 byteslauriii
#10 remove_all_usages-2560751-10.patch8.28 KBlauriii
#8 interdiff.txt1.04 KBlauriii
#8 remove_all_usages-2560751-8.patch8.28 KBlauriii
#7 interdiff.txt510 byteslauriii
#7 remove_all_usages-2560751-7.patch7.63 KBlauriii
#6 2560751.6.patch6.99 KBalexpott
#3 2560751.3.patch8.2 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
Title: Remove all usages SafeMarkup::checkPlain() from render arrays » Remove all usages SafeMarkup::checkPlain() from Views titleQuery()
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2560751.3.patch8.2 KB

Wow RolesRid is borken in 2 different ways... I think that should be put into a different patch and test coverage added.

To manually test:

  1. Add a new view on content
  2. Add a page display with a path blah/%
  3. Create a contextual filter on Node Revision: Node id (use the revision one it is hard to get the right one of normal node ones)
  4. Use %1 in the title arguments for the contextual filter
  5. Create a node with a title with markup
  6. Go to blah/1
alexpott’s picture

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

Postponing on #2560897: Drupal\user\Plugin\views\argument\RolesRid is very broken

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Postponed
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Postponed » Needs review
FileSize
2560751.6.patch6.99 KB

rebased... now that #2560897: Drupal\user\Plugin\views\argument\RolesRid is very broken has landed. That patch added a test using a role with markup in the title so now we have test coverage.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs review » Reviewed & tested by the community
FileSize
remove_all_usages-2560751-7.patch7.63 KB
interdiff.txt510 bytes

There was still one usage left. Otherwise everything seems to be fine

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
FileSize
remove_all_usages-2560751-8.patch8.28 KB
interdiff.txt1.04 KB

@alexpott spotted that one SafeMarkup::checkPlain() still existed. I also removed the bogus array_combine from StringArgument::titleQuery().

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: remove_all_usages-2560751-8.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Needs review
FileSize
remove_all_usages-2560751-10.patch8.28 KB
interdiff.txt485 bytes

This one should pass

izus queued 10: remove_all_usages-2560751-10.patch for re-testing.

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald commented
Status: Needs review » Reviewed & tested by the community

I've searched for all occurences of titleQuery() after applying the #10 patch - none of them calls SafeMarkup::checkPlain() anymore

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 9210d05 on 8.0.x 
    Issue #2560751 by lauriii, alexpott: Remove all usages SafeMarkup::...

Status: Fixed » Closed (fixed)

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