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

[Translator][TwigBridge] allow using trans() with a list of messages to use as fallbacks #35745

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 17, 2020

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

Alternative of #35636

In Twig:
{{ ["bar", "fallback"]|trans }}'

In PHP:

$translator = new AltTranslator($translator);
$translator->trans(['bar', 'fallback']);

A message is considered not translated when its translation is the same as its id.


$translator = $this->getTranslator();

foreach ($messages as $msg) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. This does not mean the message is not translated
  2. I'm not sure the $arguments, $domain, $locale will be the same in every situation

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. right now, passing an array is not allowed. So yes, it means the message is not translated, because we just defined it that way
  2. then you'd need another interface, which this PR doesn't aim to solve.

Copy link
Member

@lyrixx lyrixx Feb 17, 2020

Choose a reason for hiding this comment

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

  1. Oups, I commented on the wrong line :/ It should have been on the next line.
$msg !== $message = $translator->trans()

because we just defined it that way

ATM, this behavior is not documented

  1. OK

Copy link
Member Author

Choose a reason for hiding this comment

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

Explanation added in the docblock.

Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

in general, if the translator is type TranslatorBagInterface, we could do a better check.

i guess for translating purposes it doesnt matter too much (assuming the translation differs from its ID), but still ..conceptually we cannot break the fallback chain with a literal ID translation.

/**
* {@inheritdoc}
*
* @param string|string[]|null $id
Copy link
Contributor

Choose a reason for hiding this comment

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

why the null case?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it's already allowed in the main translator, there is no reason to be stricter here, especially when that means less code.

@nicolas-grekas
Copy link
Member Author

conceptually we cannot break the fallback chain with a literal ID translation.

this PR just did, with no practical drawback :)

Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

another thing, wont this try all fallback locales per message ID first? Instead of trying all message IDs in the current locale, and then try them all for a fallback locale.

Not sure what's the expected behavior, i tend to believe we should try all message IDs first before trying a fallback locale.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 17, 2020

I would expect the most specific id to have higher precedence, no matter the locale. Eg. I'm fine with the generic french version of some sentence when the Canadian-french is the first locale.

@lyrixx
Copy link
Member

lyrixx commented Feb 17, 2020

The implemented behavior here does not match the linked issue as spotted by @ro0NL

Let's say I have the following translation

message ID English French
contact.lost_object.url https://en.website.com/help/lost
contact.url https://en.website.com/help https://fr.website.com/aide

With the following code:

{{ ['contact.lost_object.url', 'contact.url']|trans }}

When the current locale is fr

It will display:

https://en.website.com/help/lost

instead of

https://fr.website.com/aide

The implementation in #35636 is correct. You can see the test suite.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 17, 2020

I can very easily construct another example where the expected precedence is the one implemented here. Any graph of possibilities has several ways to be linearized into a single list of possibilities, and there is a use case for every possible way.

We will have to favor one way vs another, and this preference will not fit everyone's needs.

We may just close for this reason actually.

@ro0NL
Copy link
Contributor

ro0NL commented Feb 17, 2020

i tend to favor translating all message IDs in the active locale first. Then, if still unavailable, try another round for a fallback locale.

Given the fallback locale may be a different language, i think it makes sense to try all message IDs in our own language first.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 17, 2020

OK, works for me. Signature-wise, I think this PR is what we need. On the behavior side (fallback order), I've no specific opinion.

I'm closing so that someone else can take over.

@nicolas-grekas nicolas-grekas deleted the trans-array branch February 17, 2020 15:39
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@walva
Copy link

walva commented Nov 26, 2022

I really like this feature and want to use it ! I presented my use case in #35745

@lyrixx
Copy link
Member

lyrixx commented Nov 28, 2022

@walva please, open a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants