Movatterモバイル変換

src/Symfony/Bridge/Twig/Extension/HtmlEntitiesExtension.php Outdated

		*
		* @author Roland Franssen <franssen.roland@gmail.com>
		*/
		class HtmlEntitiesExtensionextends AbstractExtension

Copy link

Member

Choose a reason for hiding this comment

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

instead of a dedicated exception, can't we make it in an existing one?

Copy link

ContributorAuthor

ro0NLMar 25, 2018•
edited
Loading

Choose a reason for hiding this comment

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

FormExtension sounds good yes, and to really be conflict-safe id even suggestform_html_entities

Copy link

Member

Choose a reason for hiding this comment

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

lgtm

Copy link

Member

Choose a reason for hiding this comment

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

the conflict-safe id is solved by not providing a legacy extension name at all (we don't have BC issues with this extension)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

but we still need a filter name.html[_]entities has higher chance to be already defined in userland, hencevar|form_html_entities.

nicolas-grekas added this to the2.7 milestone

Mar 25, 2018

nicolas-grekas changed the title~~[TwigBridge] Add html_entities filter~~[TwigBridge] Add html_entities filter to fix rendering of currency by MoneyType

Mar 25, 2018

Copy link

Member

nicolas-grekas commentedMar 25, 2018

Tests are red (and lowest deps need to be updated.)

Copy link

Member

javiereguiluz commentedMar 27, 2018

I consider this solution "too big" for the tiny use case being solved. Can't this be solved using some PHP code somewhere in Symfony Forms? Roland suggested this:htmlentities(self::getPattern($options['currency']), ENT_QUOTES | (defined('ENT_SUBSTITUTE') ? ENT_SUBSTITUTE : 0), 'UTF-8') but Nico said that the place was wrong (#25167 (comment)). Could some Form expert help us here? Thanks!

stof reviewed

Mar 27, 2018

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

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig Outdated

		<divclass="input-group">
		{%ifprepend %}
		<spanclass="input-group-addon">{{money_pattern\|replace({'{{ widget }}':''}) }}</span>
		<spanclass="input-group-addon">{{money_pattern\|html_entities\|replace({'{{ widget }}':''})\|raw }}</span>

Copy link

Member

stofMar 27, 2018

Choose a reason for hiding this comment

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

replacing{{ widget }} should be done before encoding entities IMO.

Copy link

ContributorAuthor

ro0NLMar 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

actually no.. 🤔 that would display a HTML literal as{{widget}} contains the<input>

Copy link

Member

stofMar 27, 2018

Choose a reason for hiding this comment

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

What do you mean ?{{ widget }} is replaced by an empty string here

Copy link

ContributorAuthor

ro0NLMar 27, 2018

Choose a reason for hiding this comment

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

confused the templates :) now updated.

