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

Treat locales with variants case insensitively #195

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

nampas
Copy link
Contributor

@nampas nampas commented Oct 30, 2024

We've noticed a few clients sending along funny locales like English
Screenshot 2024-10-30 at 12 18 47 PM

These result in the following error:

I18n::InvalidLocale: "En" is not a valid locale

That's happening because the locale variant match is case insensitive:
https://github.com/rack/rack-contrib/blob/main/lib/rack/contrib/locale.rb#L91

But then the locale is extracted without a downcase.

I believe it should be safe to just downcase the locale from the client in these cases.

@mpalmer
Copy link
Contributor

mpalmer commented Oct 30, 2024

While I'm loathe to do anything that encourages people to send broken request headers (:scream_cat:), the RFCs say that locale specifiers are case-insensitive, so your change is a win for that reason.

@mpalmer mpalmer merged commit 96de0b1 into rack:main Oct 30, 2024
15 checks passed
@nampas
Copy link
Contributor Author

nampas commented Oct 31, 2024

@mpalmer Thanks for the quick response on this. So quick that I wasn't able to verify that the unit test I added works 😮 My bad. I was having some trouble getting dependencies installed and happy on my machine. Are you seeing the test passing in main? Nvrm, I see that CI is passing so that's probably a good sign.

@mpalmer
Copy link
Contributor

mpalmer commented Nov 1, 2024

Yeah, if CI passes, it seems reasonable to assume it's working. What were the errors you were seeing locally?

@nampas
Copy link
Contributor Author

nampas commented Nov 1, 2024

Following the instructions here, I was seeing this from a fresh clone of the repo:

➜  rack-contrib (main) ✗ mise activate
...
➜  rack-contrib (main) ✗ which ruby
/Users/nathanpastor/.local/share/mise/installs/ruby/3.3.3/bin/ruby
➜  rack-contrib (main) ✗ gem install json rack ruby-prof test-spec test-unit
Building native extensions. This could take a while...
Successfully installed json-2.7.5
...
5 gems installed

A new release of RubyGems is available: 3.5.15 → 3.5.22!
Run `gem update --system 3.5.22` to update your installation.

➜  rack-contrib (main) ✗ rake test
...
<internal:/Users/nathanpastor/.local/share/mise/installs/ruby/3.3.3/lib/ruby/site_ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:136:in `require': cannot load such file -- timecop (LoadError)

If I add timecop to the list of gems to install, then rake test fails with:

<internal:/Users/nathanpastor/.local/share/mise/installs/ruby/3.3.3/lib/ruby/site_ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:136:in `require': cannot load such file -- minitest/hooks

I think that was around where I stopped, because I wasn't sure how many more dependencies I'd need to install. But, as one last hurrah, I tried a bundle install, which led to rake test failing with

<internal:/Users/nathanpastor/.local/share/mise/installs/ruby/3.3.3/lib/ruby/site_ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:136:in `require': cannot load such file -- rails/test_unit/reporter

@mpalmer
Copy link
Contributor

mpalmer commented Nov 1, 2024

Whoof, yeah, those instructions are waaaay out of date (as evidenced by the fact that it mentions Ruby 1.8...) I'll make an issue to get that sorted out, but in the meantime, CI says everything's good, and that's good enough for me. 🎉

@ioquatix
Copy link
Member

ioquatix commented Nov 1, 2024

This code base could do with a lot of TLC, feel free to take the lead on updating it if you have the time and energy to do so.

@nampas
Copy link
Contributor Author

nampas commented Nov 1, 2024

Ok I will try to get the readme updated at least. Thanks for the assistance y'all 🙏

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

Successfully merging this pull request may close these issues.

3 participants