Skip to content

[Form][TwigBridge] Handle MoneyType with non UTF-8 charset #25167

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

Closed
wants to merge 1 commit into from
Closed

[Form][TwigBridge] Handle MoneyType with non UTF-8 charset #25167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@
<div class="input-group">
{% set append = money_pattern starts with '{{' %}
{% if not append %}
<span class="input-group-addon">{{ money_pattern|replace({ '{{ widget }}':''}) }}</span>
<span class="input-group-addon">{{ money_pattern|convert_encoding(_charset, 'UTF-8')|replace({ '{{ widget }}':''})|raw }}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think this should be |htmlentities
we should add the filter to FormExtension

Copy link
Contributor Author

@ro0NL ro0NL Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Will patch soonish.

Also templates are UTF8 encoded by default right? Technically we shouldnt have to do any encoding conversion at all here. Instead the rendered template as a whole should be converted depending on the kernel charset i guess. Let's not go there for now.

Implicitly meaning the linked issue is (technically) not supported? Anyway.. htmlentities will settle :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templates are UTF8 encoded by default right [...] template as a whole should be converted

That's not how Twig deals with charsets - so templates are ASCII that's the only thing we can tell from here.

{% endif %}
{{- block('form_widget_simple') -}}
{% if append %}
<span class="input-group-addon">{{ money_pattern|replace({ '{{ widget }}':''}) }}</span>
<span class="input-group-addon">{{ money_pattern|convert_encoding(_charset, 'UTF-8')|replace({ '{{ widget }}':''})|raw }}</span>
{% endif %}
</div>
{%- endblock money_widget %}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
{%- endblock integer_widget -%}

{%- block money_widget -%}
{{ money_pattern|replace({ '{{ widget }}': block('form_widget_simple') })|raw }}
{{ money_pattern|convert_encoding(_charset, 'UTF-8')|replace({ '{{ widget }}': block('form_widget_simple') })|raw }}
{%- endblock money_widget -%}

{%- block url_widget -%}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Form/Extension/Core/Type/MoneyType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function buildForm(FormBuilderInterface $builder, array $options)
*/
public function buildView(FormView $view, FormInterface $form, array $options)
{
$view->vars['money_pattern'] = self::getPattern($options['currency']);
$view->vars['money_pattern'] = htmlentities(self::getPattern($options['currency']), ENT_QUOTES | (defined('ENT_SUBSTITUTE') ? ENT_SUBSTITUTE : 0), 'UTF-8');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed, buildView does not output html, that's not the correct layer

}

/**
Expand Down Expand Up @@ -83,7 +83,7 @@ public function getName()
}

/**
* Returns the pattern for this locale.
* Returns the pattern for this locale in UTF-8.
*
* The pattern contains the placeholder "{{ widget }}" where the HTML tag should
* be inserted
Expand Down
8 changes: 3 additions & 5 deletions src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected function assertMatchesXpath($html, $expression, $count = 1)
try {
// Wrap in <root> node so we can load HTML with multiple tags at
// the top level
$dom->loadXML('<root>'.$html.'</root>');
$dom->loadHTML('<!DOCTYPE html><html><body>'.$html.'</body></html>');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using loadHTML enables HTML entities (Entity 'euro' not defined otherwise). It makes sense; we're in fact loading $html.

} catch (\Exception $e) {
$this->fail(sprintf(
"Failed loading HTML:\n\n%s\n\nError: %s",
Expand All @@ -71,17 +71,15 @@ protected function assertMatchesXpath($html, $expression, $count = 1)
));
}
$xpath = new \DOMXPath($dom);
$nodeList = $xpath->evaluate('/root'.$expression);
$nodeList = $xpath->evaluate('/html/body'.$expression);

if ($nodeList->length != $count) {
$dom->formatOutput = true;
$this->fail(sprintf(
"Failed asserting that \n\n%s\n\nmatches exactly %s. Matches %s in \n\n%s",
$expression,
1 == $count ? 'once' : $count.' times',
1 == $nodeList->length ? 'once' : $nodeList->length.' times',
// strip away <root> and </root>
substr($dom->saveHTML(), 6, -8)
$html
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatted output not worth the hassle IMHO. As saveHTML includes a doctype declaration either way.

));
} else {
$this->addToAssertionCount(1);
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/MoneyTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function testPassMoneyPatternToView()
$view = $this->factory->create(static::TESTED_TYPE)
->createView();

$this->assertSame('{{ widget }} €', $view->vars['money_pattern']);
$this->assertSame('{{ widget }} &euro;', $view->vars['money_pattern']);
}

public function testMoneyPatternWorksForYen()
Expand All @@ -43,7 +43,7 @@ public function testMoneyPatternWorksForYen()
$view = $this->factory->create(static::TESTED_TYPE, null, array('currency' => 'JPY'))
->createView();

$this->assertTrue((bool) strstr($view->vars['money_pattern'], '¥'));
$this->assertTrue((bool) strstr($view->vars['money_pattern'], '&yen;'));
}

// https://github.com/symfony/symfony/issues/5458
Expand All @@ -54,8 +54,8 @@ public function testPassDifferentPatternsForDifferentCurrencies()
$view1 = $this->factory->create(static::TESTED_TYPE, null, array('currency' => 'GBP'))->createView();
$view2 = $this->factory->create(static::TESTED_TYPE, null, array('currency' => 'EUR'))->createView();

$this->assertSame('{{ widget }} £', $view1->vars['money_pattern']);
$this->assertSame('{{ widget }} €', $view2->vars['money_pattern']);
$this->assertSame('{{ widget }} &pound;', $view1->vars['money_pattern']);
$this->assertSame('{{ widget }} &euro;', $view2->vars['money_pattern']);
}

public function testSubmitNull($expected = null, $norm = null, $view = null)
Expand Down