Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Drupal.formatPlural(1, '1 comment', '@count comments');
returns '1 comments'.
Comment | File | Size | Author |
---|---|---|---|
#11 | formatPlural-2397225-11.patch | 1.12 KB | ufku |
Comments
Comment #1
Chi CreditAttribution: Chi commentedComment #2
ufku CreditAttribution: ufku commentedDrupal.formatString converts
args['@count']
to string on which the strict equality check fails.No brainer. Ready to commit.
Comment #4
Chi CreditAttribution: Chi commentedreroll
Comment #5
nod_We only use strict equals, if there is a conversion somewhere we either remove it or check for the appropriate type.
Comment #6
Chi CreditAttribution: Chi commentedI think it is no good in this particular case. We do themselves what javascript can do for us.
Comment #7
ufku CreditAttribution: ufku commentedDrupal.formatString alters the passed args object by applying sanitizers. For strict equality we need to keep the args intact.
Comment #8
nod_@Chi, sorry should have explained more. We have coding standards for javascript, we also have an eslint configuration. Our configuration doesn't allow "sloppy equals", there is no reason to relax our standards for this.
Since we know Drupal.t() will transform all args to string, I would rather do
if (args['@count'] !== '1')
, add a comment before to explain it comes from Drupal.t() escaping the value if you feel an explanation is needed.Making a new object with the arg values is a little dangerous, after calling Drupal.formatString I would expect everything to be escaped, here we end up with unsanitized values in the original arg object, and the safe values can't be accessed outside formatString.
Comment #9
ufku CreditAttribution: ufku commentedYou shouldn't expect that. Drupal.formatString is not supposed to alter the given args. There should be another method for it, something like Drupal.sanitizeArgs.
Comment #10
nod_Yeah you're right, that was a poor attempt at justifying a one line patch instead of a proper solution.
Can you just rename
newArgs
intoprocessedArgs
, or something a bit more explicit than 'new'?Comment #11
ufku CreditAttribution: ufku commentedRenamed to
processedArgs
Comment #12
nod_It works now, thanks!
Comment #13
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 7f5bc4e and pushed to 8.0.x. Thanks!
Comment #14
alexpottTagging in retrospect.