Fix #7127. Don't call after_* callbacks when around_* callbacks returns false.

[kennyj]

Please see #7127.

According to https://github.com/rails/rails/blob/master/activesupport/lib/active_support/callbacks.rb#L173-L179 and https://github.com/rails/rails/blob/master/activesupport/lib/active_support/callbacks.rb#L320, if we don't call a block, value is not assigned (nil) .

Thus I guess we call after_* callbacks incorrectly in the case.

[carlosantoniodasilva]

/cc @josevalim

[jingoro]

I was the original reporter of #7127. This PR looks good to me.

[dmitry]

/cc @rafaelfranca

[JonRowe]

Hey, is this still an issue on 4.1 @kennyj? Did this ever get a proper review? (/cc @rafaelfranca @carlosantoniodasilva )

[sgrif]

This would break existing applications which were previously calling methods that return false or nil, which is currently innocuous. Generally around blocks are meant for things like benchmarking, logging, transactions, etc -- and should be causing save to fail. That behavior should be in a before or after callback. If you really need to fail in an around callback, raise ActiveRecord::Rollback should do the trick.

[matthewd]

This sounds quite reasonable to me: if your 'around' callback fails to yield (i.e., elects to skip the operation it's wrapping), the action didn't occur, so we shouldn't bother with 'after' callbacks.

[sgrif]

Do around_action callbacks in AC do this, as well?

On Sun, Jun 15, 2014 at 4:54 PM, Matthew Draper notifications@github.com
wrote:

This sounds quite reasonable to me: if your 'around' callback fails to
yield (i.e., elects to skip the operation it's wrapping), the action didn't
occur, so we shouldn't bother with 'after' callbacks.

—
Reply to this email directly or view it on GitHub
#8479 (comment).

Thanks,
Sean Griffin

[rafaelfranca]

@claudiob do you think this is still necessary after your changes?

[claudiob]

@rafaelfranca Let me take a look. For clarity "my changes" means #17227

[rafaelfranca]

Yes

[claudiob]

@rafaelfranca This issue is not affected by #17227. In fact, I already noticed this behavior when working on that PR and wrote in #17227 (comment):

Until now, only before_ callbacks are able to halt the chain; the return value of after_ callbacks is ignored. If after_ callbacks are executed, then they are all executed.

With this PR, the behavior does not change. Therefore, if an after_ callback decided to throw(:abort), this would not be caught by terminator: it would simply bubble up to the user.

I think this okay, since it matches what the documentation says and existing code.

I just wanted to confirm.

to which @dhh replied:

Ultimately, I think you should be able to halt the after_* chain as well. But we don’t need to deal with it in the same PR.

[rafaelfranca]

Right, so lets add this to our TODO list.

I'll close this one since it is very likely that the implementation will change.

@kennyj thank you for the pull request.