Problem/Motivation

__toString method in Attribute class 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.
Files: 
CommentFileSizeAuthor
#13 Source_of__http___d8_dev_admin_structure_views_add.png92.56 KBjoelpittet
#11 remove_safemarkup_set-2501667-11.patch3.37 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,691 pass(es), 17 fail(s), and 0 exception(s). View
#7 remove_or_document-2501667-7.patch3.4 KBdtraft
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,631 pass(es), 84 fail(s), and 2 exception(s). View
#7 interdiff.txt799 bytesdtraft
#5 remove_or_document-2501667-5.patch3.56 KBdtraft
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,784 pass(es), 22,010 fail(s), and 3,397 exception(s). View
#5 interdiff.txt2.72 KBdtraft
#4 interdiff.txt1.34 KBdtraft
#4 remove_or_document-2501667-3.patch1.45 KBdtraft
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,962 pass(es), 383 fail(s), and 64,389 exception(s). View
#2 edit_issue_remove_or-2501667-1.patch1.46 KBdtraft
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,405 pass(es), 1,309 fail(s), and 64,042 exception(s). View

Comments

peezy’s picture

We're working on this as part of the NHDevDays code sprint.

dtraft’s picture

Status: Active » Needs review
FileSize
1.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,405 pass(es), 1,309 fail(s), and 64,042 exception(s). View

This patch is the result of collaboration between @kbaringer, @peezy, @jbradley428, and @dtraft. Have at it, testbot!

Status: Needs review » Needs work

The last submitted patch, 2: edit_issue_remove_or-2501667-1.patch, failed testing.

dtraft’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,962 pass(es), 383 fail(s), and 64,389 exception(s). View
1.34 KB

Minor consistency upgrades and Drupal coding standard compliance.

dtraft’s picture

FileSize
2.72 KB
3.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,784 pass(es), 22,010 fail(s), and 3,397 exception(s). View

Fixing errors and small performance improvments

The last submitted patch, 4: remove_or_document-2501667-3.patch, failed testing.

dtraft’s picture

FileSize
799 bytes
3.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,631 pass(es), 84 fail(s), and 2 exception(s). View

Small fix for casting type error.

The last submitted patch, 5: remove_or_document-2501667-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: remove_or_document-2501667-7.patch, failed testing.

peezy’s picture

Title: Remove or document SafeMarkup::set in __toString() in Attribute class » Remove SafeMarkup::set in __toString() in Attribute class
Issue summary: View changes

Updated Issue summary.

cwells’s picture

Assigned: Unassigned » cwells
Status: Needs work » Needs review
FileSize
3.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,691 pass(es), 17 fail(s), and 0 exception(s). View

That return was outside the loop, and I'm not sure why it was needed. At best, I can think that it was to avoid a SafeMarkup::format() call if there was nothing to call it in, so I used the ternary return based on non-empty $keys.

@kbaringer helped!

Status: Needs review » Needs work

The last submitted patch, 11: remove_safemarkup_set-2501667-11.patch, failed testing.

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Template/AttributeValueBase.php
@@ -57,7 +57,7 @@ public function __construct($name, $value) {
+      return SafeMarkup::format('@name="@value"', ['@name' => $this->name, '@value' => $value]);

Unfortunately @ != to SafeMarkup::checkPlain it is = SafeMarkup::escape() which checks if it's safe before checkPlain'ing it. This means if you pass something from t() through to a title attribute for example and it has double quotes in it(like the failing tests), those quotes won't be escaped and the markup is broken.

I'm going to add this to the postponed list till we figure out this joining issue:
#2501975: Determine how to update code that currently joins strings in SafeMarkup::set()

joelpittet’s picture

Status: Needs work » Postponed
pwolanin’s picture

I'm not sure it makes sense to remove this call.

pwolanin’s picture