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

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


API changes


two-formats-both-alike-in-dignity.patch1.07 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,081 pass(es). View


Cottser’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.