Fix error when including Fallbacks on non-Simple backend #260
Conversation
Commit 12aa0f0 introduced a bug, where if `Fallbacks` module is included in a class that doesnt define `translations` the code would fail. This is a pretty common scenario, for instance when the backend is a `Chain`. Also this was pretty common case to fail in rails, as its include Fallbacks in the I18n.backend, like this: ``` I18n.backend.class.send(:include, I18n::Backend::Fallbacks) ``` This stops using the `translations` method in the fallbacks, and instead ignores `I18n::InvalidLocale` errors. [fixes ruby-i18n#238] [fixes ruby-i18n#258] [fixes ruby-i18n#259]
|
Just a note: I vaguely remember that when I investigated this in my "optimized" fork (#138) I found out that fallbacks usually generate a bazillion of fallback locales that usually do not exist thus resulting in a series of |
|
WE NEED THIS TO GO IN ASAP! 0.6.10 hosed our Production environment and took hours to figure out. Rolled back to 0.6.9 and all working again. MAJOR BUG for 0.6.10 that will hose a lot of the Rails ecosystem! |
|
@stuarthannig I'm sorry about that, unfortunately our tests didn't catch the problem before. 0.6.10 has been yanked for now, expect a new release soon including this fix. Thanks. |
| I18n.backend = I18n::Backend::Chain.new(I18n::Backend::Simple.new, backend) | ||
| I18n.backend.class.send(:include, I18n::Backend::Fallbacks) |
There was a problem hiding this comment.
Aren't we having two fallbacks being triggered now? One being included in Chain, other in the Backend class being used here. Does this affect anything?
There was a problem hiding this comment.
you're right, it would call fallback 2x. But I am sure this is not an issue. I wanted to add the send(:include, I18n::Backend::Fallbacks) to the root backend because thats what rails does.
I probably could refactor those tests, to simulate situations similar to the ones that happen on rails code.
Fix error when including Fallbacks on non-Simple backend
|
@thedarkone thanks for sharing your thoughts/concerns on this one, I'm going to review your perf changes when I find some time here so we can get them in. Thanks everyone. |
|
Not to throw rocks here but @stuarthannig is right that this faulty release / version yank was entirely avoidable. I flagged this issue originally on Jan 7 (#238), and I asked for attention multiple times but was ignored. |
|
It was the first thing we could think of to avoid more people having issues installing it. I didn't want to release a possible-another version with problems without someone being able to verify it was working, so I did what I thought would be safer. I'm sorry I missed your issue before, I wasn't in the project when you sent it, anyway I'll try to review more carefully the issues before releasing. Thanks. |
|
Btw I'm planning to release a new version with that fix soon, I'll keep you guys posted. |
|
@carlosantoniodasilva thanks for the quick response. Gem yank was the appropriate action when the issue was noticed. Appreciate all your great work on this and Rails. |
Commit 12aa0f0 introduced a bug, where
if
Fallbacksmodule is included in a class that doesnt definetranslationsthe code would fail. This is a pretty commonscenario, for instance when the backend is a
Chain. Also this waspretty common case to fail in rails, as its include Fallbacks in the
I18n.backend, like this:
This stops using the
translationsmethod in the fallbacks, and insteadignores
I18n::InvalidLocaleerrors.[fixes #238]
[fixes #258]
[fixes #259]
review @carlosantoniodasilva