Problem/Motivation

AssertContentTrait::assertEscaped() currently uses SafeMarkup::checkPlain(), which adds the string to the list of safe strings. This is unnecessary.

Proposed resolution

Swap the SafeMarkup::checkPlain() call for Html::escape()

Remaining tasks

Make patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because current function works as documented.
Issue priority Normal because nothing is broken.
Prioritized changes The main goal of this issue is to remove a SafeMarkup::checkPlain as part of #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible
Disruption None; internal change with no outside effects.
CommentFileSizeAuthor
#2 2552893-2.patch1.37 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2552893-2.patch1.37 KB
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue tags: +SafeMarkup
Parent issue: #2550945: Add Html::escape() » #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping
Related issues: +#2550945: Add Html::escape()
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Looks good to me!

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs review » Reviewed & tested by the community

I think we can RTBC this as SafeMarkup::checkPlain() just wraps Html::escape() and tests are still green.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

+1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

I guess this just means there is less to do in #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping. Test only change and therefore beta permitted. Committed 2ecd353 and pushed to 8.0.x. Thanks!

  • alexpott committed 2ecd353 on 8.0.x 
    Issue #2552893 by mpdonadio: AssertContentTrait::assertEscaped() should...

Status: Fixed » Closed (fixed)

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