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

Files: 
CommentFileSizeAuthor
#10 interdiff-2550961-6-10.txt383 bytesakalata
#10 remove_or_document-2550961-10.patch1.01 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
#6 remove_or_document-2550961-6.patch1.04 KBborisson_
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,571 pass(es). View
#6 interdiff.txt904 bytesborisson_

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
943 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

+++ 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

Status: Needs work » Needs review
FileSize
904 bytes
1.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,571 pass(es). View
justAChris’s picture

Assigned: Unassigned » justAChris

Reviewing

alexpott’s picture

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

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

Status: Needs work » Needs review
FileSize
1.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
383 bytes

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

justAChris’s picture

Status: Needs review » Reviewed & tested by the community

Updates look good. Setting to RTBC

alexpott’s picture

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.