8000 MoneyType renders incorrect symbol for currency € when serving pages in ISO-8859-15 · Issue #25135 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

MoneyType renders incorrect symbol for currency € when serving pages in ISO-8859-15 #25135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
nzurita opened this issue Nov 23, 2017 · 12 comments
Closed

MoneyType renders incorrect symbol for currency € when serving pages in ISO-8859-15 #25135

nzurita opened this issue Nov 23, 2017 · 12 comments
Labels
Bug Form Status: Needs Review

Comments

@nzurita
Copy link
nzurita commented Nov 23, 2017
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.9

When serving pages in charset ISO-8859-15 instead of default UTF-8 and rendering a form MoneyType, currency symbol (€ in my case) is shown in wrong codification (see screen captures).

imagen
imagen

As additional information, this is my code in AppKernel.php regarding charset:

/**
 * Get the framework charset
 *
 * @return string The charset
 */
public function getCharset()
{
  return 'ISO-8859-15';
}

And a link showing details of ISO-8859-15 charset, which supposedly has an euro (€) symbol. My twig template is also encoded in ISO-8859-15. A possible cause of this problem is that the source code file for money template is encoded in UTF-8, which is of course right.

I know I can override MoneyType template, but a correct solution, I think, is writing the euro symbol as the € html entity, which is charset independant.
@ro0NL
Copy link
Contributor
ro0NL commented Nov 24, 2017

I think the issue comes from NumberFormatter returning a UTF-8 encoded string. Which is not properly converted to ISO.

See https://3v4l.org/O5sTJ

echo htmlentities(iconv('UTF-8', 'ISO-8859-15', $pattern), \ENT_HTML5, 'ISO-8859-15');

should do.

@nzurita
Copy link
Author
nzurita commented Nov 24, 2017

Sorry, I need some context, do you mean I should use this in a template to override default rendering?

@ro0NL
Copy link
Contributor
ro0NL commented Nov 24, 2017

not sure yet. The issue seems to happen from SF (sure you can fix it somewhere in a override). Im not fully sure how to move forward, assuming we already do htmlspecialchars by default in twig against the current charset (in your case ISO-...), but that doesnt help with € (euro) char, as it's not part of the charset.

In general i think the steps for SF in MoneyType are roughly;

  • split utf encoded string (current logic)
  • utf decode all parts (if charset is not utf, assuming intl always returns utf strings)
  • htmlentitiy encode all parts against the current charset
  • mark the view variables as "safe" to avoid double encoding

@stof
Copy link
Member
stof commented Nov 24, 2017

What could be done is accounting for the fact that Intl returns UTF-8 things, and so converting it:

{{ foo|convert_encoding(_charset, 'UTF-8') }}

However, I suggest you to do it in a custom form theme instead of changing the form theme in Symfony. Applying iconv for all people actually using UTF-8 would have overhead (and UTF-8 is the default).

@ro0NL
Copy link
Contributor
ro0NL commented Nov 24, 2017 •
edited
Loading

we could convert on the fly no? if needed.

{% if _charset != 'UTF-8' %}
    {{ foo|convert_encoding(...) }}
{% endif %}

Did not 8000 know about convert_encoding 😅

@stof
Copy link
Member
stof commented Nov 24, 2017

@ro0NL I'm not sure having lots of if for the charset in form themes would be a good idea though, even if the condition is simple. Form rendering tends to make lots of calls to Twig.
I think non-UTF-8 charset is not that widespread nowadays.

@ro0NL
Copy link
Contributor
ro0NL commented Nov 24, 2017

we could do it from MoneyType =/

I think non-UTF-8 charset is not that widespread nowadays.

Tend to agree; but a fix might be nice / solving this issue (should we close?).

@nzurita
Copy link
Author
nzurita commented Nov 24, 2017

I think non-UTF-8 charset is not that widespread nowadays.

Sure it isn't, in my case, it's a new Symfony module over a legacy app, so we are forced to apply legacy charset

@nicolas-grekas
Copy link
Member

This looks like a bug to me. I would expect at least htmlentities($money_pattern, ENT_HTML5, 'UTF-8') to be called (not sure about convert_encoding, because that requires extra knowledge about the charset, which htmlentities doesn't)

@ro0NL ro0NL mentioned this issue Nov 26, 2017
Closed
@derrabus
Copy link
Member
derrabus commented Nov 27, 2017 •
edited
Loading

that doesnt help with € (euro) char, as it's not part of the charset

Actually, ISO-8859-15 has the Euro symbol as 0xA4. So a simple charset conversion should be enough (in this case).

@ro0NL
Copy link
Contributor
ro0NL commented Nov 27, 2017

@derrabus you're right, however ISO-8859-1 hasnt. I believe htmlentities fits all cases.

@derrabus
Copy link
Member

@ro0NL

you're right, however ISO-8859-1 hasnt.

ISO-8859-1 is deprecated and 8859-15 is supposed to replace it. Both charsets are similar enough, so you can read ISO 5559-1 encoded data and treat it as ISO-8859-15. The difference is that eight character codes have been reassigned.

I believe htmlentities fits all cases.

I agree. If you can switch to HTML entities in a non-breaking way, go for it.

@ro0NL ro0NL mentioned this issue Mar 24, 2018
Closed
fabpot added a commit that referenced this issue Mar 29, 2018
@fabpot
bug #26663 [TwigBridge] Fix rendering of currency by MoneyType (ro0NL)
4243db5 
This PR was squashed before being merged into the 2.7 branch (closes #26663).

Discussion
----------

[TwigBridge] Fix rendering of currency by MoneyType

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | todo
| Fixed tickets | #25135
| License       | MIT
| Doc PR        | -

Split from #25167 as suggested by @nicolas-grekas to see if this can be reasonably fixed on 2.7, the right way using this itsy-bitsy new feature.

#25167 still contains some valuable changes regarding tests. Ill continue either one PR depending on the target branch / proposed fix.

Commits
-------

a3a2ff0 [TwigBridge] Fix rendering of currency by MoneyType
@fabpot fabpot closed this as completed Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Form Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@fabpot @nicolas-grekas @stof @nzurita @ro0NL @derrabus @xabbuh @carsonbot
0