Use `content_disposition` gem for encoding filenames

[aried3r]

The current method wrongly encodes filenames, the easiest example being a filename containing a space.

Filename: test 1.zip Prior to #81: test 1.zip After #81: test+1.zip After this PR: test 1.zip

I've verified locally that the example given in RFC6266 using the euro sign (€) still works.

This is a draft PR, because the ActionDispatch::Http::ContentDisposition is :nodoc: which I believe means private API for Rails. Additionally, it only exists starting Rails 6.0 while this gem supports Rails (well, actionpack) 3.2.1 and above.

https://github.com/fringd/zipline/blob/b5b7d74c16f010f100707358f90158bf911b8ce3/zipline.gemspec#L19

To resolve this, we could raise the minimum actionpack version to 6.0 or use a gem like content_disposition which extracted the code from Rails (supports Ruby 2.3 and above, while this gem I believe has no lower bound for supported Ruby versions) or copy the code to this gem and maintain it ourselves if necessary.

WDYT? Ping @dot.

[aried3r]

supports Ruby 2.3 and above, while this gem I believe has no lower bound for supported Ruby versions

Apparently only the .gemspec file has no lower limit by defining required_ruby_version but the tests only run on 2.4 and above. If we decide against using the ContentDisposition helper from actionpack, we could use the content_disposition gem and define a required_ruby_version so people don't end up with incompatible versions.

[dot]

I have confirmed the problem with the filename with whitespace. And I understood that the problem is caused by the different handling of whitespace characters in RFC5987, which is the escaping specification required by RFC6266, and URI.encode_www_form_component in Ruby (3.1).

• https://datatracker.ietf.org/doc/html/rfc5987#section-3.2.2

  Also, note that the space character was encoded as %20, as it is not contained in attr-char.

• https://docs.ruby-lang.org/en/3.1/URI.html#method-c-encode_www_form_component
  • https://www.w3.org/TR/2013/CR-html5-20130806/forms.html#url-encoded-form-data

    If the byte is 0x20 (U+0020 SPACE if interpreted as ASCII) Replace the byte with a single 0x2B byte ("+" (U+002B) character if interpreted as ASCII).

I think it would be a difficult decision to apply the private method of ActionDispatch or to add a new external gem. And previous PR(#81) is inadequate, and even if it is reverted, I have no objection.

[fringd]

I'd prefer to pull in a new gem and bump the required ruby version... I think more people are using old rails than old ruby. right?

[aried3r]

Hmm, I'm not sure why this would suddenly fail. I'll try to have another look today but would also appreciate another set of eyes.

[aried3r]

Friendly ping @petergoldstein, I had to remove JRuby testing which you added in #83 because it doesn't support Ruby 2.7. I wonder how the test run was able to pass in that case. OTOH I now also have test failures in this PR that weren't there before I raised the minimum actionpack and Ruby versions.

[aried3r]

The test failure was due to rspec-mocks, this was easy enough to debug thanks to my previous Terminal output, but it was made a bit more difficult due to the missing Gemfile.lock file in the repo. Perhaps we should commit it? From bundler:

Q: Should I commit my Gemfile.lock when writing a gem?

A: Yes, you should commit it.

[petergoldstein]

@aried3r I understand the comment from Bundler, but generally speaking if you want to do multi-Ruby CI, then you generally need to do choose one of several approaches:

1. Have a single shared Gemfile, which is in the root of your repo, that has no Gemfile.lock committed. That allows the different elements of the CI matrix to load the necessary gem versions for their combinations
2. Have multiple Gemfiles, one of each entry on an axis in your CI matrix, with no Gemfile.lock committed. For example, if you were testing several Ruby versions and several Rails versions, it's fairly common to have a Gemfile for each Rails version, and then run compatible Rubies against each Rails version.
3. Have multiple Gemfiles, one for each exact entry in your CI matrix. You may or may not commit Gemfile.lock files in this case

This allows compatibility to float - i.e. one version of activesupport may be compatible with your oldest supported Ruby, while it may not be compatible with your latest Ruby. And the version of activesupport that's compatible with your latest Ruby has dropped support for your oldest.

[fringd]

this looks like a good compromise for now. merging!