use nil_adapter when attachment = "" #1178

wants to merge 3 commits into


None yet

3 participants


In Rails after the 3.2.11 update, when a form with file upload field is submitted without an attachment, the attachment attribute is now passed in the params as "". This causes a 'No handler found for ""' exception in Paperclip.

This commit coerces a '' attachment to the NilAdapter and resolves the problem.

thoughtbot, inc. member

This is a good change, but I think it would be better if the check was for #blank? instead of "#nil? || == ''. That would also allow a single check against something that responds true to blank? in the test instead of looping everything twice.

thoughtbot, inc. member

And now, having to amend myself, we try to avoid #blank? elsewhere, because Paperclip is used in more than just Rails. So that part of it is actually ok. I still dislike the loop in the test as-is, though, and I wonder if there's a way we can change that.

thoughtbot, inc. member

Ok, take 3. We already require ActiveSupport and use present? elsewhere, so there's no reason to shy away from blank?.


OK Jon, do you want me to push an addendum? I'll use present?
What about the looping test? Makes sense to me, since it is checking the expectations under two initial conditions.

thoughtbot, inc. member

Yes, please push an addendum using blank?. Instead of unrolling the loop as-is, it might make more sense to move the tests to a helper. I really dislike looping over an array for things like this. Sorry for my delay.


Hi Jon, I've just pushed two commits in response.

(a) I realised while testing that switching to blank? gobbles up more than I'd originally required (only empty blank files). I'm not sure if there may be other ramifications of this, but I added a separate test case for non-empty blank files.

(b) is this the kind of test refactoring you expected? I put the helper in test_helper.rb as that seemed the place other helpers are kept.

@jyurek jyurek commented on the diff May 10, 2013
@@ -197,3 +197,27 @@ def assert_file_exists(path)
def assert_file_not_exists(path)
assert !File.exists?(path), %(Expect "#{path}" to not exists.)
+def should_register_with_valid_nil_adapter target
jyurek May 10, 2013 thoughtbot, inc. member

This refactoring is good, but the method would be better if it were in the nil_adapter_test file, close to the thing it's testing.


(copied from my similar, but different PR #1220)

This interprets posting an empty file field as an explicit deletion of the attachment. Are we sure this is expected behavior?

thoughtbot, inc. member

As mentioned in #1220, assigning a "" should preserve the existing file. I don't know why Rails is submitting an empty string when the file field is empty, but it definitely doesn't mean "Let's delete the attachment". I've added an empty string handler as of e54b5bd that will simply ignore the assignment.

@jyurek jyurek closed this Jul 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment