Rails core - Oct 2006 - 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.


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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.

On 10/27/06, richcollins@gmail.com <richcollins@gmail.com>
wrote:
>
> 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.
>
>
> >
>

-- 
* J *
  ~

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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

On Oct 27, 2006, at 1:53 AM, James Adam wrote:
>
> 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.
>
> On 10/27/06, richcollins@gmail.com <richcollins@gmail.com> wrote:
>>
>> 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.
>>
>>
>>>
>>
>
>
> -- 
> * J *
>   ~
>
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins
wrote:
> "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
-- 
Marcel Molina Jr. <marcel@vernix.org>

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

Hi,

2006/10/27, Marcel Molina Jr. <marcel@vernix.org>:
> On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins 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

Bye !
-- 
François Beausoleil
http://blog.teksol.info/
http://piston.rubyforge.org/

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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.

On Oct 27, 2006, at 10:15 AM, Francois Beausoleil wrote:
> Hi,
>
> 2006/10/27, Marcel Molina Jr. <marcel@vernix.org>:
>> On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins 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
>
> Bye !
> -- 
> François Beausoleil
> http://blog.teksol.info/
> http://piston.rubyforge.org/
>
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

On 10/28/06, Rich Collins <richcollins@gmail.com>
wrote:
>
> 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
-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

> 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

;)
-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

I personally don''t mind having a return of false stop the callback
chain. I guess I''m used to having functions return an status / error
code as the return value (C habit?).

What is trickier is the fact that ruby automatically returns the last
value evaluated. Anyway, I guess I got used to the way callbacks work.
I haven''t really had any trouble with them recently.


-carl

On 10/27/06, Michael Koziarski <michael@koziarski.com>
wrote:
>
> > 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
>
> ;)
> --
> Cheers
>
> Koz
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

On 10/27/06, Carl Lerche <carl.lerche@gmail.com>
wrote:
>
>
> What is trickier is the fact that ruby automatically returns the last
> value evaluated. Anyway, I guess I got used to the way callbacks work.
> I haven''t really had any trouble with them recently.
>
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


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

On 10/27/06, Martin Emde <martin.emde@gmail.com>
wrote:
>
>
> 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.

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


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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

On Oct 27, 2006, at 6:15 PM, Nicholas Seckar wrote:
>
>
> On 10/27/06, Martin Emde <martin.emde@gmail.com> wrote:
>
> 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.
>
> 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...
>
>
> >


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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:
> http://www.amazon.com/Design-Everyday-Things-Donald-Norman/dp/
> 0385267746 :P
>
> On Oct 27, 2006, at 6:15 PM, Nicholas Seckar wrote:
>
> >
> >
> > On 10/27/06, Martin Emde <martin.emde@gmail.com> wrote:
> >
> > 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.
> >
> > 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...
> >
> >
> > >
>
>
> --Apple-Mail-3--992351814
> Content-Type: text/html
> Content-Transfer-Encoding: quoted-printable
> X-Google-AttachSize: 1338
>
> <HTML><BODY style=3D"word-wrap: break-word; -khtml-nbsp-mode:
space; -khtml> -line-break: after-white-space; "><A
href=3D"http://www.amazon.com/Design-E>
veryday-Things-Donald-Norman/dp/0385267746">http://www.amazon.com/Design-Ev>
eryday-Things-Donald-Norman/dp/0385267746</A>
:P<DIV><BR><DIV><DIV>On Oct 2> 7, 2006, at 6:15 PM,
Nicholas Seckar wrote:</DIV><BR class=3D"Apple-interch>
ange-newline"><BLOCKQUOTE
type=3D"cite"><BR><BR><DIV><SPAN
class=3D"gmail_q> uote">On 10/27/06, <B
class=3D"gmail_sendername">Martin Emde</B> &lt;<A
hre>
f=3D"mailto:martin.emde@gmail.com">martin.emde@gmail.com</A>&gt;
wrote:</SP> AN><BLOCKQUOTE class=3D"gmail_quote"
style=3D"border-left: 1px solid rgb(20> 4, 204, 204); margin: 0pt 0pt
0pt 0.8ex; padding-left: 1ex;"> <SPAN
class=3D"q"><SPAN
class=3D"gmail_quote"></SPAN><BR></SPAN> A ruby
programmer tha> t doesn''t pay attention to his/her implicit return
value will probably run > into a lot more problems than just
this.</BLOCKQUOTE><DIV><BR> Pretending t> hat this
isn''t an easy trap to fall into won''t solve the issue.
I''ve made t> his mistake myself, as have other core members.
It''s entirely too easy to a> ccidentally halt chain execution in a
non-obvious way. <BR><BR>I''m a fan of>  the throw idea
Marcel mentioned, and I''d be pleased if it happened for 2.0>
...<BR></DIV><BR></DIV> <BR>=20

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

Francois Beausoleil wrote:
> 2006/10/27, Marcel Molina Jr. <marcel@vernix.org>:
> > On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins 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


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---

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

On Oct 28, 2006, at 3:02 AM, Chris Mear wrote:
>
> Francois Beausoleil wrote:
>> 2006/10/27, Marcel Molina Jr. <marcel@vernix.org>:
>>> On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins 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
>
>
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google  
> Groups "Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com
> To unsubscribe from this group, send email to rubyonrails-core- 
> unsubscribe@googlegroups.com
> For more options, visit this group at http://groups.google.com/ 
> group/rubyonrails-core
> -~----------~----~----~----~------~----~------~--~---
>

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:
> 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
>
> On Oct 28, 2006, at 3:02 AM, Chris Mear wrote:
>
> >
> > Francois Beausoleil wrote:
> >> 2006/10/27, Marcel Molina Jr. <marcel@vernix.org>:
> >>> On Fri, Oct 27, 2006 at 09:11:04AM -0700, Rich Collins 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
> >
> >
> > > >
>
>
> --Apple-Mail-21--944530680
> Content-Type: application/pkcs7-signature
> Content-Transfer-Encoding: base64
> Content-Disposition: inline;
> 	filename="smime.p7s"
> X-Google-AttachSize: 3272

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---