automate set_locale #36

Merged
merged 1 commit into from Jan 23, 2015

Conversation

Projects
None yet
4 participants
Contributor

masarakki commented Feb 5, 2014

make more easier to use

lib/http_accept_language/railtie.rb
+
+ config.after_initialize do |app|
+ ActionController::Base.send(:include, AutoLocale) if defined?(ActionController::Base) && app.config.try(:auto_locale)
+ end
@DouweM

DouweM Feb 5, 2014

Collaborator

Could you do this using ActiveSupport.on_load :action_controller, as with EasyAccess a couple of lines up?

@masarakki

masarakki Feb 5, 2014

Contributor

because config/application.rb is not yet loaded in ActiveSupport.on_load.
I can't find a way to configure ability of auto_locale in this timing.

so if I want to include in on_load, auto_locale method should be like this,

  def auto_locale
      I18n.locale =  http_accept_language.compatible_language_from(I18n.available_locales) if auto_locale?
  end 

but i think it it not so cool, because auto_locale will be called always if you don't want to do.

Collaborator

DouweM commented Feb 5, 2014

I'm not sure about the name "auto_locale". The included method should be set_locale, and I think the config var could be more descriptive.

Also, have you tested what happens when the language in the header isn't one of the available_locales at all?

Contributor

masarakki commented Feb 5, 2014

@DouweM
I just want a most easy way to shortcut set_locale in README,
so code of auto_locale is copied from README, and name of auto_locale is only for avoid duplication with it.

Collaborator

DouweM commented Feb 5, 2014

If we're going to add this as a feature, it needs to work and work well. In the example we can ignore the case of unknown languages, but we can't if we're going to pretend this is actually something people should use instead of writing the code manually.

Contributor

masarakki commented Feb 5, 2014

I added spec, but not squashed.

Collaborator

DouweM commented May 3, 2014

Apologies for the late response.

Could you change the filter #auto_locale to #set_locale? That way it can easily be overridden on a controller-by-controller basis by simply defining that method.

The config option needs a more descriptive name as well and may be more appropriate on config.i18n instead of config directly. Could you change it to config.i18n.automatically_set_locale?

Thanks!

Contributor

masarakki commented May 3, 2014

i changed.

bump

DouweM added a commit that referenced this pull request Jan 23, 2015

@DouweM DouweM merged commit 282f069 into iain:master Jan 23, 2015

Collaborator

DouweM commented Jan 23, 2015

Merged and released as 2.0.3!

Contributor

Kriechi commented Jan 23, 2015

I'm not sure what went wrong but it does not work for me. I get the following error:

[...]/gems/activesupport-4.1.8/lib/active_support/i18n_railtie.rb:49:in `block in initialize_i18n': undefined method `automatically_set_locale=' for I18n:Module (NoMethodError)

Rails 4.1.8 on OSX 10.9, ruby 2.1.5 with rbenv from homebrew.

# config/application.rb
config.i18n.enforce_available_locales = true
config.i18n.available_locales = [:en, :de]
config.i18n.automatically_set_locale = true

I deleted the old code from ApplicationController.

Collaborator

DouweM commented Jan 23, 2015

Yeah, I may have jumped the gun on that one. Will look into it.

Collaborator

DouweM commented Jan 23, 2015

Yanked v2.0.3 and released v2.0.4 which should fix this. The way to enable the feature has changed slightly: you should now set HttpAcceptLanguage.automatically_set_locale = true in an initializer.

Contributor

Kriechi commented Jan 23, 2015

At least the error is gone...
But it still seems to be broken. It does not select the correct language for me...

Collaborator

DouweM commented Jan 23, 2015

Could you show the Accept-Language header you're testing this with? I've tested it in a clean Rails 4 application and it works here.

Contributor

Kriechi commented Jan 23, 2015

I guess the railtie is loaded before the initializer kicks in.
So the config never gets loaded...

Contributor

Kriechi commented Jan 23, 2015

request.headers['Accept-Language']
=> "de,en-US;q=0.8,en;q=0.6"

yet my application still shows english text (which is the default locale).

Collaborator

DouweM commented Jan 23, 2015

Hmm, that's annoying. Could you verify if it's a race condition between the railtie and initializers by adding some logging to them? You can get to the http_accept_language source using bundle show http_accept_language from your project folder.

Contributor

Kriechi commented Jan 23, 2015

No it is not a race condition. The railtie simply gets loaded before the custom initializer.
I checked with puts after https://github.com/iain/http_accept_language/blob/master/lib/http_accept_language/railtie.rb#L4
and in the initializer file config/initializers/http_accept_language.rb.

And the output from the railtie came first. Which means it won't read the setting properly, because at this time it is still not set to true.

Collaborator

DouweM commented Jan 23, 2015

Yeah, that's what I was referring to even if the choice of words wasn't correct. :) Thanks for verifying. Weird that it is working right for me. I'll look into it.

Contributor

Kriechi commented Jan 23, 2015

Ok then. I even tried renaming my initializer to config/00_aa_http_accept_language.rb - without any luck. Seems as railties are always loaded first...

Collaborator

DouweM commented Jan 23, 2015

Railties may always be loaded first, but the point is that in your case the ActiveSupport.on_load :action_controller block is called before the other initializers, which is definitely not right, since that's only supposed to run when ActionController::Base is loaded, which should only happen when your ApplicationController is loaded upon the first incoming request.

I have no idea why in your case ActionController::Base is being loaded before the http_accept_language initializer, because it definitely isn't in my case (both for a clean Rails 4.2.0 app and a large, real-life Rails 4.1.7 one.)

@Kriechi Kriechi referenced this pull request Jan 23, 2015

Merged

fix auto_locale #45

4 of 5 tasks complete
Contributor

Kriechi commented Jan 23, 2015

@DouweM @masarakki how about #45 ?

Collaborator

DouweM commented Jan 23, 2015

Much better!

@masarakki masarakki deleted the masarakki:auto_locale branch Jan 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment