Stopping a callback chain should require an explicit method call

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

:wink:

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…

http://www.amazon.com/Design-Everyday-Things-Donald-Norman/dp/0385267746 :stuck_out_tongue:

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: