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

Don't hardcode locale for to_sentence #37047

Merged
merged 1 commit into from Aug 28, 2019

Conversation

dvandersluis
Copy link
Contributor

Summary

When a HasManyThroughSourceAssociationNotFoundError exception is raised, and there is no en locale, the user receives a I18n::InvalidLocale: :en is not a valid locale exception instead which suppresses the actual error.

This change just checks for an en locale being available and falls back to I18n.default_locale if it's not.

Fixes #37044.

@eileencodes
Copy link
Member

I'm not sure the locale is needed at all. This isn't an error that we show to users on the frontend and looking through the rest of ActiveRecord we don't directly pass locale into those either. I tracked this change down to 6de8356 and it's clear from other uses of to_sentence that we're not forcing the locale anymore. I don't think there are consequences from removing the locale from this error instead.

What do you think?

@dvandersluis
Copy link
Contributor Author

@eileencodes makes sense to me, I'll make that change!

@dvandersluis dvandersluis force-pushed the 37044-fix-exception-message branch 2 times, most recently from 9951701 to 63b3617 Compare August 26, 2019 20:27
@dvandersluis
Copy link
Contributor Author

dvandersluis commented Aug 26, 2019

FWIW it looks like

super("Only #{allowed_methods.to_sentence(locale: :en)} requests are allowed.")
is the only other place that is using a hardcoded I18n locale with to_sentence currently.

@dvandersluis dvandersluis changed the title Fallback to the default locale if there is no en locale Don't hardcode locale for to_sentence Aug 26, 2019
@dvandersluis
Copy link
Contributor Author

@eileencodes should I change ☝️ that one too?

@eileencodes
Copy link
Member

Yup, let's update that one too.

For example, HasManyThroughSourceAssociationNotFoundError uses
to_sentence to create its message, but was hardcoded to use the `en`
locale. In cases where this locale did not exist, this caused the
exception to be suppressed in favour of an I18n::InvalidLocale
exception.

This change removes the hardcoded locale, which allows I18n to use its
default locale.
@dvandersluis
Copy link
Contributor Author

@eileencodes done!

@eileencodes eileencodes merged commit 063056b into rails:master Aug 28, 2019
eileencodes added a commit that referenced this pull request Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails assumes there is an :en locale for HasManyThroughSourceAssociationNotFoundError
2 participants