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

Delete I18n logic from engine #802

Merged
merged 1 commit into from
Mar 12, 2016
Merged

Conversation

johnnyshields
Copy link
Contributor

The lines deleted in the PR are unnecessary, and they inadvertently change the I18n behavior of your entire app.

(In my case, it breaks the i18n-js gem's export, but others may be affected.)

It looks like this was added by @jasl here: 78fa331

My guess was he wanted the English error messages for Doorkeeper to appear even when using a different default locale. But the Doorkeeper gem should NOT reconfigure your Rails app globally in order to achieve this goal!

The deleted lines are unnecessary and change the I18n behavior of your entire app.
@johnnyshields johnnyshields changed the title Remove I18n logic from engines Delete I18n logic from engine Mar 11, 2016
@tute
Copy link
Contributor

tute commented Mar 11, 2016

Thanks for your contribution!

I agree that doorkeeper shouldn't silently reconfigure your app, and that should be considered a bug. That said, this is a backwards incompatible change, which in turns will bring back the bug described in #627.

How can we keep that bug fixed, while being considerate of the application's configuration? Is there a way to fallback only the translations with a doorkeeper. prefix?

@johnnyshields
Copy link
Contributor Author

#627 is not a bug. It is expected behavior of Rails if:

  1. You do not have a translation for a given locale AND
  2. You have not set locale fallbacks in your app.

This is how I18n works for every app and gem in existence, and it's not up to Doorkeeper to force an override on the user.

@johnnyshields
Copy link
Contributor Author

I think we can put a warning in the gem log when upgrading:

****
As of version x.y.z Doorkeeper no longer sets I18n.fallback_locales by default. 
If you are using I18n in your app, please either ensure you have fallbacks in your
Rails configuration (see: http://guides.rubyonrails.org/i18n.html) or use one of our
translations at https://github.com/doorkeeper-gem/doorkeeper-i18n
****

@tute
Copy link
Contributor

tute commented Mar 12, 2016

#627 is not a bug. It is expected behavior of Rails if:

You do not have a translation for a given locale AND
You have not set locale fallbacks in your app.

This make sense.

About backwards incompatibility, we didn't yet release v4, so we can be backwards incompatible. Merging, thank you!

tute added a commit that referenced this pull request Mar 12, 2016
Delete I18n logic from engine
@tute tute merged commit a563a9d into doorkeeper-gem:master Mar 12, 2016
@tute
Copy link
Contributor

tute commented Mar 12, 2016

Ouch, we should have added this context to the commit message. I'll add it to NEWS when we prepare a new release.

@johnnyshields
Copy link
Contributor Author

Cheers!

@johnnyshields johnnyshields deleted the patch-1 branch March 12, 2016 20:34
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.

2 participants