Hello all, I''ve been working with the action_controller filter code lately and I''ve noticed a lot of code repetition and a lot of room for improvement without breaking existing filter usage. I plan on submitting a patch that would clean this all up and add a few additional features. The main goal I am going for with the filter code is enabling "true" around filters: A filter method that may run whatever it wants before or after the action, including running the action within a block. This becomes incredibly useful when you talk about something like this: def filter(controller) if session_user? Data.with_scope( :find => { :conditions => ["user_id = ?", controller.session[:user_id] ] } ) do yield # this runs the action (or subsequent filters) making sure that all finds for Data require that the use is an owner. end With this sort of syntax for filters, you can completely drop the idea of "before filters" and "after filters." With scoping this easily lends itself to the simplified CRUD controllers mentioned by DHH in his keynote speech at RailsConf by keeping the controllers simple. I think that filters and their supporting methods should be scoped out of the controller. Adding a bunch of methods to the controller that aren''t actions and don''t assist actions isn''t very clean: I picture one of thefollowing uses being the most useful. add_filter :filter => ProjectScope.new, :except => [:list,:index] class ProjectScope def filter(controller) Project.with_scope( ..etc.. ) yield end end end add_filter :filter => :project_scope, :except => [:list, :index] def project_scope yield # this way drops the clean filter scope in favor of the simplicity of using controller methods. end I would also like to enable an add_filter call, passing a block, but the only way to allow that method to use yield is to instance_eval (any input on that would be good) What I want from this list, if I could, is some opinions on things you wish filters did. Anything you think would simplify filters, etc. Something that would be cool is to define the types of filter usage and just cater to each type of usage. We already have ''verify'' which is just a before_filter that checks things like params and request.method. Maybe we can think of a way to express any other usages that are comon, providing a simple way to do the most comon filters. Right now I can think of these cases. Please add any you can think of. * validate or restrict access based on session * scope or limit access based on params * setup variables for usage in the actions or views. * run methods that are called every time a action is called * update record keeping objects Sorry for the super long mail. I appreciate any input. -Marty _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
I''ve definitely had a need for more improved around and after filters. If an action is cached, any after or around filters will not execute as processing is basically halted after the cached page is served. It would be nice to have filters that are guaranteed to run regardless of whatever happens during the execution of the action. Is this something that you would be addressing with your improvements? On 7/2/06, Martin Emde <martin.emde@gmail.com> wrote:> Hello all, > > I''ve been working with the action_controller filter > code lately and I''ve noticed a lot of code repetition and a > lot of room for improvement without breaking existing filter usage. I plan > on submitting a patch that would clean this all up and add a few additional > features. > > The main goal I am going for with the filter code is enabling "true" around > filters: A filter method that may run whatever it wants before or after the > action, including running the action within a block. This becomes incredibly > useful when you talk about something like this: > > def filter(controller) > if session_user? > Data.with_scope( :find => { :conditions => ["user_id = ?", > controller.session[:user_id] ] } ) do > yield # this runs the action (or subsequent filters) making sure that > all finds for Data require that the use is an owner. > end > > With this sort of syntax for filters, you can completely drop the idea of > "before filters" and "after filters." With scoping this easily lends itself > to the simplified CRUD controllers mentioned by DHH in his keynote speech at > RailsConf by keeping the controllers simple. > > I think that filters and their supporting methods should be scoped out of > the controller. Adding a bunch of methods to the > controller that aren''t actions and don''t > assist actions isn''t very clean: > > I picture one of thefollowing uses being the most useful. > > add_filter :filter => ProjectScope.new, :except => [:list,:index] > class ProjectScope > def filter(controller) > Project.with_scope( ..etc.. ) > yield > end > end > end > > add_filter :filter => :project_scope, :except => [:list, :index] > def project_scope > yield > # this way drops the clean filter scope in favor of > the simplicity of using controller methods. > end > > I would also like to enable an add_filter call, passing a block, but the > only way to allow that method to use yield is to instance_eval (any input on > that would be good) > > What I want from this list, if I could, is some opinions on things you wish > filters did. Anything you think would simplify filters, etc. > > Something that would be cool is to define the types of filter usage and just > cater to each type of usage. We already have ''verify'' which is just a > before_filter that checks things like params and request.method. Maybe we > can think of a way to express any other > usages that are comon, providing a simple > way to do the most comon filters. Right now I can think of > these cases. Please add any you can think of. > > * validate or restrict access based on session > * scope or limit access based on params > * setup variables for usage in the actions or views. > * run methods that are called every time a action is called > * update record keeping objects > > Sorry for the super long mail. I appreciate any input. > > -Marty > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >
The way I want these filters to work is that every filter is a block that yields the next filter or action. That would mean that unless the action raises an exception, it should still go back thru all the filter methods after returning from each yield.. I''ll try to make sure this is going to work, I think it should be doable. On 7/2/06, Andrew Kaspick <akaspick@gmail.com> wrote:> > I''ve definitely had a need for more improved around and after filters. > If an action is cached, any after or around filters will not execute > as processing is basically halted after the cached page is served. > > It would be nice to have filters that are guaranteed to run regardless > of whatever happens during the execution of the action. > > Is this something that you would be addressing with your improvements? > > On 7/2/06, Martin Emde <martin.emde@gmail.com> wrote: > > Hello all, > > > > I''ve been working with the action_controller filter > > code lately and I''ve noticed a lot of code repetition and a > > lot of room for improvement without breaking existing filter usage. I > plan > > on submitting a patch that would clean this all up and add a few > additional > > features. > > > > The main goal I am going for with the filter code is enabling "true" > around > > filters: A filter method that may run whatever it wants before or after > the > > action, including running the action within a block. This becomes > incredibly > > useful when you talk about something like this: > > > > def filter(controller) > > if session_user? > > Data.with_scope( :find => { :conditions => ["user_id = ?", > > controller.session[:user_id] ] } ) do > > yield # this runs the action (or subsequent filters) making sure > that > > all finds for Data require that the use is an owner. > > end > > > > With this sort of syntax for filters, you can completely drop the idea > of > > "before filters" and "after filters." With scoping this easily lends > itself > > to the simplified CRUD controllers mentioned by DHH in his keynote > speech at > > RailsConf by keeping the controllers simple. > > > > I think that filters and their supporting methods should be scoped out > of > > the controller. Adding a bunch of methods to the > > controller that aren''t actions and don''t > > assist actions isn''t very clean: > > > > I picture one of thefollowing uses being the most useful. > > > > add_filter :filter => ProjectScope.new, :except => [:list,:index] > > class ProjectScope > > def filter(controller) > > Project.with_scope( ..etc.. ) > > yield > > end > > end > > end > > > > add_filter :filter => :project_scope, :except => [:list, :index] > > def project_scope > > yield > > # this way drops the clean filter scope in favor of > > the simplicity of using controller methods. > > end > > > > I would also like to enable an add_filter call, passing a block, but the > > only way to allow that method to use yield is to instance_eval (any > input on > > that would be good) > > > > What I want from this list, if I could, is some opinions on things you > wish > > filters did. Anything you think would simplify filters, etc. > > > > Something that would be cool is to define the types of filter usage and > just > > cater to each type of usage. We already have ''verify'' which is just a > > before_filter that checks things like params and request.method. Maybe > we > > can think of a way to express any other > > usages that are comon, providing a simple > > way to do the most comon filters. Right now I can think of > > these cases. Please add any you can think of. > > > > * validate or restrict access based on session > > * scope or limit access based on params > > * setup variables for usage in the actions or views. > > * run methods that are called every time a action is called > > * update record keeping objects > > > > Sorry for the super long mail. I appreciate any input. > > > > -Marty > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 2-jul-2006, at 22:49, Andrew Kaspick wrote:> I''ve definitely had a need for more improved around and after filters. > If an action is cached, any after or around filters will not execute > as processing is basically halted after the cached page is served.The problem here is that there is no assurance that an after-filter will run. That way scoping sometimes propagates through all requests via the class variables. Basically it seems that it would be a nice idea to have around filters which are wrapped in begin/ensure so that the around filter is always fired for both before and after. I looked at how AP handles that and it might be somewhat cumbersome to implement. -- Julian ''Julik'' Tarkhanov please send all personal mail to me at julik.nl
Well, if you use the new around filter that I''m proposing you won''t have a "before action" and "after action" you''ll just have one method that yields the perform_action for the controller. This way it''s unavoidable (without an exception being raised) for the code not to be run. For example you would make a filter that acts like a current "around" filter. (I would call it a "before and after" filter) def filter(controller) if stuff_that_would_normally_be_a_before_action(args) yield # runs the action (or the following filter) stuff_that_would_normally_be_an_after_action else # cleanup if before_action stuff failed end end This way when the action returns, it should run. If this is not the case, please let me know a test case I could use to make sure this will work in the new system. -Martin On 7/2/06, Julian ''Julik'' Tarkhanov <listbox@julik.nl> wrote:> > > On 2-jul-2006, at 22:49, Andrew Kaspick wrote: > > > I''ve definitely had a need for more improved around and after filters. > > If an action is cached, any after or around filters will not execute > > as processing is basically halted after the cached page is served. > > The problem here is that there is no assurance that an after-filter > will run. That way scoping sometimes propagates > through all requests via the class variables. Basically it seems that > it would be a nice idea to have around filters which are wrapped > in begin/ensure so that the around filter is always fired for both > before and after. > > I looked at how AP handles that and it might be somewhat cumbersome > to implement. > -- > Julian ''Julik'' Tarkhanov > please send all personal mail to > me at julik.nl > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Martin- Take a look at this plugin that just came out. I think it does what you are looking for as far as wrapping actions and yielding to them. Even allows for a chain of such filters. http://roman2k.free.fr/rails/meantime_filter/0.1.0/rdoc/ -Ezra On Jul 2, 2006, at 4:27 PM, Martin Emde wrote:> Well, if you use the new around filter that I''m proposing you won''t > have a "before action" and "after action" you''ll just have one > method that yields the perform_action for the controller. This way > it''s unavoidable (without an exception being raised) for the code > not to be run. > > For example you would make a filter that acts like a current > "around" filter. (I would call it a "before and after" filter) > > def filter(controller) > if stuff_that_would_normally_be_a_before_action(args) > yield # runs the action (or the following filter) > stuff_that_would_normally_be_an_after_action > else > # cleanup if before_action stuff failed > end > end > > This way when the action returns, it should run. If this is not the > case, please let me know a test case I could use to make sure this > will work in the new system. > > -Martin > > On 7/2/06, Julian ''Julik'' Tarkhanov <listbox@julik.nl> wrote: > > On 2-jul-2006, at 22:49, Andrew Kaspick wrote: > > > I''ve definitely had a need for more improved around and after > filters. > > If an action is cached, any after or around filters will not execute > > as processing is basically halted after the cached page is served. > > The problem here is that there is no assurance that an after-filter > will run. That way scoping sometimes propagates > through all requests via the class variables. Basically it seems that > it would be a nice idea to have around filters which are wrapped > in begin/ensure so that the around filter is always fired for both > before and after. > > I looked at how AP handles that and it might be somewhat cumbersome > to implement. > -- > Julian ''Julik'' Tarkhanov > please send all personal mail to > me at julik.nl > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Great. Good to see people agree with the idea. I still think there''s a lot of improvement to be made in the filter code so I will continue making this patch. I also wanted to see what people''s preference was for yielding the action from a filter. The problem with yield is that yield from a block passed to meantime_filter (or whatever you want to call it) will call the block passed to the surrounding method at the time the Proc is created. This means the filter has to use block.call when it''s passed as a Proc, but can call yield when it''s done as a method or an object responding to filter. It would be nice to eliminate this inconsitency. An alternate option to yield would be controller.filter_chain.next to call the next filter/action. Yield may make more sense in some cases, but when you''re forced to do a proc like { |controller, block| block.call } sometimes and def filter(controller); yield; end; other times it may cause confusion. What seems less confusing to you guys? -Martin On 7/2/06, Ezra Zygmuntowicz <ezmobius@gmail.com> wrote:> > Martin- > Take a look at this plugin that just came out. I think it does what you > are looking for as far as wrapping actions and yielding to them. Even allows > for a chain of such filters. > > http://roman2k.free.fr/rails/meantime_filter/0.1.0/rdoc/ > > -Ezra > > On Jul 2, 2006, at 4:27 PM, Martin Emde wrote: > > Well, if you use the new around filter that I''m proposing you won''t have a > "before action" and "after action" you''ll just have one method that yields > the perform_action for the controller. This way it''s unavoidable (without an > exception being raised) for the code not to be run. > > For example you would make a filter that acts like a current "around" > filter. (I would call it a "before and after" filter) > > def filter(controller) > if stuff_that_would_normally_be_a_before_action(args) > yield # runs the action (or the following filter) > stuff_that_would_normally_be_an_after_action > else > # cleanup if before_action stuff failed > end > end > > This way when the action returns, it should run. If this is not the case, > please let me know a test case I could use to make sure this will work in > the new system. > > -Martin > > On 7/2/06, Julian ''Julik'' Tarkhanov <listbox@julik.nl> wrote: > > > > > > On 2-jul-2006, at 22:49, Andrew Kaspick wrote: > > > > > I''ve definitely had a need for more improved around and after filters. > > > If an action is cached, any after or around filters will not execute > > > as processing is basically halted after the cached page is served. > > > > The problem here is that there is no assurance that an after-filter > > will run. That way scoping sometimes propagates > > through all requests via the class variables. Basically it seems that > > it would be a nice idea to have around filters which are wrapped > > in begin/ensure so that the around filter is always fired for both > > before and after. > > > > I looked at how AP handles that and it might be somewhat cumbersome > > to implement. > > -- > > Julian ''Julik'' Tarkhanov > > please send all personal mail to > > me at julik.nl > > > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
I would tend to think { |controller, block| block.call } would more clear, especially to people new to the framework and ruby itself. On 7/5/06, Martin Emde <martin.emde@gmail.com> wrote:> Great. Good to see people agree with the idea. > > I still think there''s a lot of improvement to be made in the filter code so > I will continue making this patch. > > I also wanted to see what people''s preference was for yielding the action > from a filter. The problem with yield is that yield from a block passed to > meantime_filter (or whatever you want to call it) will call the block passed > to the surrounding method at the time the Proc is created. This means the > filter has to use block.call when it''s passed as a Proc, but can call yield > when it''s done as a method or an object responding to filter. It would be > nice to eliminate this inconsitency. > > An alternate option to yield would be > controller.filter_chain.next > to call the next filter/action. > > Yield may make more sense in some cases, but when you''re forced to do a proc > like { |controller, block| block.call } sometimes and def > filter(controller); yield; end; other times it may cause confusion. > > What seems less confusing to you guys? > > -Martin > > > On 7/2/06, Ezra Zygmuntowicz <ezmobius@gmail.com > wrote: > > > > > > Martin- > > > > > > Take a look at this plugin that just came out. I think it does what you > are looking for as far as wrapping actions and yielding to them. Even allows > for a chain of such filters. > > > > > > http://roman2k.free.fr/rails/meantime_filter/0.1.0/rdoc/ > > > > > > > > -Ezra > > > > > > > > > > On Jul 2, 2006, at 4:27 PM, Martin Emde wrote: > > > > Well, if you use the new around filter that I''m proposing you won''t have a > "before action" and "after action" you''ll just have one method that yields > the perform_action for the controller. This way it''s unavoidable (without an > exception being raised) for the code not to be run. > > > > For example you would make a filter that acts like a current "around" > filter. (I would call it a "before and after" filter) > > > > def filter(controller) > > if stuff_that_would_normally_be_a_before_action(args) > > yield # runs the action (or the following filter) > > stuff_that_would_normally_be_an_after_action > > else > > # cleanup if before_action stuff failed > > end > > end > > > > This way when the action returns, it should run. If this is not the case, > please let me know a test case I could use to make sure this will work in > the new system. > > > > -Martin > > > > > > On 7/2/06, Julian ''Julik'' Tarkhanov < listbox@julik.nl> wrote: > > > > > > On 2-jul-2006, at 22:49, Andrew Kaspick wrote: > > > > > > > I''ve definitely had a need for more improved around and after filters. > > > > If an action is cached, any after or around filters will not execute > > > > as processing is basically halted after the cached page is served. > > > > > > The problem here is that there is no assurance that an after-filter > > > will run. That way scoping sometimes propagates > > > through all requests via the class variables. Basically it seems that > > > it would be a nice idea to have around filters which are wrapped > > > in begin/ensure so that the around filter is always fired for both > > > before and after. > > > > > > I looked at how AP handles that and it might be somewhat cumbersome > > > to implement. > > > -- > > > Julian ''Julik'' Tarkhanov > > > please send all personal mail to > > > me at julik.nl > > > > > > > > > _______________________________________________ > > > Rails-core mailing list > > > Rails-core@lists.rubyonrails.org > > > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >