Here's the code (from actual git version, commerce.module line 513 in function commerce_currency_format()):
return trim(t('@code_before @negative@symbol_before@price @symbol_after @code_after', $replacements));
Any necessary formating already happened here since currencies define their own, it's already localized. The t() call here will save a string with only placeholders and is a useless usage of localization API.
Am I wrong?
If I'm not, a simple str_replace() or strtr() usage would be enough, and a lot faster, you could even use a basic simple string concat (even simpler, don't know if it's faster though).
Plus for consistency using non-breakable whitespace instead of normal whitespace between the amount and its symbol would avoid currency display to break and split over multiple lines (can happen if rendered in text or anywhere like on a small screen).
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | 1313154-currency-format-22.patch | 810 bytes | khiminrm |
| #4 | 1313154-currency-format-4.patch | 588 bytes | amateescu |
| #3 | 1313154-currency-format-3.patch | 588 bytes | mr.baileys |
| #2 | 1313154-currency-format-2.patch | 1.71 KB | amateescu |
| #1 | 1313154-currency-format.patch | 583 bytes | mr.baileys |
Comments
Comment #1
mr.baileysSeems like a valid point to me.
Comment #2
amateescu commentedI don't think
string_format()is a good idea, it was added in D7.8 and our minimun requirement is D7.2. Let's just emulate it: do acheck_plain()on our values and usestrtr().Comment #3
mr.baileysGood catch on format_string(), I didn't think to check whether it was available for earlier versions of D7.
I think that we can optimize the patch from #2 slightly though, by running the entire string through check_plain once, instead of doing so for each individual component. What do you think?
Comment #4
amateescu commentedI agree, micro-optimizations FTW! :) And let's do it before trim().
Comment #5
pounardNice, it took a long time for this issue to wake up, but thanks.
Comment #6
rszrama commentedThe thing we lose here that made t() useful is the ability for sites to specify different formats for the same currency by language. It would be a hack, to be sure, but you could for example use a country-specific format for the Euro in France vs. Germany if you tweaked the currency formatting string for the language. Any thoughts on this?
Comment #7
pounardThe localized string was huge, and not all variables are being used everytime we hit it, so my guess is that for site integrators it might be hell to edit such translation.
If I go to the translation admin page 1) I will never be able to find it there is no visible word in it, and 2) I really won't know what to do with that:
@code_before@code_spacer@negative@symbol_before@price@symbol_spacer@symbol_after@code_spacer@code_afterComment #8
pounardBut, as you said it, the ability to tune it can be good, but I would rather set up a per langage variable override, maybe, but not the translation system?
Comment #9
rszrama commentedYeah, we might be able to trim the fat from that function further and just define the default pattern in the currency info itself, using this function merely to drop in replacements. Ugh, but that would've been better from the get-go before we introduced these specific properties of currencies (code placement, symbol placement, etc.). Maybe that's better left for Commerce 2.x...
Comment #10
rszrama commentedTagging for review. Just wondering if it wouldn't be interesting to provide the format pattern in the currency info array... but perhaps that's overengineering the fix for the 1.x branch. Using an alternate currency formatting function would work just as well for now...
Comment #11
rszrama commentedLet's make one more change to the patch to accommodate pounard's suggestion that we use a non-breaking space instead of a regular space for the separators. This will likely entail a reversion to amateescu's previous iteration, since we can't wrap the whole thing in check_plain() and preserve the . The alternative would be to use a more permissive filter function, like filter_xss(), which may actually be better - no reason to not permit HTML in this output that I know of.
We'll need to update the default values for the separator tokens in commerce_currencies().
Comment #12
pounardThe only dark spot for the nbsp is in case you want this function to output a mail or a webservice result, or anything which is not HTML, maybe a parameter for that could be ok.
Comment #13
rszrama commentedAhh, hmm. True, perhaps we'd be better off committing this as is and wrapping them in the theme layer in a span with
white-space: nowrap.Comment #14
damien tournoud commentedWe want an Unicode non-breaking space here (on Mac: Command + Shift + Space).
is so 1999.Comment #15
pounardThe unicode non breakable white space is dangerous because hardly greppable (and even development tools such as firebug will make it impossible to see), I'd strongly suggest to use it only for non html content (and non plain text content as well, such as plain text mails). That's why I proposed to have the separator configurable via anew parameter for the function.
Comment #16
damien tournoud commentedNope, the Unicode non-breaking space is precisely what we need. If we want to make it more greppable we can use
"\xC2\xA0", but I don't really see the point thing mostly all the editors visually represent this character differently.Comment #17
pounardAs in there http://drupal.org/node/1232608 for example?
EDIT: Not exactly the same problem, but I search for it because non breakable unicode white space literally drove me insane, firebug does not interpret it and you cannot find it easily in generated browser source either. And you are forgetting plain text versions of mails (potentially involved too).
Comment #18
nicolas bouteille commentedHi,
I need a non-breakable space between the amount and the symbol.
I have used hook_commerce_currency_info_alter() and set 'symbol_spacer' to
' 'but it is printed in plain text. How am I supposed to achieve this ?Comment #19
nicolas bouteille commentedSwitching to support request because the thread is more than one year old so I was scared not to get any answer.
Comment #20
rszrama commentedEvery issue in this queue will eventually be addressed, no need to play with the status to get a response. What you're seeing is the result of text formatting I'm assuming and is not related to this issue / patch.
Comment #21
bojanz commentedSince #2888674: Regenerate the currency list from CLDR we are using non-breaking spaces.
Not using t() still makes sense.
Comment #22
khiminrm commentedComment #23
rszrama commentedCommitted.