[Translation] Revert inlining fallback catalogues as it might cause inconsistent results when a cache is used #14315
Conversation
|
I don't think #14318 is a solution. Just extend the test to also check the fallback catalogue and you'll see that the problem has only been shifted there. |
|
I used the test, isn't it? |
|
Whenever you remove messages from the catalogue (currently upon caching), they will later on be missing. So I think this cannot be fixed 😦 I'd suggest we focus on caching and loading smaller chunks instead, for example only a single domain and locale at a time. And also the #13986 attempt (however implemented) would gain so much that merging fallbacks might no longer be necessary. |
|
@mpdude Is there a use case for the last commit ? |
|
I cannot tell. It's just that it behaved differently before and only occurs when reading from the cache in production. I don't know what to do here - @symfony/deciders time? |
|
ok let's wait for the feedback :) |
|
FYI I agree that it should be removed in the next version 2.8 |
|
Haven't read the whole thing, so not sure if this is relevant, but if something introduced in 2.7 should be removed, it's still possible during the beta period. |
|
Note that knowing from which locale comes the translation is important when using the translator: it impacts the pluralization rules. So it this info is lost, it introduces a bug in the translator |
|
I have tried various approaches to fix Although in the best case I was able to get consistent results independent of whether a cache was used or not, there would still be a difference in the messages available in the returned catalogue (all messages from a particular locale present in <= 2.6, only the actual fallbacks in 2.7). Also, in my attempts the MessageCatalogue chain would be very fragile with results (correctness & optimization) depending on adding fallbacks in a particular order (top-down) or not changing catalogues after the optimization has been applied – at least ugly API-wise. So, for the time being, I would like to recommend merging this PR; that is, revert the optimization introduced in 2.7. For now, that would keep results unchanged for our users and gives us time to thoroughly think this through, before adding another (new) mode of behaviour we'd be required to support in the future. |
|
@mpdude ok thank you for your effort here :) , can you update the PR contribution header in the description and squash your commits. |
…eb5e73 Also add a test that shows where this would cause inconsistent behaviour of the getFallbackCatalogue() method.
|
Done. |
|
👍 |
1 similar comment
|
👍 |
|
Thank you @mpdude. |
…ght cause inconsistent results when a cache is used (mpdude) This PR was merged into the 2.7 branch. Discussion ---------- [Translation] Revert inlining fallback catalogues as it might cause inconsistent results when a cache is used | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14315 | License | MIT | Doc PR | n/a The results from `getCatalogue()` are inconsistent when we're in production *and* a cache comes into play. This is due to the changes in 6eb5e73, where the fallback catalogues will be *merged* into the primary catalogue when it is written to the cache. Strictly speaking, this is a BC break because it behaved differently before. I am not sure what the relevance of this might be in practice. However, it may cause headaches because * The result changes only in the second+ try (when the cache is warm); a priori you cannot tell whether you're going to see this * The catalogue is clearly not what the loader provided * You have no chance of telling whether the message was originally available in the catalogue or not * Generally, for every message you retrieve from the catalogue, you have no way of telling which locale it actually comes from (it need not be from the catalogue's locale, and the catalogue does not provide fallback catalogues either) Regarding the last point, you usually don't care when using the `Translator`. Its purpose is to get the "best" translation available. However, when you bother to explicitly retrieve the catalogue, chances are your intentions are different. Commits ------- 12a183a Revert inlining the fallback messages in production that was added in 6eb5e73
The results from
getCatalogue()are inconsistent when we're in production and a cache comes into play.This is due to the changes in 6eb5e73, where the fallback catalogues will be merged into the primary catalogue when it is written to the cache.
Strictly speaking, this is a BC break because it behaved differently before.
I am not sure what the relevance of this might be in practice.
However, it may cause headaches because
Regarding the last point, you usually don't care when using the
Translator. Its purpose is to get the "best" translation available. However, when you bother to explicitly retrieve the catalogue, chances are your intentions are different.