[2.7][Form][choice] added choice_translation_domain to avoid trans options. #13651

aitboudad · 2015-02-10T20:59:36Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Fixed tickets	~
Tests pass?	yes
License	MIT

see #12941 (comment)

aitboudad · 2015-02-10T21:41:09Z

https://blackfire.io/profiles/compare/93b74379-ba97-40b7-9153-14c1cba44b3e/graph

Tobion · 2015-02-10T23:24:23Z

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

@@ -79,7 +79,7 @@
                {{- block('choice_widget_options') -}}
            </optgroup>
        {%- else -%}
-            <option value="{{ choice.value }}"{% if choice is selectedchoice(value) %} selected="selected"{% endif %}>{{ choice.label|trans({}, translation_domain) }}</option>
+            <option value="{{ choice.value }}"{% if choice is selectedchoice(value) %} selected="selected"{% endif %}>{{ trans_options is sameas(false) ? choice.label:choice.label|trans({}, translation_domain) }}</option>


missing space around :

Tobion · 2015-02-10T23:31:17Z

I think this change makes sense because database entries are unlikely to be translated. And if they you would translate them, then not with the translator component but instead also read them from the database when fetching.

tadcka · 2015-02-11T08:01:18Z

+1

egils · 2015-02-11T08:14:07Z

👍 @Tobion, my thoughts exactly

Tobion · 2015-02-11T09:36:34Z

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

@@ -231,6 +232,7 @@ public function configureOptions(OptionsResolver $resolver)
            // is manually set to an object.
            // See https://github.com/symfony/symfony/pull/5582
            'data_class' => null,
+            'trans_options' => true,


i think it should be named translate_choices

we don't abbreviate option names AFAIK, e.g. preferred

choices is also the term in the related option

Tobion · 2015-02-11T09:52:49Z

As the default for the Doctrine type changed to not translating, it's a bc break. As such it should be documented in the upgrade file.

raziel057 · 2015-02-11T10:27:37Z

+1

aitboudad · 2015-02-11T10:55:46Z

@Tobion changes has been made.

aitboudad · 2015-02-11T12:02:16Z

FYI the CountryType, CurrencyType, LanguageType and LocaleType use Intl component so I changed to not use trans.

Tobion · 2015-02-11T12:17:43Z

src/Symfony/Component/Form/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 2.7.0
 -----

+ * added options "translate_choices" to ChoiceType.


option singular

Tobion · 2015-02-11T14:26:36Z

I think the TimezoneType should disable translation as well. You don't want to manually translate all the timezone identifiers. Actually it probably should translate internally with http://php.net/manual/en/intltimezone.getdisplayname.php but that's a different story. Created: #13662

Tobion · 2015-02-11T19:13:30Z

Please also update the php template in https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/choice_widget_options.html.php and you need to update https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/composer.json#L43 and https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bridge/Twig/composer.json#L24 to ~2.7

Tobion · 2015-02-16T22:02:54Z

Actually, I just found #7611 and #12148 (comment)
So I think it makes more sense to implement this as part of choice_translation_domain (bool|closure) which when false or the closure returns false, then does not translate the given choice.

fabpot · 2015-03-24T08:23:09Z

Is this one ready to be merged?

Tobion · 2015-03-24T12:30:54Z

In theory it would be ready because it works and implementation is ok. But I think the idea expressed in #12148 (comment) is the better solution. But this would require @webmozart or someone else to finish #12148.
So if we would merge this PR here, there is a high chance that we would deprecate the new option again as soon as #12148 is finished. And thus involve maiting BC and documentation confusion. So I'd prefer to wait for #12148 and then rebase this one.

aitboudad · 2015-03-24T12:40:04Z

@Tobion What's about renaming this option to choice_translation_domain (bool) ? so this won't involve a BC when adding (closure) feature ?

aitboudad · 2015-04-01T14:14:24Z

ping @symfony/deciders

stof · 2015-04-01T14:14:39Z

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

