#5650 callbacks execution order reversed in after_* - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5650 ✓resolved

callbacks execution order reversed in after_*

Reported by Pietro | September 18th, 2010 @ 12:30 AM | in 3.0.2

In rails 3.0.0 the callbacks "after_*" executes
the methods in reverse order.

If in a model you have

before_save :aa, :bb
after_save :aa, :bb

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

saving the object you get

a
b
b
a

In rails 2.x the result was

a
b
a
b

Comments and changes to this ticket

Jeff Kreeftmeijer September 19th, 2010 @ 01:58 PM

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">

Neeraj Singh September 19th, 2010 @ 03:38 PM
- Assigned user set to “Neeraj Singh”
- Importance changed from “” to “Low”
Looking into it.
You flagged this item as spam.
Pietro September 19th, 2010 @ 06:49 PM
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.
Neeraj Singh September 19th, 2010 @ 06:58 PM
@Pietro

Now that you have the fix could you please attach a patch.
You flagged this item as spam.
Pietro September 19th, 2010 @ 07:36 PM
@Neeraj

Here is the patch.

I'll try to find in git history when the reverse_each
first appeared.
- fix_callbacks_order.diff 789 Bytes
Neeraj Singh September 19th, 2010 @ 07:38 PM
@Pietro Can you add a test please? Thanks.
You flagged this item as spam.
Pietro September 19th, 2010 @ 07:59 PM
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...
Neeraj Singh September 20th, 2010 @ 01:40 AM
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.
Neeraj Singh September 20th, 2010 @ 01:41 AM
- Assigned user changed from “Neeraj Singh” to “José Valim”
Assigning it to Mr. Valim to get his input.
José Valim September 24th, 2010 @ 11:37 AM
- Milestone cleared.
- State changed from “new” to “open”
- Importance changed from “Low” to “High”
The problem is this option forcing callbacks to be prepended:

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

Removing this line should fix it. :)
José Valim September 25th, 2010 @ 10:09 AM
Actually, I was wrong. Because of the reverse_each, we need that prepend line. I will investigate deeper.
José Valim September 25th, 2010 @ 10:10 AM
Btw, changing reverse_each is not the solution, as it will break controller callbacks.
José Valim September 25th, 2010 @ 10:12 AM
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?
Neeraj Singh September 25th, 2010 @ 05:07 PM
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.
José Valim September 25th, 2010 @ 05:08 PM
Neeraj, can you attach the failing test here? This way I can debug as well!
Neeraj Singh September 25th, 2010 @ 05:12 PM
- no changes were found...
- 5650_fix_for_edge.diff 2 KB
José Valim September 25th, 2010 @ 05:17 PM
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!
Neeraj Singh September 25th, 2010 @ 05:19 PM
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?
José Valim September 25th, 2010 @ 05:31 PM
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
```
Neeraj Singh September 25th, 2010 @ 11:39 PM
- Tag set to “patched”
Attached is patch with test. All the hard work for this patch was done by Mr. Valim
- 5650_fix_for_edge.diff 2.2 KB
Repository September 27th, 2010 @ 10:12 PM
- State changed from “open” to “resolved”
(from [72c1e19c33ca06008d5d64619a533bdf34e3fc2b]) after_create in ActiveModel should in the order specified

[#5650 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/72c1e19c33ca06008d5d64619a533b...
Repository September 27th, 2010 @ 10:22 PM
(from [9e32b1cbe5fca3965919c7a604ee92426bf2bf5e]) after_create in ActiveModel should in the order specified

[#5650 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/9e32b1cbe5fca3965919c7a604ee92...
You flagged this item as spam.
Andrea Campi October 11th, 2010 @ 07:23 AM
- Tag changed from “patched” to “patch”
bulk tags cleanup
Jeremy Kemper October 15th, 2010 @ 11:02 PM
- Milestone set to “3.0.2”
[bulk edit]
Ryan Bigg October 16th, 2010 @ 02:28 AM
- Tag changed from “sheepskin boots, patch” to “patch”
Automatic cleanup of spam.