Revert #591, add a regression test for #617

[radar]

After #591, and with I18n v1.9.x versions, the fallbacks logic has broken. In this PR, I include a simple test to reproduce the issue as well as reverting #591.

cc @movermeyer / @jeffblake

[radar]

@jeffblake I've just confirmed in that test application that:

gem 'i18n', github: "ruby-i18n/i18n", branch: "617"

No longer makes this issue appear.

As it's currently the weekend (or soon to be) in most places of the world, and I am cautious about releasing a new version and disrupting people's weekends, I will be holding off on committing + releasing this one until Tuesday morning Australian Eastern Time.

[radar]

It appears that the issue is with defaults here being a single object, and not being duplicated. So when defaults.shift is called, the defaults in the options hash is being changed also. If it was a duplicated object, we wouldn't have this problem.

This change to ActiveModel would also fix this issue, and allow for #591 to be added back into I18n:

diff --git a/activemodel/lib/active_model/naming.rb b/activemodel/lib/active_model/naming.rb
index 72a3e43e71..2b7f02fd0b 100644
--- a/activemodel/lib/active_model/naming.rb
+++ b/activemodel/lib/active_model/naming.rb
@@ -205,7 +205,7 @@ def human(options = {})
       defaults << options[:default] if options[:default]
       defaults << @human

-      options = { scope: [@klass.i18n_scope, :models], count: 1, default: defaults }.merge!(options.except(:default))
+      options = { scope: [@klass.i18n_scope, :models], count: 1, default: defaults.dup }.merge!(options.except(:default))
       I18n.translate(defaults.shift, **options)
     end

However, I no longer make contributions to Rails code itself, after bad interactions with a particular person on the core team.

[jeffblake]

Thanks @radar ! That branch fixes it.

[movermeyer]

However, I no longer make contributions to Rails code itself, after bad interactions with a particular person on the core team.

@radar Understandable. Sorry you had to go through that.
I'll test out the fix and open a PR against Rails with that change.

[movermeyer]

Suggested change

	defaults = [:"product/ticket", :product, "Ticket"]
	options = {:scope=>[:activerecord, :models], :count=>1, :default=> defaults}
	assert_equal("Ticket", I18n.t(defaults.shift, **options))
	defaults = [:product, "Ticket"]
	options = {:scope=>[:activerecord, :models], :count=>1, :default=> defaults}
	assert_equal("Ticket", I18n.t(:"product/ticket", **options))

[movermeyer]

@radar To be clear before I say other things: I'm fine with the revert, but disagree with that regression test as written. I've added a suggestion to fix it.

---

I reviewed the proposed fix to Rails, and while I agree that works as a workaround, I disagree that's the solution to the problem.

I think it makes sense that they don't provide the original Symbol as part of the defaults (i.e., the shift modifying options['defaults'] is desired; even though their code could better express their intent. Indeed, they have since refactored that code and it's somewhat clearer now). They don't want to do the lookup of the initial value twice, since they shouldn't expect it to be any different the second time around.

---

#591 is incorrect, since I didn't check/realize that resolve is called in three different cases:

1. When the value found in the translation file is a Symbol (Code)
   • This was the case I handled in Resolve Symbols using the original fallback locale #591. In this case, you need/want to restart your search from the original fallback locale.
2. When a value is not found, then values from defaults are resolved. (Code)
   • When resolving defaults, you want to do it only at the level of the current locale, before falling back.
   • i.e., Go through all defaults before falling back through fallbacks locales, keeping the behaviour as it has always been.
3. When the value found in the translation file is a Proc (Code)
   • I haven't yet given enough thought to convince myself about which locale should be used when the Proc returns a Symbol (current locale or the original locale)

Prior to #591, there was no distinction between the cases. #591 should have introduced that distinction.

---

I'll look into what can be done and open a replacement to #591 (Update: replacement PR is here)
But it won't require a change in the caller's defaults parameters.

[movermeyer]

@radar I've opened an alternative PR to this revert: #622

[radar]

Looks like #622 is the go here. I'll be closing this one, and doing a release tomorrow AM including #622.