+
+        if ($view->parent) {
+            if (null === $choiceTranslationDomain) {
+                $choiceTranslationDomain = $view->parent->vars['translation_domain'];


it should use the translation domain of the current form when set to null, not of the parent one (the parent value is read for the translation_domain inheritance

that is already done in the lazy option. but if both translation_domain and choice_translation_domain is null, it must read it from the parent. otherwise it would be a bc break.

No, it is not done if the user is passing null in the option

if you pass null, you need to read it from translation_domain (and do it after it has itself be inherited eventually)

@stof if I remove this test fail in https://github.com/aitboudad/symfony/blob/form_entity_trans/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php#L1366

I'm not saying it should be removed entirely. I'm sayign the current implementation is buggy.

Try to set the choice_translation_domain explicitly with a non-inherited translation domain.

you should not inherit the choice_translation_domain from the parent translation_domain. you should inherit it from the current translation domain (be it inherited or no)

I see, the problem is when passing null explicitly.

aitboudad · 2015-04-02T07:14:29Z

Doc issue: symfony/symfony-docs#5134

aitboudad · 2015-04-03T20:24:56Z

ping @fabpot

xabbuh · 2015-04-03T20:56:37Z

Is it intended that the Twig template uses the label while the PHP templates uses the index in the optgroup element (it has been this way before, but it looks odd to me)?

xabbuh · 2015-04-03T21:00:11Z

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

+            if (null === $choiceTranslationDomain) {
+                $choiceTranslationDomain = $view->vars['translation_domain'];
+            }
+        }


We can use one if here:

if ($view->parent && null === $choiceTranslationDomain) { // ... }

xabbuh · 2015-04-03T21:02:08Z

And should we also document in the upgrade file that you need to update your custom form themes too?

aitboudad · 2015-04-03T22:17:04Z

@xabbuh

upgrade done
for optgroup I renamed $index --> $group_label

fabpot · 2015-04-04T05:59:21Z

👍

… avoid trans options. (aitboudad) This PR was merged into the 2.7 branch. Discussion ---------- [2.7][Form][choice] added choice_translation_domain to avoid trans options. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | ~ | Tests pass? | yes | License | MIT see #12941 (comment) Commits ------- 5a33c2c [Form][choice] added choice_translation_domain to avoid trans options.

aitboudad · 2015-04-04T09:14:13Z

I merged this PR as I think it's part of translation.

Tobion · 2015-04-04T09:27:59Z

lol I guess I could have merged it as well then because it uses the Options Resolver. ;)

aitboudad · 2015-04-04T09:31:31Z

I really appreciate your help on this PR :)
Thank you @Tobion

lsmith77 · 2015-12-09T07:20:16Z

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

@@ -74,13 +74,13 @@
 {%- block choice_widget_options -%}
    {% for group_label, choice in options %}
        {%- if choice is iterable -%}
-            <optgroup label="{{ group_label|trans({}, translation_domain) }}">
+            <optgroup label="{{ choice_translation_domain is sameas(false) ? group_label : group_label|trans({}, choice_translation_domain) }}">


just for the record .. this change breaks things for users of earlier Form component versions. maybe we should therefore adjust the composer dependency when we do such changes in the future

aitboudad force-pushed the form_entity_trans branch from 43ce6b9 to a5fb3ec Compare February 10, 2015 21:00

Tobion reviewed Feb 10, 2015
View reviewed changes

aitboudad force-pushed the form_entity_trans branch from a5fb3ec to b0b0792 Compare February 10, 2015 23:27

Tobion reviewed Feb 11, 2015
View reviewed changes

aitboudad force-pushed the form_entity_trans branch from b0b0792 to 06082b2 Compare February 11, 2015 10:06

aitboudad force-pushed the form_entity_trans branch from 06082b2 to 3eff033 Compare February 11, 2015 10:53

Tobion reviewed Feb 11, 2015
View reviewed changes

aitboudad force-pushed the form_entity_trans branch from df5fcf2 to 4b42447 Compare February 11, 2015 12:37

aitboudad force-pushed the form_entity_trans branch 5 times, most recently from 64d38f1 to 7904f74 Compare February 14, 2015 07:32

fabpot added the Form label Feb 25, 2015

aitboudad force-pushed the form_entity_trans branch from 7904f74 to 0a90cf8 Compare March 16, 2015 09:52

aitboudad changed the title ~~[Form][choice] avoid trans options for entity type.~~ [2.7][Form][choice] added choice_translation_domain to avoid trans options. Apr 1, 2015

stof reviewed Apr 1, 2015
View reviewed changes

aitboudad force-pushed the form_entity_trans branch 2 times, most recently from a514d4f to 6c8f624 Compare April 2, 2015 07:07

aitboudad mentioned this pull request Apr 2, 2015

[2.7][Form][choice] add doc about the new option: choice_translation_domain symfony/symfony-docs#5134

Closed

aitboudad force-pushed the form_entity_trans branch from 6c8f624 to 0a4f1be Compare April 3, 2015 18:48

xabbuh reviewed Apr 3, 2015
View reviewed changes

aitboudad force-pushed the form_entity_trans branch 2 times, most recently from 8086358 to ddfa468 Compare April 3, 2015 22:12

[Form][choice] added choice_translation_domain to avoid trans options.

5a33c2c

aitboudad force-pushed the form_entity_trans branch from ddfa468 to 5a33c2c Compare April 3, 2015 22:14

aitboudad merged commit 5a33c2c into symfony:2.7 Apr 4, 2015

aitboudad deleted the form_entity_trans branch April 6, 2015 14:19

This was referenced Apr 6, 2015

Controllers in Symfony 2.6.0 slower than 2.5.x? #12790

Closed

Some ideas to improve performance of the Translator component #13948

Closed

Tobion mentioned this pull request Sep 30, 2015

[Translation][Form] Do not translate placeholder and/or empty_value when translation_domain is false #16016

Closed

lsmith77 reviewed Dec 9, 2015
View reviewed changes

corphi mentioned this pull request Feb 5, 2017

Allow disabling label translation phiamo/MopaBootstrapBundle#1184

Merged

Uh oh!

[2.7][Form][choice] added choice_translation_domain to avoid trans options. #13651

[2.7][Form][choice] added choice_translation_domain to avoid trans options. #13651

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!