Defect #42815
closed
Limit available locales to those defined by Redmine itself no longer works
Added by Marius BĂLTEANU 24 days ago.
Updated 15 days ago.
Description
The change made in #38529 no longer works, Redmine loads available languages also from plugins or gems in the current trunk.
The issue can be reproduce by adding the following gem:
--- a/Gemfile
+++ b/Gemfile
@@ -19,6 +19,7 @@ gem 'rack', '>= 3.1.3'
gem "stimulus-rails", "~> 1.3"
gem "importmap-rails", "~> 2.0"
gem 'commonmarker', '~> 2.3.0'
+gem "doorkeeper-i18n", "~> 5.2"
and then run the test
.F
Failure:
Redmine::I18nTest#test_locales_validness [test/unit/lib/redmine/i18n_test.rb:208]:
Expected: 50
Actual: 54
I'm not sure which change caused this issue and I didn't test yet on 5.1-stable branch.
Files
The attach patch that extracts available_locales
from the module Implementation
seems to fix the issue; all tests pass.
Please ignore the change in the Gemfile; it is only for testing purposes.
Looking at the code, I'm not sure that we need the custom backend anymore, the available locales can be loaded only from the locales files provided by Redmine directly in config/application.rb
.
All tests pass with the attached patch that completely removes the custom backed.
I very much like the idea of the second patch!
However, I'm a bt conflicted if we should monkey-patch the "core" I18n::Backend::Simple
class and forcibly include other modules there. It may be cleaner if we would retain our custom backend, even if it's just an "empty" subclass with some includes. The i18n gem recommends to patch the I18n::Backend::Simple
in its documentation a lot though, so they might be okay with it and commit to keeping mostly stable internal interfaces...
In any case though, if you do want to keep your approach of using I18n::Backend::Simple
directly, is it deliberate that you did not also include the ::I18n::Backend::Fallbacks
module into I18n::Backend::Simple
in your second patch?
Holger Just wrote in #note-3:
I very much like the idea of the second patch!
However, I'm a bt conflicted if we should monkey-patch the "core" I18n::Backend::Simple
class and forcibly include other modules there. It may be cleaner if we would retain our custom backend, even if it's just an "empty" subclass with some includes. The i18n gem recommends to patch the I18n::Backend::Simple
in its documentation a lot though, so they might be okay with it and commit to keeping mostly stable internal interfaces...
That was my initial plan too, but then I saw the documentation.
In any case though, if you do want to keep your approach of using I18n::Backend::Simple
directly, is it deliberate that you did not also include the ::I18n::Backend::Fallbacks
module into I18n::Backend::Simple
in your second patch?
I chose to simply not include it because from my understanding, the config option (config.i18n.fallbacks = true
) from source:trunk/config/application.rb#L60 is doing the same thing, it fallsback to :en
if a translation is missing. Also, the test test_fallback
from source:trunk/test/unit/lib/redmine/i18n_test.rb is passing.
Marius BĂLTEANU wrote in #note-4:
Holger Just wrote in #note-3:
However, I'm a bt conflicted if we should monkey-patch the "core" I18n::Backend::Simple
class and forcibly include other modules there. It may be cleaner if we would retain our custom backend, even if it's just an "empty" subclass with some includes. The i18n gem recommends to patch the I18n::Backend::Simple
in its documentation a lot though, so they might be okay with it and commit to keeping mostly stable internal interfaces...
That was my initial plan too, but then I saw the documentation.
Turns out, that is more or less also what Rails does. In that case, just including the module seems to be the accepted and intended way to enable this.
In any case though, if you do want to keep your approach of using I18n::Backend::Simple
directly, is it deliberate that you did not also include the ::I18n::Backend::Fallbacks
module into I18n::Backend::Simple
in your second patch?
I chose to simply not include it because from my understanding, the config option (config.i18n.fallbacks = true
) from source:trunk/config/application.rb#L60 is doing the same thing, it fallsback to :en
if a translation is missing. Also, the test test_fallback
from source:trunk/test/unit/lib/redmine/i18n_test.rb is passing.
I see. I was not aware how much Rails actually offers here via configuration. Still seems a little selective, but oh well... As such, I agree that enabling config.i18n.fallbacks = true
is the way to go here.
- Related to Patch #38529: Limit available locales to those defined by Redmine itself added
- Status changed from New to Resolved
- Assignee set to Marius BĂLTEANU
- Target version changed from 6.1.0 to 5.1.9
- Resolution set to Fixed
I have committed the first patch and I will merge it to all stable branches since the issue is present on all of them.
Holger, thanks again for you review! I've opened #42859 for refactoring.
- Status changed from Resolved to Closed
Merged to stable branches.
Also available in: Atom
PDF