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

fix I18n.load_path injection #546

Merged
merged 3 commits into from
May 30, 2024
Merged

fix I18n.load_path injection #546

merged 3 commits into from
May 30, 2024

Conversation

glaszig
Copy link
Contributor

@glaszig glaszig commented Nov 9, 2023

ice_cube would inject its locale files at the end of I18n.load_path due to its IceCube::I18n module being autoloaded and thereby overwrite any customisation the user may have made in other locale files earlier in the load path.

i fix this by injecting ice cube locales at gem load time so that the user has a chance to modify locale keys later.

this also eliminates a bunch of delegation having a custom I18n module; IceCube::I18n is just the same as ::I18n if the i18n gem is available.

resolves #489, #432, #431

@glaszig
Copy link
Contributor Author

glaszig commented Nov 9, 2023

since I18n.load_path is dependent on gem load order there still is a chance ice_cube's vendored translations will overwrite definitions:

Gemfile:

gem 'rails-i18n'
gem 'ice_cube'

I18n.localize requires the user define translations anyway so it's better to be a good gem and not simply break things. thus i moved all <locale>.date.* keys to be used only during tests and cleaned up a little.

@glaszig
Copy link
Contributor Author

glaszig commented Nov 9, 2023

in the meantime i'm using this patch in a rails app to move ice_cube's translations to the top of I18n.load_paths:

config/initializers/ice_cube.rb

module IceCube
  LOCALES_PATH = I18n::LOCALES_PATH

  remove_const :I18n

  I18n = begin
    require "i18n"
    ::I18n.load_path.prepend *Dir[File.join(LOCALES_PATH, "*.yml")]
    ::I18n
  rescue LoadError
    NullI18n
  end
end

@glaszig
Copy link
Contributor Author

glaszig commented Jan 27, 2024

?

@GitToTheHub
Copy link

GitToTheHub commented Feb 2, 2024

This was breaking my app also. I was wondering why I18n.localize was using a different rendering for times, when i used in a view Schedule.to_s. The patch you showed here, fixed it:

in the meantime i'm using this patch in a rails app to move ice_cube's translations to the top of I18n.load_paths:

config/initializers/ice_cube.rb

module IceCube
  LOCALES_PATH = I18n::LOCALES_PATH

  remove_const :I18n

  I18n = begin
    require "i18n"
    ::I18n.load_path.prepend *Dir[File.join(LOCALES_PATH, "*.yml")]
    ::I18n
  rescue LoadError
    NullI18n
  end
end

Thanks.

@danielschwab
Copy link

Using ice-cube germ breaks our app and the mentioned workaround fixed it.
This PR should be merged!

@pacso
Copy link
Collaborator

pacso commented May 14, 2024

By removing the non-ice_cube date translations and loading them only in the tests, I think that would break things like this and this

@@ -0,0 +1 @@
I18n.load_path += Dir[File.expand_path("../locales/*.yml", __dir__)]
Copy link
Collaborator

@pacso pacso May 14, 2024

Choose a reason for hiding this comment

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

We definitely shouldn't have test-only code like this. If we need these translations for code to work, then we need them in the main codebase. And if we don't need these translations in the main codebase, then we shouldn't need them here.

It looks like it's just the day_of_week and month_of_year validations that need fixing so that these translations can just be completely removed.

@glaszig
Copy link
Contributor Author

glaszig commented May 14, 2024

i have 2 ideas:

  1. have an ice_cube namespace as fallback: I18n.t("day_names", scope: %i(date ice_cube))[day]
  2. raise on missing translation: I18n.t!("date.day_names")[day]

@glaszig glaszig requested a review from pacso May 14, 2024 18:42
@pacso
Copy link
Collaborator

pacso commented May 18, 2024

I assume the reason it's done this way is so that if ActiveSupport is available, it'll load the translations for day_names and month_names from there instead, rather than "duplicating" it. However, we're duplicating it anyway.

Why not just move those translations into the ice_cube namespace, and then you can remove those includes from the tests completely? We also don't ever use the abbr_day_names or abbr_month_names, so I'm not sure why we include translations for them?

@pacso
Copy link
Collaborator

pacso commented May 18, 2024

OK - having had a poke about, we cannot remove the date.day_names etc from the translation files. These are used by the localisation calls (I18n.l) such as here.

Please revert the changes to the locale files, and remove the added locales from the spec directory, and see if everything tests ok. Without doing this, the gem would be broken for anybody using it in an environment without ActiveSupport.

Copy link
Collaborator

@pacso pacso left a comment

Choose a reason for hiding this comment

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

As discussed above, please remove these locale changes from the specs, and reinstate in the main locale files.

ice_cube would inject its locale files at the end of `I18n.load_path` due to its `IceCube::I18n` module being `autoload`ed and thereby overwrite any customisation the user may have made in other locale files earlier in the load path.

i fix this by injecting ice cube locales at gem load time so that the user has a chance to modify locale keys later.

this also eliminates a bunch of delegation having a custom I18n module; `IceCube::I18n` is just the same as ::I18n if the i18n gem is available.

resolves ice-cube-ruby#489, ice-cube-ruby#432, ice-cube-ruby#431
@pacso pacso merged commit ab36aba into ice-cube-ruby:master May 30, 2024
11 checks passed
@pacso
Copy link
Collaborator

pacso commented May 30, 2024

That's great, thank you @glaszig.

There are a couple of other fixes I'd like to get merged in, and then we'll have a new release out with your changes.

@jonchay
Copy link

jonchay commented Jun 13, 2024

Would it be possible to release a gem version with this change?

@1st8
Copy link

1st8 commented Jul 3, 2024

Just ran into this as well, will use master until it is released.

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

Successfully merging this pull request may close these issues.

6 participants