Problem/Motivation

ShortcutSetsTest::testShortcutSetSwitchNoSetName calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.

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
  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 issue is fixing a test.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps

N/A

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#12 interdiff.txt1.06 KBkgoel
#12 2550963-12.patch1.33 KBkgoel
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,209 pass(es). View
#8 interdiff.txt959 byteskgoel
#8 2550963-8.patch1.33 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,112 pass(es), 2 fail(s), and 0 exception(s). View
#6 interdiff.txt1.33 KBkgoel
#6 2550963-6.patch1.34 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,095 pass(es), 2 fail(s), and 0 exception(s). View
#3 2550963-3.patch1.14 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). View

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.14 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). 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.

akalata’s picture

Postponing on #2550961: Remove or document SafeMarkup::set in UserBlocksTest::testUserLoginBlock since the solution should be nearly identical.

kgoel’s picture

Status: Postponed » Active

Per @xjm - this is something that can be work on.

kgoel’s picture

Status: Active » Needs review
FileSize
1.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,095 pass(es), 2 fail(s), and 0 exception(s). View
1.33 KB
akalata’s picture

Looks like #6 adds a new line in the code where there was not one in the code before?

kgoel’s picture

FileSize
1.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,112 pass(es), 2 fail(s), and 0 exception(s). View
959 bytes

@akalata - thank you! I did add a new line :P

The last submitted patch, 6: 2550963-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2550963-8.patch, failed testing.

justAChris’s picture

+++ b/core/modules/shortcut/src/Tests/ShortcutSetsTest.php
@@ -133,9 +132,7 @@ function testShortcutSetSwitchCreate() {
+    $this->asssertRaw('1 error has been found: <a href="#edit-label">Label</a>');

Likely failing on the extra "s" in assert

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,209 pass(es). View
1.06 KB

@justAChris, thank you for review!

Yay, go me :P

akalata’s picture

Status: Needs review » Reviewed & tested by the community

Thanks justachris, I missed that too!

xjm’s picture

Status: Reviewed & tested by the community » Fixed

This is indeed much better -- as much as possible, tests should just test the expected result without adding dependencies on other code.

Committed and pushed to 8.0.x. Thanks!

  • xjm committed 9594553 on 8.0.x
    Issue #2550963 by kgoel, akalata, josephdpurcell, justAChris: Remove or...

Status: Fixed » Closed (fixed)

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