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
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
Comment | File | Size | Author |
---|---|---|---|
two-formats-both-alike-in-dignity.patch | 1.07 KB | xjm | |
Comments
Comment #1
star-szrNice filename :)
Comment #2
alexpottCommitted f034232 and pushed to 8.0.x. Thanks!
Thanks for adding the beta to issue summary.