It is far too easy to forget that returning false stops the subsequent callbacks. I was just bitten by this when creating a callback that sets an attribute (sometimes to false). An explicit method call would be a good constraint against accidental callback cancellation.
Requiring an explicit call would prohibit using methods or expressions which are already floating about in the Rubysphere (which evaluate to true/false) as filters, without wrapping them in another method. Crufty. Since the outcome is binary (either continue with the action, or don't), returning true or false works well, IMHO.
It's OK to forget or get bitten by this a few times; I do, and I'm sure everyone else does too. With a relatively good set of functional/integration tests you'll catch any slips before they become problematic.
"It's OK to forget or get bitten by this a few times"
That is not a good design philosophy.
Stopping the chain by returning false violates the principle of least
surprise. I think a stop_chain method should be added and there
should be a deprecation warning for filters returning false.
This has come up. A while ago we had decided that throw/catch might be a good api for this. Something like:
def some_filter # ... throw :halt end
But a change like that was deemed 2.0 (or even 3.0) territory.
marcel
Hi,
OK. Just wondering if it had been considered. I'm not sure why
throw was suggested instead of an explicit method call that sets the
state of the callback chain.
Either way, it's an intrusive change. It would break every app which used callbacks. So it's definitely 1.0 territory
Either way, it's an intrusive change. It would break every app which used callbacks. So it's definitely 1.0 territory
2.0, 2.0
A ruby programmer that doesn’t pay attention to his/her implicit return value will probably run into a lot more problems than just this.
I agree with Carl and the others above, it’s just a part of Ruby and has been used efficiently in Rails to keep callbacks simple.
Martin
Pretending that this isn’t an easy trap to fall into won’t solve the issue. I’ve made this mistake myself, as have other core members. It’s entirely too easy to accidentally halt chain execution in a non-obvious way.
I’m a fan of the throw idea Marcel mentioned, and I’d be pleased if it happened for 2.0…
Callbacks are there to control the life cycle of model objects. Such as validation, mapping column values, and preventing operations from completing. The current implementation makes perfect sense in the context of "controlling the life cycle of model objects"; where returning false stops the callback chain. IMHO.
Carlos Gabaldon http://www.blog.notesonrails.com/
Rich Collins wrote:
Francois Beausoleil wrote:
> > "It's OK to forget or get bitten by this a few times" > > This has come up. A while ago we had decided that throw/catch might be a good > api for this. Something like: > > def some_filter > # ... > throw :halt > end > > But a change like that was deemed 2.0 (or even 3.0) territory.
I was the instigator of that change. Jeremy Kemper said "throwing is not a good API": http://dev.rubyonrails.org/ticket/2241
Yeah, I always taught only to use exceptions to handle genuine error cases, not for regular flow control.
The problem here is that people are triggering the cancellation accidentally by unknowingly returning false, right? I might be missing something really obvious (I usually am), but couldn't we just change it so that you have to return some really specific symbol to halt the callbacks, like
return :halt
Your intent there is as clear as 'throw :halt'. And it would guard against random "false"s falling through Ruby's implicit method return and cancelling the rest of the callback chain.
It would still break old code, but at least it's not a complete sea-change in the grammar of the callback system.
Chris
Note that the semantics of "throw" differ from the semantics of "raise". Raise is for exceptions. Throw is for rewinding the stack in non-exceptional circumstances. In fact, this use of throw is a perfect example of how throw can be legitimately used.
Me, I like how throw works in this case. I'd be for it.
- Jamis
See, I said I was probably missing something really obvious. My brain was so sure you were all talking about 'raise'. Thanks for clearing it up.
Chris
Jamis Buck wrote:
