Problem/Motivation

UserBlocksTest::testUserLoginBlock calls SafeMarkup::set() which is meant to be for internal use only. It only needs to call it so that a proper assertion can be run on HTML that has been parsed by \Drupal::translation()->formatPlural().

Proposed resolution

Because there are no variables used in this markup string, there is no need to use a formatter of any kind. The only need is to mark the string as safe.

The most expedient way to do this is to use SafeString::create.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 None of those are needed, since formatting isn't required.
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it. This is a test, so not needed.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented. Refactor can be successful

Manual testing steps

No manual testing is needed since this is in a test.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#10 interdiff-2550961-6-10.txt383 bytesakalata
#10 remove_or_document-2550961-10.patch1.01 KBakalata
#6 remove_or_document-2550961-6.patch1.04 KBborisson_
#6 interdiff.txt904 bytesborisson_
#3 2550961-3.patch943 bytesjosephdpurcell

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes
josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2550961-3.patch943 bytes

Refactored to use SafeString since this is an internal-use-only test. The string being passed should not be escaped, and we do not need to add it to a list of safe strings.

Updating issue summary to be specific to the issue.

Status: Needs review » Needs work

The last submitted patch, 3: 2550961-3.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/user/src/Tests/UserBlocksTest.php
@@ -50,7 +51,7 @@ function testUserLoginBlock() {
     $this->assertRaw(\Drupal::translation()->formatPlural(1, '1 error has been found: !errors', '@count errors have been found: !errors', [
-      '!errors' => SafeMarkup::set('<a href="#edit-name">Username</a>')
+      '!errors' => SafeString::create('<a href="#edit-name">Username</a>')
     ]));

This is a test - let's just do...
$this->assertRaw('1 error has been found: <a href="#edit-name">Username</a>');

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt904 bytes
new remove_or_document-2550961-6.patch1.04 KB
justachris’s picture

justachris commented
Assigned: Unassigned » justachris

Reviewing

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Looks great apart from...

+++ b/core/modules/user/src/Tests/UserBlocksTest.php
@@ -8,6 +8,7 @@
+use Drupal\Core\Render\SafeString;

This is not used.

justachris’s picture

justachris commented
Assigned: justachris » Unassigned

Additionally

  1. +++ b/core/modules/user/src/Tests/UserBlocksTest.php
@@ -8,6 +8,7 @@
 use Drupal\Component\Utility\SafeMarkup;

    I think we can remove this now that the SafeMarkup::set() is gone, no other uses.

akalata’s picture

akalata commented
Status: Needs work » Needs review
StatusFileSize
new remove_or_document-2550961-10.patch1.01 KB
new interdiff-2550961-6-10.txt383 bytes

Saw that as I was making a new patch as well.

justachris’s picture

justachris commented
Status: Needs review » Reviewed & tested by the community

Updates look good. Setting to RTBC

alexpott’s picture

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

Committed 2b702ac and pushed to 8.0.x. Thanks!

  • alexpott committed 2b702ac on 8.0.x 
    Issue #2550961 by borisson_, akalata, josephdpurcell, justAChris: Remove...

Status: Fixed » Closed (fixed)

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