Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Translation][cache fallback] keep only missing messages in fallbackCatalogue.#14318
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
aitboudad commentedApr 11, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Fixed tickets | #14315 |
| Tests pass? | yes |
| License | MIT |
eda1193 to00f84f0Compare| @@ -444,6 +430,14 @@ private function getFallbackContent(MessageCatalogue $catalogue) | |||
| $fallbackSuffix = ucfirst(preg_replace($replacementPattern, '_', $fallback)); | |||
| $currentSuffix = ucfirst(preg_replace($replacementPattern, '_', $current)); | |||
| $fallbackMessages = $fallbackCatalogue->all(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be inelse to avoid call if not needed as you replace it if not in debug anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
I don't think this optimizes anything? |
547c8a9 to41fd50aCompare41fd50a to41f1eccCompare@mpdude this PR is about fixing a bug in the previous optimization. and keeping only the missing messages rather than the whole fallback catalogue is about optimizing memory usage. |
@nicolas-grekas we have a Fatal Error related to the Debug component in the deps=high build when running the TwigBundle tests. It would be great to investigate it |
@stof sure, I understand what it is about. The thing is that this optimization and dropping the messages causes unintuitive and possibly BC breaking behaviour when getting the catalogue from the translator (#14315). As this PR passes the tests from#14315 I wonder if itreally fixes the problem or if the actual optimization does not happen anymore at all, although (at a glimpse) the code suggests it. |
No, it's not useless. It shows that your fix here does not suffice. Run it against a Symfony version before the optimization was introduced and it will pass. |
@mpdude ok no problem for me :) |
I have an idea how we might be able to rescue this. It's not gonna be beautiful, but might work. Need to think about it a little more and will get back to you. |