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 allow nil to be submitted as a key to i18n.translate() #236

Merged
merged 7 commits into from Mar 5, 2017

Conversation

@jcstringer
Copy link
Contributor

@jcstringer jcstringer commented Dec 18, 2013

Based on the conversation in this previously filed issue #207 I've taken a stab at preventing nil from being accepted as a key when calling the I18n.t method. These changes cause a I18n::ArgumentError to be raised, and I also added the same behavior on the exists? method which seemed appropriate.

As it turns out there were a lot of tests which passed in nil as the key so I had to make quite a few changes there. That led me to having to make a related change to the Fallbacks module which was explicitly sending a nil key to the t method. My change there was to just use the original key instead of nil, so I'd ask that the maintainers take a close look at that to make sure that doing so won't cause any other issues. Test suite is passing for me locally with these changes.

@radar
Copy link
Collaborator

@radar radar commented Nov 17, 2016

Hey @jcstringer, sorry for the delay. Could you please update this against the latest master? I'd be keen to get this into the 0.8.0 release.

@jcstringer
Copy link
Contributor Author

@jcstringer jcstringer commented Nov 18, 2016

@radar great to hear from you on this PR, I will make some time to catch this up with master as you have requested in the next few weeks. Cheers!

@sandstrom
Copy link
Contributor

@sandstrom sandstrom commented Dec 4, 2016

@jcstringer this is a great PR! would love to see it included in i18n ⛵️ any chance you have time to rebase this on top of master?

@jcstringer
Copy link
Contributor Author

@jcstringer jcstringer commented Dec 5, 2016

@sandstrom absolutely just need to find some time hopefully this week?

@jcstringer
Copy link
Contributor Author

@jcstringer jcstringer commented Dec 6, 2016

@sandstrom @radar I've caught this up to master. Locally the specs pass for me on ruby 1.9.3 and 2.3.0. I'm seeing Travis breaking on some of the Rails versions it is testing against, not sure what is expected with this gem and its CI, the few that I checked out in Travis appeared to be related to Gem dependency issues which I would assume are not related to the changes in this PR, but that said let me know if you want help chasing them down especially if they are in fact related to these changes in some way.

Your Gemfile.lock is corrupt. The following gem is missing from the DEPENDENCIES
section: 'i18n'
@sandstrom
Copy link
Contributor

@sandstrom sandstrom commented Dec 6, 2016

@jcstringer Everything looks good to me! Lets see what @radar says.

(Yes, the Your Gemfile.lock is corrupt error is something else)

@jcstringer
Copy link
Contributor Author

@jcstringer jcstringer commented Mar 1, 2017

@sandstrom @radar any updates on the next release of this gem and related PRs?

@sandstrom
Copy link
Contributor

@sandstrom sandstrom commented Mar 1, 2017

@jcstringer I don't have commit rights, so I cannot influence the releases or merge PRs. Sorry that I cannot give a more informative answer!

@radar
Copy link
Collaborator

@radar radar commented Mar 1, 2017

I have commit rights and therefore can influence releases and merge PRs. Sorry that I did not attend to this sooner. I will take a look now.

@radar
Copy link
Collaborator

@radar radar commented Mar 1, 2017

@jcstringer there's some new and exciting conflicts. Could you take a look again please?

@jcstringer jcstringer force-pushed the jcstringer:nil_errors branch to ce7358f Mar 2, 2017
@jcstringer jcstringer force-pushed the jcstringer:nil_errors branch from ce7358f to 842d8d9 Mar 2, 2017
@jcstringer
Copy link
Contributor Author

@jcstringer jcstringer commented Mar 2, 2017

@radar done!

@radar radar modified the milestones: 0.9.0, 0.8.0 Mar 5, 2017
@radar radar merged commit c26891e into ruby-i18n:master Mar 5, 2017
@radar
Copy link
Collaborator

@radar radar commented Mar 5, 2017

Done! :) Thanks for your patience @jcstringer

@cbeer
Copy link

@cbeer cbeer commented May 30, 2017

This change broke cancancan (see the linked CanCanCommunity/cancancan#415). I'm not sure what their intent is, or why passing nil is now disallowed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants