The motivation here is simply best practices for Drupal. The problem is the implementation of hook_field_formatter_settings_summary(), which has the following:
$summary = t('Trim length') . ': ' . $settings['trim_length'];
As the title says, this shouldn't be done like this.
I suggest we use an @placeholder like everywhere else in this module.
$summary = t('Trim length: @trim_length', array(
'@trim_length' => $settings['trim_length'],
));
I'll add a comment with a patch.
The entire D8 change can't be backported due to it breaking translations.
The change to D8 added variable sanitization where there was none previously. A patch to backport the variable sanitization exists in #9.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | text-check_plain-1790612-18.patch | 599 bytes | mgifford |
| #9 | text-check_plain-1790612-9.patch | 566 bytes | eric_a |
| #7 | text-format_string-1790612-7.patch | 602 bytes | eric_a |
| #3 | text-do-not-concat-variables-t-function-1790612-1.patch | 619 bytes | carwin |
| #1 | text-do-not-concat-variables-t-function-1790612-1.patch | 618 bytes | carwin |
Comments
Comment #1
carwin commentedNotes
The only reason the same patch cannot be applied is that the function starts one line higher in d8 than in d7.
Comment #3
carwin commentedOopsie daisy, forgot a parentheses in the d8 version.
Comment #4
das-peter commentedAs carwin mentioned I'd say this is RTBC. But another review would be nice :)
Comment #5
larowlanNice catch.
I don't think this can be backported without breaking existing translations.
Comment #6
dries commentedCommitted to 8.x. Thanks.
Comment #7
eric_a commentedSanitizing of the trim_length variable sneeked in with the t() concat fix. Here's a patch that does just the sanitizing for D7.
Comment #8
tstoecklerformat_string() seems a little verbose for that. Can't we do a check_plain() directly?!
Comment #9
eric_a commentedYeah.
BTW, the patch in #7 is missing quotes around the array key...
Comment #10
tstoeckler...which means this isn't tested at all :-/
That shouldn't be part of this issue, though.
Comment #11
mgifford@Eric_A the latest patch still applies.
This is a 1 line patch that's already in D8. Should be an easy fix.
Comment #12
dcam commented#9 looks ok to me. It only adds sanitization where there is none.
Comment #13
dcam commentedComment #16
dcam commentedComment #17
tstoecklerSorry for only noticing this now but I think the D7 version should follow what the D8 version does, i.e. see the patch in #3.
Comment #18
mgiffordGood point @tstoeckler - The D8 way should be better for translation. However, it will be a change so that string will need to be adjusted in the existing translations.
Comment #19
tstoecklerAhh, right. That's true. Hmm... maybe that's why the patch in #9 was proposed in that way? Not sure.
Anyway, it seems due to #9/#10 this needs manual testing? Otherwise it's RTBC.
Sorry for holding this up :-/
Comment #20
dcam commentedYeah, in #7 it was noted that the 8.x fix happened to add variable sanitization where there previously was none. So that's the only 8.x change that @Eric_A sought to backport.
Moving #9 back to RTBC.
Comment #21
tstoecklerOK, sorry for the noise.
Comment #23
dcam commented#9 still RTBC.
Comment #24
David_Rothstein commentedCommitted #9 to 7.x - thanks!
I think the full patch could be backported if necessary (i.e. if it's actually causing problems with real translations), especially since it's an admin-facing string (https://www.drupal.org/node/1527558) - would just want to do it early in the release cycle.
But will leave it to someone to reopen for that only if it's causing an actual translation problem in its current state.