Rails core - Oct 2007 - Halting filters with render/redirect instead of returning false

We''ve been wanting to deal with the fact that filters are too easy to
inadvertently halt when the last statement returns false for some
reason. A few suggestions were discussed, like throwing symbols, but
after a thorough inspection of a ton of real filters in real
applications, we realized that we already have a pattern: When a
filter renders or redirects, it''s always followed by returning false.

After spotting this, it seems like the return false part is redundant
and that we instead could just assume that a filter wants to halt if
performed? is true after it has run.

But before we make this happen for Rails 2.0, I thought it would be a
good idea to hear if there''s any reasonable claims as to when
you''d
want to either render/redirect, but still allow the chain to continue,
or do neither and still want halting. Emphasis is on reasonable, I
don''t mind obscure edge cases breaking. This is 2.0, after all.


--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On 10/21/07, DHH <david.heinemeier@gmail.com>
wrote:
>
>
> After spotting this, it seems like the return false part is redundant
> and that we instead could just assume that a filter wants to halt if
> performed? is true after it has run.

This makes sense to me also. I can''t imagine a scenario when I render
or
redirect in a filter where it makes sense to continue with the action.

Does positive feedback for this change mean you''ll consider dropping
the old
way of halting filters? Personally I''ve never felt that
"false" was a bad
value and I liked that it was chosen for halting. I don''t think it has
ever
inadvertently halted the chain for me -- really, what are the chances of
putting a method that returns boolean as the last statement in a filter?

--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

This is a good move, and I feel ActiveRecord before_* filters'' support
for "false" has bit people just as often.

On Oct 20, 4:43 pm, DHH <david.heineme...@gmail.com>
wrote:
> We''ve been wanting to deal with the fact that filters are too easy
to
> inadvertently halt when the last statement returns false for some
> reason. A few suggestions were discussed, like throwing symbols, but
> after a thorough inspection of a ton of real filters in real
> applications, we realized that we already have a pattern: When a
> filter renders or redirects, it''s always followed by returning
false.
>
> After spotting this, it seems like the return false part is redundant
> and that we instead could just assume that a filter wants to halt if
> performed? is true after it has run.
>
> But before we make this happen for Rails 2.0, I thought it would be a
> good idea to hear if there''s any reasonable claims as to when
you''d
> want to either render/redirect, but still allow the chain to continue,
> or do neither and still want halting. Emphasis is on reasonable, I
> don''t mind obscure edge cases breaking. This is 2.0, after all.

--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

> Does positive feedback for this change mean you''ll consider
dropping the old
> way of halting filters? Personally I''ve never felt that
"false" was a bad
> value and I liked that it was chosen for halting. I don''t think it
has ever
> inadvertently halted the chain for me -- really, what are the chances of
> putting a method that returns boolean as the last statement in a filter?
Returning false will no longer halt the chain. It never really made
any sense to halt the chain without rendering or redirecting, it only
lead to White Screens of Death. Plenty of people were bitten by it.

This is now fact from http://dev.rubyonrails.org/changeset/7984 and
forth.


--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On Oct 21, 2007, at 3:01 PM, DHH wrote:
> Returning false will no longer halt the chain. It never really made
> any sense to halt the chain without rendering or redirecting, it only
> lead to White Screens of Death. Plenty of people were bitten by it.
What happens to old code that goes:

    redirect_to foo_url and return if foo.nil?

Does it break? Or does it now work just like:

    redirect_to foo_url if foo.nil?

?

--
	Jordi

> What happens to old code that goes:
>
>     redirect_to foo_url and return if foo.nil?
>
> Does it break? Or does it now work just like:
>
>     redirect_to foo_url if foo.nil?
>
Because the original code redirects, the filter chain is halted.  The
return won''t do any harm, it will just have no effect.


-- 
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On Oct 20, 8:40 pm, "Mislav Marohnić"
<mislav.maroh...@gmail.com>
wrote:
> This makes sense to me also. I can''t imagine a scenario when I
render or
> redirect in a filter where it makes sense to continue with the action.
Will this change prevent this error from occurring anymore?

ActionController::DoubleRenderError

"Render and/or redirect were called multiple times in this action.
Please note that you may only call render OR redirect, and only once
per action. Also note that neither redirect nor render terminate
execution of the action, so if you want to exit an action after
redirecting, you need to do something like \"redirect_to(...) and
return\". Finally, note that to cause a before filter to halt
execution of the rest of the filter chain, the filter must return
false, explicitly, so \"render(...) and return false\"."

I always forget to add "and return false" because having a render or
redirect seems like it should halt the chain to me.

- Trevor


--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On 10/22/07, Trevor Turk <trevorturk@gmail.com>
wrote:
>
>
> Will this change prevent this error from occurring anymore?
>
> ActionController::DoubleRenderError

Trevor, that''s exactly what this change did. You won''t be
getting that error
anymore and you can safely remove "and return false" from your
filters.

--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On Oct 22, 12:09 pm, "Mislav Marohnić"
<mislav.maroh...@gmail.com>
wrote:
> Trevor, that''s exactly what this change did. You won''t be
getting that error
> anymore and you can safely remove "and return false" from your
filters.
Terrific. This seems like a smart change to me. I dig all of the
simplification and streamlining going on in this release.

If that error can''t occur anymore, shouldn''t it be removed
from Rails?
I didn''t see that it was taken out in the changeset linked above,
which is why I''m wondering. I see a few mentions of
"DoubleRenderError" in this file:

http://dev.rubyonrails.org/browser/trunk/actionpack/lib/action_controller/base.rb

I can open a ticket (or try to create a patch) if that''s more
appropriate.

- Trevor


--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---

On 10/22/07, Trevor Turk <trevorturk@gmail.com>
wrote:
>
>
> If that error can''t occur anymore, shouldn''t it be
removed from Rails?

Oh, it can occur all right, but not in the context that we were discussing
in this thread.

It still needs to catch errors caused by clumsy conditionals in a single
filter or action.

--~--~---------~--~----~------------~-------~--~----~
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?hl=en
-~----------~----~----~----~------~----~------~--~---