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

Support ActionDispatch::Http::UploadedFile again #585

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

mcls
Copy link
Contributor

@mcls mcls commented Mar 27, 2018

Using the path method instead of passing the file directly to File.basename ensures that ActionDispatch::Http::UploadedFile can be used again.

Related issue: #584

Note: ActionDispatch isn't a dependency of the library, so I opted not to write a test specifically for it. I tried to account for it in the existing tests by checking that e.g. to_str isn't called on the file object, but that didn't work since File.basename is implemented in C.

Using the `path` method instead of passing the file directly to
`File.basename` to get the filename ensures that
`ActionDispatch::Http::UploadedFile` can be used again.

Related issue: jnunemaker#584
@jnunemaker
Copy link
Owner

So this works with regular files and action dispatch uploads? Is path right? File.basename returns the filename, not a full path, but path makes me think it is more than just the filename. Any thoughts either way on that? Just curious.

@mcls
Copy link
Contributor Author

mcls commented Mar 27, 2018

Hi, it's still using File.basename. What I meant to explain is that passing the ActionDispatch::Http::UploadedFile object directly to File.basename doesn't work, while it does for regular File objects.

All I changed is passing the file's path to the object instead, which is what File.basename does normally. The effect is the same, it's just that it supports ActionDispatch now.

Example:

irb(main):003:0> f = File.open("lib/httparty.rb")
=> #<File:lib/httparty.rb>
irb(main):004:0> f.path
=> "lib/httparty.rb"
irb(main):005:0> File.basename(f)
=> "httparty.rb"
irb(main):006:0> File.basename(f.path)
=> "httparty.rb"

versus

irb(main):002:0> file = ActionDispatch::Http::UploadedFile.new(tempfile: File.open("config/routes.rb"))
#<ActionDispatch::Http::UploadedFile:0x00007fe148b67bd8 @tempfile=#<File:config/routes.rb>, @original_filename=nil, @content_type=nil, @headers=nil>
irb(main):003:0> file.path
"config/routes.rb"
irb(main):004:0> File.basename(file.path)
"routes.rb"
irb(main):005:0> File.basename(file)
TypeError: no implicit conversion of ActionDispatch::Http::UploadedFile into String
        from (irb):5:in `basename'
        from (irb):5
        from /Users/mcls/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/railties-5.0.6/lib/rails/commands/console.rb:65:in `start'
        from /Users/mcls/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/railties-5.0.6/lib/rails/commands/console_helper.rb:9:in `start'
        from /Users/mcls/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/railties-5.0.6/lib/rails/commands/commands_tasks.rb:78:in `console'
        from /Users/mcls/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/railties-5.0.6/lib/rails/commands/commands_tasks.rb:49:in `run_command!'
        from /Users/mcls/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/railties-5.0.6/lib/rails/commands.rb:18:in `<top (required)>'
        from bin/rails:4:in `require'
        from bin/rails:4:in `<main>'

@jnunemaker
Copy link
Owner

Hi, it's still using File.basename.

I overlooked that somehow. 😆 My bad.

@jnunemaker jnunemaker merged commit e91c120 into jnunemaker:master Mar 30, 2018
@jnunemaker
Copy link
Owner

This is out in 0.16.2. Thank you so much for caring and fixing this.

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.

2 participants