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.

Sign up for GitHub

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

Jump to bottom

Fix crash when available locales are enforced but fallback locale unavailable #1796

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Fix crash when available locales are enforced but fallback locale unavailable #1796

merged 1 commit into from
Oct 9, 2018

Conversation

Morred
Copy link
Contributor

@Morred Morred commented Sep 29, 2018 •
edited
Loading

  • If I18n enforces available locales, in case of an exception return the translation key if the fallback locale is not supported
  • Set I18n.enforce_available_locales to true in the spec helper because this reflects the default settings for a standard Rails app

@Morred Morred force-pushed the fix-unavailable-locale-crash branch from d70a565 to 100c705 Compare September 29, 2018 01:29
dblock
dblock reviewed Sep 29, 2018
View reviewed changes
spec/grape/exceptions/base_spec.rb Outdated
after do
I18n.available_locales = %i[de en]
I18n.enforce_available_locales = false
end
Copy link
Member

Choose a reason for hiding this comment

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

These probably mess up global state, so the entire spec here should have some kind of after block that restores I18n options to default rather than to de/en.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock How about the enforce_available_locales setting? As far as I can see, the default setting for this is true, so should that be the default setting for all specs too?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like it. If you want to be sure set it globally in spec_helper.rb.

@Morred Morred force-pushed the fix-unavailable-locale-crash branch from 336fafa to 7ee0007 Compare September 29, 2018 02:21
@Morred Morred mentioned this pull request Sep 29, 2018
Closed
@Morred
Copy link
Contributor Author

Morred commented Sep 29, 2018

Not sure why the rails edge test is failing actually, seems to be an issue within ActiveSupport? 🤔

@dblock
Copy link
Member

dblock commented Sep 29, 2018

Yeah, see #1795 (comment). First one to fix gets their PR merged ;)

@Morred Morred force-pushed the fix-unavailable-locale-crash branch from 0169aad to 7e3af39 Compare October 9, 2018 01:13
Morred added a commit to Morred/grape that referenced this pull request Oct 9, 2018
@Morred
Fix crash when available locales are enforced but fallback locale una…
7e3af39 
…vailable (ruby-grape#1796)
@Morred Morred changed the title Add test to confirm crash when available locales are enforced but fal… Fix crash when available locales are enforced but fallback locale unavailable Oct 9, 2018
@Morred Morred force-pushed the fix-unavailable-locale-crash branch from 7e3af39 to 7b18778 Compare October 9, 2018 01:31
Morred added a commit to Morred/grape that referenced this pull request Oct 9, 2018
@Morred
Fix crash when available locales are enforced but fallback locale una…
7b18778 
…vailable (ruby-grape#1796)
@Morred Morred force-pushed the fix-unavailable-locale-crash branch from 7b18778 to f0d1fa1 Compare October 9, 2018 01:38
@dblock dblock merged commit b88a8df into ruby-grape:master Oct 9, 2018
@dblock
Copy link
Member

dblock commented Oct 9, 2018

I merged this.

@dblock
Copy link
Member

dblock commented Oct 9, 2018

Thanks @Morred !

@Morred Morred deleted the fix-unavailable-locale-crash branch October 9, 2018 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Morred @dblock