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

Fall back to parent locale before falling back to the :errors namespace #35424

Merged
merged 2 commits into from Mar 12, 2019

Conversation

Korri
Copy link
Contributor

@Korri Korri commented Feb 27, 2019

Summary

Right now, validation messages always fall back to generic messages before falling back to another locale.

This causes users using sub-locales to sometimes see Generic error messages instead of more specific ones (if not as localized).

For example, because those generic messages are defined inside rails-i18n, for people using this gem, it is not possible to overwrite specific error message for all english locales (as an example) without adding them to all the sub locales en-CA, en-US etc..

TLDR: This is what the fallback order looks like for a blank error on a product's title field with localeen-US (from top to bottom):

en-US.activerecord.errors.models.product.attributes.title.blank
en-US.activerecord.errors.models.product.blank
en-US.activerecord.errors.messages.blank
en-US.errors.attributes.title.blank
en-US.errors.messages.blank

en.activerecord.errors.models.product.attributes.title.blank
en.activerecord.errors.models.product.blank
en.activerecord.errors.messages.blank
en.errors.attributes.title.blank
en.errors.messages.blank

And this is what it looks like after this PR:

en-US.activerecord.errors.models.product.attributes.title.blank
en-US.activerecord.errors.models.product.blank
en-US.activerecord.errors.messages.blank

en.activerecord.errors.models.product.attributes.title.blank
en.activerecord.errors.models.product.blank
en.activerecord.errors.messages.blank

en-US.errors.attributes.title.blank
en-US.errors.messages.blank

en.errors.attributes.title.blank
en.errors.messages.blank

Backward compatibility

It it possible that some users rely on the current behaviour, I can't think of a scenario where it would make sense, but it does change the current behaviour.

@Korri Korri changed the title Fall back to parent locale before it falls back to the :errors namespace Fall back to parent locale before falling back to the :errors namespace Feb 27, 2019
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

I don't have any problems with the behavior change, but I had some questions for the implementation. If @rafaelfranca are okay with those (which he might be since he approved), then let's just ship this.


I18n.translate(key, options)
I18n.translate(key, { default: defaults }.merge!(options))
Copy link
Contributor

Choose a reason for hiding this comment

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

Weren't these options already merged in on 489?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This merge is just to add the default key.

Copy link
Member

Choose a reason for hiding this comment

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

If it is only to add the default keys I think we should add it using []=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it


catch(:exception) do
translation = I18n.translate(defaults.first, options.merge(default: defaults.drop(1), throw: true))
return translation unless translation.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

This deep nested return seems off. Can we do a larger restructuring of this method that makes this flow clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to find a cleaner way to do this, but I can't find a clean way to keep the two following in the case were the entity responds to :i18n_scope:

  1. Try the more precise translations first.
  2. Still show #{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type} as the key that failed translating when both lookup fail.

That makes it so that when i18_scope I need to do two things: add new defaults, and try a first lookup. That makes it hard to refactor into a private method.

I'm open to any suggestions though!

@Korri Korri force-pushed the validation-rules-locale-fallback branch from 6ffb3e6 to 2176f4b Compare March 4, 2019 21:54
@Korri Korri force-pushed the validation-rules-locale-fallback branch from b6c52db to 9ccc5e1 Compare March 11, 2019 19:55
@rafaelfranca rafaelfranca merged commit f2cd46b into rails:master Mar 12, 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.

None yet

3 participants