Skip to content

[Translation] Handle the translation of empty strings #48833

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

Jump to bottom
Merged
merged 1 commit into from
Feb 23, 2023
Merged

[Translation] Handle the translation of empty strings #48833

merged 1 commit into from
Feb 23, 2023

Conversation

javiereguiluz
Copy link
Member

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

In some apps, you might use an expression as the value of some TranslatableMessage object. The result of this expression can result in using an empty string as the value of the that object:

new TranslatableMessage($someCondition ? $someObject->someMethod() : '')

The issue is that Symfony will report that empty string in the list of "missing translations" (like in the first row of this image):

I think this is a bug and an empty string should just be output "as is" without reporting it as missing.

What do you think? Is this truly a bug? Would it be a new feature for 6.3? The current behavior is correct and we should close without merging? Thanks.

@javiereguiluz javiereguiluz added this to the 5.4 milestone Dec 30, 2022
@carsonbot
Copy link

Hey!

I think @rvanlaak has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@maxbeckers
Copy link
Contributor

maxbeckers commented Jan 10, 2023 •
edited
Loading

Hi @javiereguiluz,

i can't reproduce the problem. When I translate {{ 'invalid'|trans }}, then i get one message for missing Translation. When I translate {{ ''|trans }} then there is no message.

Tried with a clean symfony 5.4 and 6.1 environment.

UPDATE
I can reproduce with setting this in controller ['test' => new TranslatableMessage(''),] and then {{ test|trans }}. Sorry, didn't read the issue correlctly. Then I'm fine with the fix :) And yes for me it's as well a kind of bugfix, so I'm fine with 5.4.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 11, 2023 •
edited
Loading

Why don't you use this code instead?

$someCondition ? new TranslatableMessage($someObject->someMethod()) : ''

It would make more sense to me. Why wrap the empty string in a translatable when it's not translatable content?

@javiereguiluz
Copy link
Member Author

javiereguiluz commented Jan 17, 2023 •
edited
Loading

Looking at Twig Bridge trans() function, I think there's an inconsistency:

symfony/src/Symfony/Bridge/Twig/Extension/TranslationExtension.php

Lines 99 to 122 in 1f7bc10

public function trans(string|\Stringable|TranslatableInterface|null $message, array|string $arguments = [], string $domain = null, string $locale = null, int $count = null): string
{
if ($message instanceof TranslatableInterface) {
if ([] !== $arguments && !\is_string($arguments)) {
throw new \TypeError(sprintf('Argument 2 passed to "%s()" must be a locale passed as a string when the message is a "%s", "%s" given.', __METHOD__, TranslatableInterface::class, get_debug_type($arguments)));
}
return $message->trans($this->getTranslator(), $locale ?? (\is_string($arguments) ? $arguments : null));
}
if (!\is_array($arguments)) {
throw new \TypeError(sprintf('Unless the message is a "%s", argument 2 passed to "%s()" must be an array of parameters, "%s" given.', TranslatableInterface::class, __METHOD__, get_debug_type($arguments)));
}
if ('' === $message = (string) $message) {
return '';
}
if (null !== $count) {
$arguments['%count%'] = $count;
}
return $this->getTranslator()->trans($message, $arguments, $domain, $locale);
}

The behavior is different:

(1) If we pass a string:

  • If it's empty, return ''
  • If it's not empty, translate it

(2) It we pass a TranslatableInterface object:

  • Translate it in all cases, even if it's an empty string

That's why we don't get a "missing translation" warning when doing ''|trans but we get it when doing t('')|trans

Would you agree to update trans() code to also return '' when TranslatableInterface content is an empty string?

@javiereguiluz
Copy link
Member Author

I've changed the proposed solution completely. If you can, please review it again. Thanks.

@nicolas-grekas
Copy link
Member

Thank you @javiereguiluz.

@nicolas-grekas nicolas-grekas merged commit cfda9b4 into symfony:5.4 Feb 23, 2023
@javiereguiluz javiereguiluz deleted the translatable_empty branch February 23, 2023 15:37
@javiereguiluz javiereguiluz mentioned this pull request Feb 25, 2023
Merged
This was referenced Feb 28, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@welcoMattic welcoMattic welcoMattic approved these changes

@maxbeckers maxbeckers maxbeckers approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed Translation
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@javiereguiluz @carsonbot @maxbeckers @nicolas-grekas @welcoMattic