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

Support from Acquia helps fund testing for Drupal Acquia logo

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

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

@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
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.