src/Symfony/Bridge/Twig/Extension/HtmlEntitiesExtension.php Outdated


		publicfunctionencode($input)
		{
		returnhtmlentities($input,ENT_QUOTES \| (defined('ENT_SUBSTITUTE') ?ENT_SUBSTITUTE :0),'UTF-8');

Copy link

Member

stofMar 27, 2018

Choose a reason for hiding this comment

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

Twig does not guarantee it always runs in UTF-8. The charset should be handled properly.

src/Symfony/Bridge/Twig/Extension/HtmlEntitiesExtension.php Outdated

		*/
		publicfunctiongetName()
		{
		return'html_entities';

Copy link

Member

stofMar 27, 2018

Choose a reason for hiding this comment

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

not needed. The versions we support don't care about the name except for BC reasons (in latest 1.x), and we don't need to provide a BC layer for the name-based access for a new extension.

Copy link

ContributorAuthor

ro0NL commentedMar 27, 2018•
edited
Loading

@javiereguiluz

I consider this solution "too big" for the tiny use case being solved.

I understand your point, but then again, to me anything related to escaping, raw vars, encoding etc. is serious topic. I prefer the twig filter to properly solve this on the right level, the templating level. Doing it beforehand in PHP / ignoring the issue feels weird.

To me the fix is just OK, this is really about 2.7 vs. master.

ro0NL added4 commits

March 27, 2018 18:29

[TwigBridge] Add html_entities filter

0a94383

optim

95218e1

php templates

2c692c3

tests

f128bcf

ro0NL mentioned this pull request

Mar 27, 2018

Closed

favor consistency

3887e49

nicolas-grekas changed the title~~[TwigBridge] Add html_entities filter to fix rendering of currency by MoneyType~~[TwigBridge] Fix rendering of currency by MoneyType

Mar 27, 2018

stof requested changes

src/Symfony/Bridge/Twig/Extension/FormExtension.php Outdated

		publicfunctionencodeCurrency(Environment$environment,$text)
		{
		if ('UTF-8' ===$charset =$environment->getCharset()) {
		returnhtmlspecialchars($text,ENT_QUOTES \| (defined('ENT_SUBSTITUTE') ?ENT_SUBSTITUTE :0),'UTF-8');

Copy link

Member

stofMar 28, 2018

Choose a reason for hiding this comment

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

please use\defined, so that OPCache can resolve this at compile-time for recent PHP versions as it will know it is defined.

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig Outdated

		<divclass="input-group">
		{%ifprepend %}
		<spanclass="input-group-addon">{{money_pattern\|replace({'{{ widget }}':''}) }}</span>
		<spanclass="input-group-addon">{{money_pattern\|form_encode_currency\|replace({'{{ widget }}':''})\|raw }}</span>

Copy link

Member

stofMar 28, 2018

Choose a reason for hiding this comment

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

the replacement should be done first, which will avoid the need for the|raw filter.

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig Outdated

		{{-block('form_widget_simple') -}}
		{%ifappend %}
		<spanclass="input-group-addon">{{money_pattern\|replace({'{{ widget }}':''}) }}</span>
		<spanclass="input-group-addon">{{money_pattern\|form_encode_currency\|replace({'{{ widget }}':''})\|raw }}</span>

Copy link

Member

stofMar 28, 2018

Choose a reason for hiding this comment

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

same here

src/Symfony/Component/Form/Tests/Extension/Core/Type/MoneyTypeTest.php Outdated

		->createView();

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

Copy link

Member

stofMar 28, 2018

Choose a reason for hiding this comment

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

These test changes look suspicious to me

Copy link

ContributorAuthor

ro0NLMar 28, 2018•
edited
Loading

Choose a reason for hiding this comment

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

yeah we need 2 tests for utf+iso. The entity is a direct effect of using htmlentities, and what this topic is about: euro sign not being available in ISO (hence we use entities).

carsonbot added Status: Needs Work and removed Status: Needs Review labels

revise

768d1e2

Copy link

ContributorAuthor

ro0NL commentedMar 28, 2018•
edited
Loading

apart from tests; this seems the proper fix. As discussed with@nicolas-grekas to move widget replacement to extension level, making the template concise.

functional-wise, ive tested both ISO+UTF on 2.7 without any problems.

ro0NL added4 commits

March 28, 2018 12:39

revision round 2

9495826

revert form type tests

a9e9081

first tests

b078f7e

fix tests

73d5602

Copy link

ContributorAuthor

ro0NL commentedMar 28, 2018•
edited
Loading

HHVM strikes again :(https://3v4l.org/NCWh2

ro0NL added2 commits

March 28, 2018 13:58

for hhvm

ee50318

raise deps=low

1182c69

ro0NL commented

src/Symfony/Bridge/Twig/Extension/FormExtension.php

Copy link

ContributorAuthor

ro0NL left a comment

Choose a reason for hiding this comment

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

Now ready. Yay :}

		publicfunctionencodeCurrency(Environment$environment,$text,$widget ='')
		{
		if ('UTF-8' ===$charset =$environment->getCharset()) {
		$text =htmlspecialchars($text,ENT_QUOTES \| (\defined('ENT_SUBSTITUTE') ?ENT_SUBSTITUTE :0),'UTF-8');

Copy link

ContributorAuthor

ro0NLMar 28, 2018

Choose a reason for hiding this comment

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

ENT_QUOTES | ENT_SUBSTITUTE as of 3.4 instead

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig

		<divclass="input-group">
		{%ifprepend %}
		<spanclass="input-group-addon">{{money_pattern\|replace({'{{ widget }}':''}) }}</span>
		<spanclass="input-group-addon">{{money_pattern\|form_encode_currency }}</span>

Copy link

ContributorAuthor

ro0NLMar 28, 2018

Choose a reason for hiding this comment

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

bootstrap_base_layout.html.twig + bootstrap_4_layout.html.twig as of 3.4 instead

src/Symfony/Bundle/FrameworkBundle/composer.json

		"symfony/security-csrf":"~2.6",
		"symfony/stopwatch":"~2.3",
		"symfony/templating":"~2.1",
		"symfony/templating":"~2.7",

Copy link

ContributorAuthor

ro0NLMar 28, 2018

Choose a reason for hiding this comment

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

perhaps a conditional test is better, let me know.

nicolas-grekas removed the Feature label

nicolas-grekas reviewed

src/Symfony/Bridge/Twig/Extension/FormExtension.php

		/**
		* @internal
		*/
		publicfunctionencodeCurrency(Environment$environment,$text,$widget ='')

Copy link

Member

nicolas-grekasMar 28, 2018

Choose a reason for hiding this comment

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

$currency instead of $text, and mandatory $widget

src/Symfony/Bridge/Twig/Extension/FormExtension.php

		}else {
		$text =htmlentities($text,ENT_QUOTES \| (\defined('ENT_SUBSTITUTE') ?ENT_SUBSTITUTE :0),'UTF-8');
		$text =iconv('UTF-8',$charset,$text);
		$widget =iconv('UTF-8',$charset,$widget);

Copy link

Member

nicolas-grekasMar 28, 2018

Choose a reason for hiding this comment

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

Are we sure $widget is always in UTF-8? This looks strange to me, because that would mean this is also bugged. Is it?

Copy link

ContributorAuthor

ro0NLMar 28, 2018•
edited
Loading

Choose a reason for hiding this comment

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

i think we can assume we get raw html in UTF from the form component. Here we include it in a new safe html value concerning the target charset, usinghtmlentities instead ofhtmlspecialchars for escaping.

In general we are consistent with regular escaped output (ie. also in target charset) fromtwig_escape_filter.

Copy link

ContributorAuthor

ro0NLMar 28, 2018

Choose a reason for hiding this comment

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

At least we treated this as UTF before also (result fromblock() in twig).

carsonbot added Status: Reviewed and removed Status: Needs Work labels

nicolas-grekas approved these changes

fabpot approved these changes

Copy link

Member

fabpot commentedMar 29, 2018

Thank you@ro0NL.

fabpot added a commit that referenced this pull request

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 closed this