search

refinery / refinerycms-i18n

I18n logic for Refinery CMS
http://refinerycms.com
54 stars 65 forks source link

Globalize thread issue in admin controller #62

Closed j15e closed 7 years ago

j15e commented 7 years ago

In the before filter to set the locale for Refinery::AdminController, the Globalize.locale is only set if currently nil. This is causing issue with threaded servers like Puma, where the locale is kept set as during the previous request and could be different than the current request I18n.locale :

https://github.com/refinery/refinerycms-i18n/blob/master/lib/refinery/i18n/engine.rb#L63

In the before filter for Refinery::ApplicationController, there is no such condition and therefore it is reset properly on each request.

https://github.com/refinery/refinerycms-i18n/blob/master/lib/refinery/i18n/engine.rb#L37

I tried to find why there is a condition to reset the globalize locale only in the admin section and the only commit I found was this one : https://github.com/refinery/refinerycms-i18n/commit/f89ef903634092b8c355f24a315ff1de65d4bb94

Seems like there is no specific reason to have that condition.

I will make a path on a fork for the old version I am using, but I could make a PR for the latest version too if you deem the change correct.

simi commented 7 years ago

Hmm, that's weird. Can you open pull request? I'll try to test your changes.

j15e commented 7 years ago

I think its a bit more complex in the end because of all the conditions in the before filters. I think it should be rewritten to be more concise and clear.

The issue only arise if you change the locale in a request out of the AdminController (in the AppController), then in a subsequent request, you go back to the default locale in an AdminController request without using the switch_locale param (which would force to reset Globalize.locale).

2 steps to reproduce :

# Refinery app configured with 
# - Refinery::I18n.default_frontend_locale = :en
# - Refinery::I18n.frontend_locales = [:en, :fr]

GET /fr/some-page
# ApplicationController#find_or_set_locale filter will set Globalize.locale to :fr 

GET /refinery 
# Refinery::AdminController#globalize! will NOT reset Globalize.locale to :en

In that second request, the default locale will be set for ::I18n and will be the same as ::Refinery::I18n.default_frontend_locale, but Globalize will not be set properly because there is no params[:switch_locale] and ::I18n.locale != ::Refinery::I18n.default_frontend_locale is false and Globalize.locale is still set to the previous locale (and not nil).

I think find_or_set_locale and globalize! should be combined to make the code more concise & easy to understand. There is so many conditions its really hard to get ahold of what will actually happen.

I will try to come up with a pull request.

j15e commented 7 years ago

I also just found out the whole globalize! method is overwritten by the core refinery gem which also make the codebase hard to understand :

https://github.com/refinery/refinerycms/blob/master/pages/app/controllers/refinery/admin/pages_controller.rb#L70-L79

Making progress on my investigation