Drupal.formatPlural(1, '1 comment', '@count comments'); returns '1 comments'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi’s picture

Status: Active » Needs review
FileSize
458 bytes
ufku’s picture

Status: Needs review » Reviewed & tested by the community

Drupal.formatString converts args['@count'] to string on which the strict equality check fails.
No brainer. Ready to commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: formatPlural-2397225-1.patch, failed testing.

Chi’s picture

Status: Needs work » Needs review
FileSize
523 bytes

reroll

nod_’s picture

Status: Needs review » Needs work

We only use strict equals, if there is a conversion somewhere we either remove it or check for the appropriate type.

Chi’s picture

Status: Needs work » Needs review
FileSize
534 bytes

We only use strict equals

I think it is no good in this particular case. We do themselves what javascript can do for us.

ufku’s picture

Drupal.formatString alters the passed args object by applying sanitizers. For strict equality we need to keep the args intact.

nod_’s picture

Status: Needs review » Needs work

@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.

ufku’s picture

after calling Drupal.formatString I would expect everything to be escaped

You 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.

nod_’s picture

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 into processedArgs, or something a bit more explicit than 'new'?

ufku’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

Renamed to processedArgs

nod_’s picture

Status: Needs review » Reviewed & tested by the community

It works now, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

alexpott’s picture

Issue tags: +JavaScript

Tagging in retrospect.

  • alexpott committed 7f5bc4e on 8.0.x
    Issue #2397225 by Chi, ufku: Drupal.formatPlural  does not work
    

Status: Fixed » Closed (fixed)

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