Navigation Menu

Skip to content
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.

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

Allow disabling label translation #1184

Merged
merged 1 commit into from Apr 24, 2017

Conversation

core23
Copy link
Contributor

@core23 core23 commented Aug 7, 2016

Adopts the symfony mechanism to disable the translation of form labels.

@core23 core23 force-pushed the patch/disable-label-translation branch from c9e781c to 85aeaba Compare August 7, 2016 11:33
@corphi
Copy link
Contributor

corphi commented Aug 18, 2016

We use Doctrine embeddables for translation of user-generated strings. To actually translate these, they need to be passed to the trans filter, even in the false case. By swapping the translation extension, you get the desired behaviour in all of your templates, not just this special case.

parameters:
    twig.extension.trans.class: My\Twig\TranslationExtension
<?php

namespace My\Twig;

use Symfony\Bridge\Twig\Extension\TranslationExtension as BaseExtension;

class TranslationExtension extends BaseExtension
{
    public function trans($message, array $arguments = [], $domain = null, $locale = null)
    {
        if (false === $domain) {
            return $message;
        }

        return parent::trans($message, $arguments, $domain, $locale);
    }
}

@phiamo
Copy link
Owner

phiamo commented Feb 3, 2017

@corphi does this mean the PR is valid ?

@corphi
Copy link
Contributor

corphi commented Feb 5, 2017

TL;DR: Please merge it.

Well. It’s probably a good idea to handle form translations in the same way that Symfony does it - and the PR accomplishes just that.

The core issue here is that the Symfony form component adds the special domain false to disable translation. This case is not covered by the Twig extension which just forwards the call to the component. Then, the domain value false ends up here and is converted to 0. The entry probably doesn’t exist and the message is thus not translated. In dev mode, you reap a missing translation error for that. Symfony addresses this by eliminating the false case before applying the trans filter, nine times in form_div_layout.html.twig alone. Handling a false domain was introduced during the 2.7 cycle and originally targeted labels of expanded choices in ChoiceType. But it works for regular labels too, as they are using the same code for rendering. This behaviour is not documented, though - translation_domain is supposed to be a string. Only choice_translation_domain explicitly allows false.

In MopaBootstrapBundle on the other hand, this case is (still) not handled specially; all labels are passed to the trans filter without checking them first. That causes error messages in documented use cases and is a bug.

My workaround also allows disabling translation outside of forms; and we leverage it to pick the right translation for objects storing multiple languages. The workaround will be broken by the fix. Both translation and this hack are somewhat out of the scope of the bundle. A form type extension to translate labels when constructing the FormView does the trick nicely.

(As a side note: Translation in forms is generally a mess. The symptoms are usually treated by using special translation domains. The root cause is that child labels are translated by the child, not the parent. And don’t get me started on EntityType.)

@core23
Copy link
Contributor Author

core23 commented Apr 24, 2017

@phiamo Can you please review this?

@phiamo phiamo merged commit aef44d3 into phiamo:master Apr 24, 2017
@phiamo
Copy link
Owner

phiamo commented Apr 24, 2017

thanks a lot!

@core23 core23 deleted the patch/disable-label-translation branch April 25, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants