DRAFT toward fixing after_post_process is run even if validations fail

lib/paperclip/attachment.rb

@@ -50,7 +50,7 @@ def self.default_options
     # +url+ - a relative URL of the attachment. This is interpolated using +interpolator+
     # +path+ - where on the filesystem to store the attachment. This is interpolated using +interpolator+
     # +styles+ - a hash of options for processing the attachment. See +has_attached_file+ for the details
-    # +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is 
+    # +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is
     #                  a special case that indicates all styles should be processed)
     # +default_url+ - a URL for the missing image
     # +default_style+ - the style to use when an argument is not specified e.g. #url, #path
@@ -499,11 +499,18 @@ def process_options(options_type, style) #:nodoc:
 
     def post_process(*style_args) #:nodoc:
       return if @queued_for_write[:original].nil?
-
-      instance.run_paperclip_callbacks(:post_process) do
-        instance.run_paperclip_callbacks(:"#{name}_post_process") do
-          if !@options[:check_validity_before_processing] || !instance.errors.any?
+      catch(:cancel_all_after_callbacks) do
+        instance.run_paperclip_callbacks(:post_process) do
+          inner_result = instance.run_paperclip_callbacks(:"#{name}_post_process") do
+            # paperclip validations set instance.errors at this point...
+            if @options[:check_validity_before_processing] && instance.errors.any?
+              throw :cancel_all_after_callbacks
+            end
             post_process_styles(*style_args)
+            true
+          end
+          unless inner_result
+            throw :cancel_all_after_callbacks
           end
         end
       end

lib/paperclip/callbacks.rb

@@ -7,7 +7,11 @@ def self.included(base)
 
     module Defining
       def define_paperclip_callbacks(*callbacks)
-        define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby }].flatten)
+        options = {
+          terminator: hasta_la_vista_baby,
+          skip_after_callbacks_if_terminated: true,
+        }
+        define_callbacks(*[callbacks, options].flatten)
         callbacks.each do |callback|
           eval <<-end_callbacks
             def before_#{callback}(*args, &blk)

spec/paperclip/attachment_callbacks_spec.rb

@@ -0,0 +1,56 @@
+require 'spec_helper'
+
+describe Paperclip::Attachment do
+  context "callbacks" do
+    context "with content_type validation" do
+      def rebuild(required_content_type)
+        rebuild_class styles: { something: "100x100#" }
+        Dummy.class_eval do
+          validates_attachment_content_type :avatar, content_type: [required_content_type]
+          before_avatar_post_process :do_before_avatar
+          after_avatar_post_process :do_after_avatar
+          before_post_process :do_before_all
+          after_post_process :do_after_all
+          def do_before_avatar ; end
+          def do_after_avatar; end
+          def do_before_all; end
+          def do_after_all; end
+        end
+        @dummy = Dummy.new
+      end
+
+      context "that passes" do
+        let!(:required_content_type) { "image/png" }
+        before { rebuild(required_content_type) }
+        let(:fake_file) {  StringIO.new(".").tap { |s| s.stubs(:to_tempfile).returns(s) } }
+
+        it "calls all callbacks when assigned" do
+          @dummy.expects(:do_before_avatar).with()
+          @dummy.expects(:do_after_avatar).with()
+          @dummy.expects(:do_before_all).with()
+          @dummy.expects(:do_after_all).with()
+          Paperclip::Thumbnail.expects(:make).returns(fake_file)
+          @dummy.avatar = File.new(fixture_file("5k.png"))
+        end
+      end
+
+      context "that fails" do
+        let!(:required_content_type) { "image/jpeg" }
+        before { rebuild(required_content_type) }
+
+        it "does not call after callbacks when assigned" do
+          # before callbacks ARE still called at present
+          @dummy.expects(:do_before_all).with()
+          @dummy.expects(:do_before_avatar).with()
+
+          # But after_* are not
+          @dummy.expects(:do_after_avatar).never
+          @dummy.expects(:do_after_all).never
+          Paperclip::Thumbnail.expects(:make).never
+
+          @dummy.avatar = File.new(fixture_file("5k.png"))
+        end
+      end
+    end
+  end
+end

spec/paperclip/attachment_spec.rb

@@ -847,16 +847,16 @@ def do_after_all; end
       @dummy.expects(:do_before_avatar).never
       @dummy.expects(:do_after_avatar).never
       @dummy.expects(:do_before_all).with().returns(false)
-      @dummy.expects(:do_after_all)
+      @dummy.expects(:do_after_all).never
       Paperclip::Thumbnail.expects(:make).never
       @dummy.avatar = @file
     end
 
     it "cancels the processing if a before_avatar_post_process returns false" do
       @dummy.expects(:do_before_avatar).with().returns(false)
-      @dummy.expects(:do_after_avatar)
+      @dummy.expects(:do_after_avatar).never
       @dummy.expects(:do_before_all).with().returns(true)
-      @dummy.expects(:do_after_all)
+      @dummy.expects(:do_after_all).never
       Paperclip::Thumbnail.expects(:make).never
       @dummy.avatar = @file
     end