Browse files

only forwarding enough methods to work. People should grab the delega…

…te tempfile if they really need to do hard work
  • Loading branch information...
1 parent fc9e9ed commit 5e685caf1c9cb1efabc91e1125cf7513760fb379 @tenderlove tenderlove committed Oct 5, 2010
Showing with 16 additions and 19 deletions.
  1. +8 −5 actionpack/lib/action_dispatch/http/upload.rb
  2. +8 −14 actionpack/test/dispatch/uploaded_file_test.rb
View
13 actionpack/lib/action_dispatch/http/upload.rb
@@ -13,13 +13,16 @@ def initialize(hash)
raise(ArgumentError, ':tempfile is required') unless @tempfile
end
- def respond_to?(name)
- super || @tempfile.respond_to?(name)
+ def read(*args)
+ @tempfile.read(*args)
end
- def method_missing(name, *args, &block)
- return super unless respond_to?(name)
- @tempfile.send(name, *args, &block)
+ def rewind
+ @tempfile.rewind
+ end
+
+ def size
+ @tempfile.size
end
end
View
22 actionpack/test/dispatch/uploaded_file_test.rb
@@ -29,36 +29,30 @@ def test_tempfile
end
def test_delegates_to_tempfile
- tf = Class.new { def tenderlove; 'thunderhorse' end }
+ tf = Class.new { def read; 'thunderhorse' end }
uf = Http::UploadedFile.new(:tempfile => tf.new)
- assert_equal 'thunderhorse', uf.tenderlove
+ assert_equal 'thunderhorse', uf.read
end
def test_delegates_to_tempfile_with_params
- tf = Class.new { def tenderlove *args; args end }
+ tf = Class.new { def read *args; args end }
uf = Http::UploadedFile.new(:tempfile => tf.new)
- assert_equal %w{ thunder horse }, uf.tenderlove(*%w{ thunder horse })
- end
-
- def test_delegates_to_tempfile_with_block
- tf = Class.new { def tenderlove; yield end }
- uf = Http::UploadedFile.new(:tempfile => tf.new)
- assert_equal('thunderhorse', uf.tenderlove { 'thunderhorse' })
+ assert_equal %w{ thunder horse }, uf.read(*%w{ thunder horse })
end
def test_delegate_respects_respond_to?
- tf = Class.new { def tenderlove; yield end; private :tenderlove }
+ tf = Class.new { def read; yield end; private :read }
uf = Http::UploadedFile.new(:tempfile => tf.new)
assert_raises(NoMethodError) do
- uf.tenderlove
+ uf.read
end
end
def test_respond_to?
- tf = Class.new { def tenderlove; yield end }
+ tf = Class.new { def read; yield end }
uf = Http::UploadedFile.new(:tempfile => tf.new)
assert uf.respond_to?(:headers), 'responds to headers'
- assert uf.respond_to?(:tenderlove), 'responds to tenderlove'
+ assert uf.respond_to?(:read), 'responds to read'
end
end
end

11 comments on commit 5e685ca

@nragaz

Sad morning - this breaks Paperclip, which uses the to_tempfile method on Http::UploadedFile. I wouldn't raise it except that I doubt Thoughtbot will update the Paperclip gem in the absence of a 3.0.1 release, and others may want to be aware of the issue.

@r3ap3r2004

Hmmmm. Wondering why this change was made to the "3-0-stable" branch. If there's a logical reason to make it to Master fine, but you just broke Paperclip on a 'Stable' branch. Maybe Rails operates by its own rules, but the rest of the world tends to view Stable as something whose interface remains constant so that nothing breaks.

Now I need to try and figure out WTF you are talking about when you say "People should grab the delegate tempfile if they really need to do hard work" so that I can even have a thought of fixing it.

-1 for this commit.

@locriani

This change should have been made to master, not stable.

@alloy

@r3ap3r2004 If you need to do more work with the Tempfile instance, then get the reference to the @tempfile object and do so yourself, instead of having to provide an opaque method_missing method. There's a :tempfile reader accessor on the class for your convenience. Now you can fix whatever is broken (hint: it starts with ‘papercli’).

@r3ap3r2004

alloy,
Unfortunately I don't run the Paperclip project so fixing it is a little out of my hands (although I do have a fork of it since it wouldn't be the first thing that needed fixing).

I don't have a problem with removing an opaque method and requiring the users to access the functionality through a different interface. I do have a problem with someone changing the interface in a stable branch.

These are the type changes that you make in steps.

Step 1) Is that you go ahead and allow the undesired behavior (in this case the method_missing() functionality), and you throw a deprecation warning to the log file indicating that relying on this functionality will cause your product to break when version x.y is released.
Step 2) Is to release an actual release version of the code that includes Step #1's deprecation warning (ex. Release x.x) while still allowing the undesired behavior.
Step 3) is to release version x.y and actually remove the undesired functionality

These are very basic software design principles.

I think the problem is that everyone got in the habit of just tossing out whatever they didn't like during the buildup to 3.0 since they didn't need to necessarily be compatible with anything since 3.0 was such a major change. Now, they are still in the habit of doing that even though they shouldn't be doing it to a stable released version.

@alloy

I know, and it might indeed not have been the smartest move to do this on the ‘stable’ branch. But, and this is the important part, it’s the reality now. So if something’s broken it has to be fixed. And since you apparently had to “figure out WTF you are talking about when you say …”, I gave you an explanation so you could fix it, not really to start a discussion.

Contrary to popular believe, up or down voting, or swearing, or actually anything besides creating a patch fixes software. This is the basic OSS principle.

Finally, moving towards a solution, I would suggest you invest time in a patch for paperclip, rather than in a patch for rails that reverts this. Because imo the reasoning for this patch is solid and I don't think one will take much more time than the other.

@alloy

Oh, by the way, even though this branch has the name ‘stable’ in it, it’s not released code, yet, and thus you're using it at your own peril. I would, however, expect that on this branch no experimental stuff would happen, but this patch is far from it imo. So, since this will probably end up in the next release, there's still time to fix it the way you think it should happen.

@nragaz

Please bros, chill. Changing "to_tempfile" to "tempfile" in Paperclip's lib/attachment.rb:91 should be all that's needed.

@locriani

Paperclip isn't the only code in this universe that uses this interface. As stated earlier, "I do have a problem with someone changing the interface in a stable branch."

@nicksieger

The problem is you can't deprecate switching from an is-a behavior to a has-a behavior. It's going to hurt a little bit. By making this change now, we're telling people to get in line with a coming change. Proper, idiomatic Ruby code should not be depending on specific classes or inheritance in order to work. If you just asked the object if it respond_to?(:read), we'd all be better off.

FYI, for those in need of a patch to paperclip, please try out http://github.com/thoughtbot/paperclip/pull/330.

@nragaz

FYI - Jon Yurek reports that this is fixed in Paperclip now:

http://github.com/thoughtbot/paperclip/issues#issue/327/comment/464440

Please sign in to comment.