Returning false in around_* callback still firing after_* callbacks? #7127

Closed
jingoro opened this Issue Jul 22, 2012 · 6 comments

Projects

None yet

5 participants

@jingoro
jingoro commented Jul 22, 2012

When returning false in an around_* callback, this will halt the chain and prevent the method from being executed. However, after_* callbacks are still being triggered. This behavior does not occur with before_*.

Here is some example code:

require 'active_model'

class A
  extend ActiveModel::Callbacks
  define_model_callbacks :create
  def create; run_callbacks(:create) { p 'create A' }; end
  before_create { false }
  after_create { p 'not here' }
end

class B
  extend ActiveModel::Callbacks
  define_model_callbacks :create
  def create; run_callbacks(:create) { p 'create B' }; end
  around_create { false }
  after_create { p 'but here?' }
end

a = A.new; p a.create
b = B.new; p b.create

This results in:

false
"but here?"
nil

This was tested on both ActiveModel 3.2.6 and ActiveModel on master. Is this a bug or expected behavior?

See https://github.com/mongoid/mongoid/issues/2198

@MJIO
MJIO commented Aug 9, 2012

I'm not quite sure if this is an expected behavior. I only can find a single test, actually saying that the after callback shouldn't be executed:

https://github.com/rails/rails/blob/master/activemodel/test/cases/callbacks_test.rb#L60

@jingoro
jingoro commented Aug 9, 2012

My inclination is that this is a bug for a couple reasons:

  1. If you return false in a before callback, it stops execution (of save) and does not execute any after callbacks.
  2. Returning false in the around callback before yield does stop execution (of the save), and causes save to return to false. Thus, it would seem more natural that it would follow the same pattern of a before callback and not execute any after callbacks.
@kennyj kennyj added a commit to kennyj/rails that referenced this issue Dec 10, 2012
@kennyj kennyj Fix #7127. Don't call after_* when around_* returns false. 3d12ae2
@kennyj
Contributor
kennyj commented Dec 11, 2012

@jingoro I sent the above PR. Please confirm it.

@jingoro
jingoro commented Dec 11, 2012

@kennyj Just left a note on #8479 confirming the PR.

@jingoro jingoro added the stale label Apr 23, 2014
@rafaelfranca
Member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rafaelfranca rafaelfranca added attached PR and removed stale labels May 1, 2014
@rafaelfranca rafaelfranca self-assigned this May 1, 2014
@sgrif
Member
sgrif commented Jun 15, 2014
@sgrif sgrif closed this Jun 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment