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

Explicitly use full constant name in locale helper #78

Merged
merged 2 commits into from
Dec 15, 2016
Merged

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Nov 26, 2016

Sometimes Rails Module lockup causes errors like in #73

Closes #73

Sometimes Rails Module lockup causes errors like in #73

Closes #73
@fschwahn
Copy link

@tvdeyen rails autoloading usually (or by default) does not include the lib directory, only stuff in app is autoloaded. I think the better way to fix this would be to just require this file in lib/solidus_i18n.rb.

@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 14, 2016

@fschwahn good catch! Will do. Thanks for the heads up

We use them for listing all available locales in the admin,
so for sake of Rails autoloading we should require it explicitely.

Thanks @fschwahn
@tvdeyen tvdeyen requested a review from mamhoff December 14, 2016 10:03
@tvdeyen tvdeyen self-assigned this Dec 14, 2016
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me. In #73, though, the OP said this does not solve his problem, did you verify it did? His comment is ten days old, and I think your PR got recently updated.

@fschwahn
Copy link

@tvdeyen @mamhoff you can reproduce this issue by putting config.cache_classes = true and config.eager_load = true (ie. disable autoloading) and visiting "/en/admin/general_settings/edit".

@mamhoff
Copy link
Contributor

mamhoff commented Dec 14, 2016

@fschwahn If you tell me it fixes your issue, I'll trust you on it.

@fschwahn
Copy link

Yes, this fixes the issue, but the change to LocaleHelper is not necessary, just the require.

@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 14, 2016

but the change to LocaleHelper is not necessary

I know, but it is not harmful and even better to avoid strange Rails module namespace lookup issues like in solidusio/solidus#1615

@fschwahn
Copy link

Yes, you're right

@tvdeyen tvdeyen merged commit 45cb043 into master Dec 15, 2016
@tvdeyen tvdeyen deleted the tvdeyen-fix-73 branch December 15, 2016 08:19
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.

3 participants