Web Profiler logs missing messages when setting the translation domain to false

[tonynelson19]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no
Symfony version	3.2.1

If you use a false translation domain to disable a translation, the web profiler displays the string as missing a translation. This commonly happens inside forms when using the translation_domain and choices_translation_domain options, but can also happen when translating directly inside Twig.

The following can recreate the issue on a new Symfony project.

{{ "Hello World!"|trans({}, false) }}

This can be fixed by adding the following condition to the top of DataCollectorTranslator::collectMessage to prevent the web profiler from collecting the disabled translation.

if (false === $domain) {
    return;
}

The same fix can be added to the top of LoggingTranslator::log to prevent a "Translation not found." warning from be logged.

[xabbuh]

false is not a valid value for the translation domain (it must be a string). If you do not want a string to be translated at all, you should actually should not call trans() at all.

[tonynelson19]

According to New in Symfony 2.7: Form and Validator Updates:

The values of the choice_translation_domain option can be true (reuse the current translation domain), false (disable translation), null (uses the parent translation domain or the default domain) and a string which represents the exact translation domain to use.

[xabbuh]

That's when you are using the Form component, but your example uses the trans filter directly.

[tonynelson19]

My example was the simplest way to recreate the issue. I mentioned the form component in the description. No methods use the same underlying translator.

[xabbuh]

Well, I am not able to reproduce your issue using a form. Do you have a custom form theme?

[tonynelson19]

Symfony's theme works around this issue by wrapping all calls to trans() in the following logic:

{{ translation_domain is same as(false) ? label : label|trans({}, translation_domain) }}

This logic exists 9 times in the theme. Remembering to add it to custom form themes can be a challenge, and some third party or legacy themes might not apply the logic correctly. However, the logic could be removed entirely by applying my proposed changes.

Would you be more open to this change if the issue was labeled a DX improvement rather than a bug?

[xabbuh]

Even then, we couldn't make the change to the interface as that would mean a BC break. So, no, I do not see another solution than using form themes for this.

[tonynelson19]

How exactly is it a BC break?

[xabbuh]

Because all implementations of the interface would need to be updated to still work even when they get passed a boolean value instead of a string.

[tonynelson19]

This wouldn't break any Symfony implementations that I'm aware of.

If you're concerned about breaking third party implementations, would you like me to create a different issue about how there currently is not any logic to prevent users from using non-string values for domains when adding translations resources? The translation ID is cast to a string in some places, but aside from checking for a null domain, other scalar types are allowed.

[xabbuh]

The TranslationInterface is one of our extension points in the Translation component and its documentation is clear on that all implementations can expect the domain to be either a string or null: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Translation/TranslatorInterface.php#L26 So we have no way to influence what other implementations out there are doing inside trans().

[tonynelson19]

Which is why my request was to modify DataCollectorTranslator::collectMessage and LoggingTranslator::log, not TranslatorInterface::trans.

I just want to improve the developer experience.

[xabbuh]

But being able to pass false as the message domain to LoggingTranslator::log() means that you must have passed false to trans() before. This could be caught inside the LoggingTranslator before it forwards the trans() call to the decorated translator. However, to do that you must know upfront whether or not the current translator instance is a LoggingTranslator or another implementation of the TranslatorInterface. Currently, I do not see how we could solve that issue.

[tonynelson19]

Since the TranslationInterface's documentation makes it clear that all implementations can expect either a string or null, couldn't the LoggingTranslator and DataCollectorTranslator implementations safely ignore any domains that aren't either strings or null in their respective log and collectMessage methods, or should they throw an InvalidArgumentException?

[xabbuh]

The log() method is only called internally by the LoggingTranslator when one of its methods given by the interface is called. This still means that you will have to ensure that the translator you are using actually is the LoggingTranslator before calling any of its method with false as the message domain.

[tonynelson19]

Then shouldn't any implementations of TranslationInterface inside the Symfony\Component\Translation namespace throw an InvalidArgumentException if you call either trans() or transChoice() with false as the message domain?

Right now it seems like using a false domain is both not supported and yet not unsupported, allowing developers to use it due to lack of strict type checking.

[xabbuh]

Well, we could avoid logging if the domain is false. But I am not sure if that's worth the effort. If we did that change, you would never notice that you'd better update your form theme.

[javiereguiluz]

I agree with @tonynelson19 and I'm trying to fix this in #21726.

[curry684]

As the proposed fix did not get merged, nor a consensus on that it should be fixed, this issue should likely also be closed. If anything we now do have the option to make the interface "correct" in 4.x (or even 5.x) as we now have a base PHP version supporting nullable type hinting.

[xabbuh]

I am closing here as the dicsussion in the related PR didn't reach a consensus.