callbacks execution order reversed in after_*

Confirmed on master. The after_ callback order get reversed:

class User < ActiveRecord::Base
  before_save :aa, :bb, :cc
  after_save :aa, :bb, :cc

  def aa; puts "a"; end
  def bb; puts "b"; end
  def cc; puts "c"; end
end

>> User.create!
a
b
c
c
b
a
=> #<User id: 1, created_at: "2010-09-19 12:57:03", updated_at: "2010-09-19 12:57:03">

In activesupport-3.0.0/lib/active_support/callbacks.rb
there is only one "reverse" (method "compile" in class "CallbackChain")

changing

reverse_each do |callback|

in

each do |callback|

the order in after_* seems to be preserved.

Have no idea why it was reverse_each in the first
place.

@Pietro

Now that you have the fix could you please attach a patch.

@Neeraj

Here is the patch.

I'll try to find in git history when the reverse_each
first appeared.

@Pietro Can you add a test please? Thanks.

I think the code came from

commit 21e7b8462113fc7c0fd1882dcc2bf7225de67b1a
Author: Joshua Peek
Date: Mon Oct 12 22:15:43 2009 -0500

Callbacks, DeprecatedCallbacks = NewCallbacks, Callbacks

Maybe Joshua can help since I have no experience
in testing activesuport :-)
I'll take a look to activesupport/test/callbacks_test.rb
for inspiration...

In commit http://github.com/rails/rails/commit/21e7b8462113fc7c0fd1882dcc2bf7... Johsua is refactoring the code. That commit did not introduce reverse_each .

@Pietro your patch is breaking a number of tests. If you look at ActiveSupport tests it seems the author intended after_* callbacks to be in reverse order.

I will ping someone from core team to have their input.

Actually, I was wrong. Because of the reverse_each, we need that prepend line. I will investigate deeper.

Btw, changing reverse_each is not the solution, as it will break controller callbacks.

Here is the fix:

http://github.com/rails/rails/blob/master/activesupport/lib/active_...

It should actually be:

options[:prepend] ? chain.unshift(*(mapped.reverse)) : chain.push(*mapped)

So when reverse_each is called the order is properly kept.

Anyone in mood for a patch with tests?

José Valim: That did not fix it.

I have a failing test. And it is still failing :-(

I will look into it more in the evening.

Neeraj, can you attach the failing test here? This way I can debug as well!

Neeraj, by default, after callbacks in AS::Callbacks are executed like in controller: the ones added first are executed later. We can change this by passing :prepend as option to set_callback, as here:

http://github.com/rails/rails/blob/master/activemodel/lib/active_mo...

The test case you added is in AS::Callbacks, which does not set prepend and therefore behaves as controllers. I believe that if you move the test case you added to ActiveModel::Callbacks, it will probably pass!

I can try that in the evening.

However that raises the question of after_save behaving differently in controller and model. Is this inconsistency really desired?

Unfortunately this inconsistency exists since early versions of Rails. Notice that ActiveRecord in Rails 3 behaves exactly as in ActiveRecord in 2.3, the only exception is when you pass more than one parameter to after_save.

So this currently works as in Rails 2.3:

after_save :aa
after_save :bb
after_save :cc

The only case which is broken (the one discussed in this ticket) is:

after_save :aa, :bb, :cc

(from [9e32b1cbe5fca3965919c7a604ee92426bf2bf5e]) after_create in ActiveModel should in the order specified

[#5650 callbacks execution order reversed in after_* state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/9e32b1cbe5fca3965919c7a604ee92...