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.
CommentFileSizeAuthor
#13 Source_of__http___d8_dev_admin_structure_views_add.png92.56 KBjoelpittet
#11 remove_safemarkup_set-2501667-11.patch3.37 KBchrisfromredfin
#7 remove_or_document-2501667-7.patch3.4 KBdtraft
#7 interdiff.txt799 bytesdtraft
#5 remove_or_document-2501667-5.patch3.56 KBdtraft
#5 interdiff.txt2.72 KBdtraft
#4 interdiff.txt1.34 KBdtraft
#4 remove_or_document-2501667-3.patch1.45 KBdtraft
#2 edit_issue_remove_or-2501667-1.patch1.46 KBdtraft

Comments

peezy’s picture

peezy commented
Issue tags: -D8 Accelerate Dev Days +D8 Accelerate

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

dtraft’s picture

dtraft commented
Status: Active » Needs review
StatusFileSize
new edit_issue_remove_or-2501667-1.patch1.46 KB

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

dtraft commented
Status: Needs work » Needs review
StatusFileSize
new remove_or_document-2501667-3.patch1.45 KB
new interdiff.txt1.34 KB

Minor consistency upgrades and Drupal coding standard compliance.

dtraft’s picture

dtraft commented
StatusFileSize
new interdiff.txt2.72 KB
new remove_or_document-2501667-5.patch3.56 KB

Fixing errors and small performance improvments

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

dtraft’s picture

dtraft commented
StatusFileSize
new interdiff.txt799 bytes
new remove_or_document-2501667-7.patch3.4 KB

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

peezy commented
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.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Assigned: Unassigned » chrisfromredfin
Status: Needs work » Needs review
StatusFileSize
new remove_safemarkup_set-2501667-11.patch3.37 KB

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

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
StatusFileSize
new Source_of__http___d8_dev_admin_structure_views_add.png92.56 KB 
+++ 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

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Postponed
pwolanin’s picture

pwolanin commented

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

pwolanin’s picture

pwolanin commented
Status: Postponed » Closed (duplicate)

dup now to #2505701: Document SafeMarkup::set and Use htmlspecialchars() directly in Attribute() so we don't bloat the list of safe strings