Problem/Motivation

Follow-up to #2280963: Refactor use of SafeMarkup in HWLDFWordAccumulator. In that issue, a combination of SafeMarkup::checkPlain() and SafeMarkup::set() were converted to SafeMarkup::format(). This makes sense. However, the added use of SafeMarkup::format() is not best practice because it passes a variable as the first argument:

$this->line = SafeMarkup::format($format_string, ['@original_line' => $this->line, '@group' => $this->group]);

This is not technically incorrect because $format_string is defined as one or the other of two string literals in an if/else immediately above and is composed of @ tokens (so there is no chance of unsanitized user input in the variable), and since it's not a translated string, we also don't need to worry about scanning for the translations. However, the code would set a better example (and be easier to read and security audit) if SafeMarkup::format were just used with the appropriate pattern in the two different codepaths.

Proposed resolution

Use SafeMarkup::format with the appropriate pattern in the two different codepaths. Patch attached.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because there is not actually anything functionally wrong with the current code.
Issue priority Normal; it's not that big of a deal and also in part of component code that's barely not vendor code.
Prioritized changes This issue is a followup for a recent major which is also part of an ongoing critical.
Disruption No disruption.

Remaining tasks

Needs review.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
two-formats-both-alike-in-dignity.patch1.07 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Nice filename :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f034232 and pushed to 8.0.x. Thanks!

Thanks for adding the beta to issue summary.

  • alexpott committed f034232 on 8.0.x
    Issue #2509448 by xjm: Further refactor use of SafeMarkup in...

Status: Fixed » Closed (fixed)

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