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

CommentFileSizeAuthor
#12 interdiff.txt1.06 KBkgoel
#12 2550963-12.patch1.33 KBkgoel
#8 interdiff.txt959 byteskgoel
#8 2550963-8.patch1.33 KBkgoel
#6 interdiff.txt1.33 KBkgoel
#6 2550963-6.patch1.34 KBkgoel
#3 2550963-3.patch1.14 KBakalata

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes
akalata’s picture

akalata commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2550963-3.patch1.14 KB

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

akalata commented
Status: Needs review » Postponed
Related issues: +#2550961: Remove or document SafeMarkup::set in UserBlocksTest::testUserLoginBlock

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

kgoel’s picture

kgoel commented
Status: Postponed » Active

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

kgoel’s picture

kgoel commented
Status: Active » Needs review
StatusFileSize
new 2550963-6.patch1.34 KB
new interdiff.txt1.33 KB
akalata’s picture

akalata commented

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

kgoel’s picture

kgoel commented
StatusFileSize
new 2550963-8.patch1.33 KB
new interdiff.txt959 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

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

kgoel commented
Status: Needs work » Needs review
StatusFileSize
new 2550963-12.patch1.33 KB
new interdiff.txt1.06 KB

@justAChris, thank you for review!

Yay, go me :P

akalata’s picture

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

Thanks justachris, I missed that too!

xjm’s picture

xjm
she/her
Primary language English
commented
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.