New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controller Filters #443

Closed
activestylus opened this Issue Mar 7, 2011 · 22 comments

Comments

Projects
None yet
6 participants
@activestylus
Contributor

activestylus commented Mar 7, 2011

Sinatra's default before blocks leave much to be desired. In order to filter out certain actions you have to write your own regex parser, and it ain't pretty

# helper
  def before_only(routes, &block)
    before do
      routes.map!{|x| x = x.gsub(/\*/, '\w+')}
      routes_regex = routes.map{|x| x = x.gsub(/\//, '\/')}
      instance_eval(&block) if routes_regex.any? {|route| (request.path =~ /^#{route}$/) != nil}
    end
  end

# controller

before_only(['/people/new','/people/*/edit','/people/index']) do
  @person.addresses.new
end

get :new do
  ...
end

As opposed to Rails

before_filter :my_method, :only => [:new, :edit, :index]

Although keeping things in line with Sinatra's philosophy I could see that rewritten as

before(:new, :edit) do
  ...
end

I know the overall goal here is to keep the core light and magic-free, but these types of helpers are pretty explicit on their own, and supremely useful.

Thoughts?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 7, 2011

Member

Yeah I have always wanted Rails-y controller filters but have never gotten around to it yet. I would hope we can have these (or a separate gem that adds them i.e padrino-filters) before a 1.0 release. This would be a killer feature.

Member

nesquena commented Mar 7, 2011

Yeah I have always wanted Rails-y controller filters but have never gotten around to it yet. I would hope we can have these (or a separate gem that adds them i.e padrino-filters) before a 1.0 release. This would be a killer feature.

@activestylus

This comment has been minimized.

Show comment
Hide comment
@activestylus

activestylus Mar 8, 2011

Contributor

Okay thats great news. I'd dip in myself but still learning the padrino/sinatra ropes here.

Contributor

activestylus commented Mar 8, 2011

Okay thats great news. I'd dip in myself but still learning the padrino/sinatra ropes here.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 8, 2011

Member

I don't think it would be hard to do. If anyone reads this and wants to try and take a stab at it, I would be most appreciative :) But yeah i'll try to get around to it myself at some point.

Member

nesquena commented Mar 8, 2011

I don't think it would be hard to do. If anyone reads this and wants to try and take a stab at it, I would be most appreciative :) But yeah i'll try to get around to it myself at some point.

@glaville

This comment has been minimized.

Show comment
Hide comment
@glaville

glaville Mar 8, 2011

Sinatra supports patterns as parameters for filters (http://www.sinatrarb.com/intro#Filters).

The following code should allow your "Sinatra's philosophy" syntax (by overriding the Sinatra method in the "array passed as criteria" case) :

def add_filter(type, path = nil, options = {}, &block)
  if path.respond_to?(:each)
    path.each { |e| super(type, e, options, block) }
  else
    super
  end
end

A custom matcher seems however the way to go :
http://www.sinatrarb.com/intro.html#Custom%20Route%20Matchers (the previous code requires to be included at the class level, and not as an helper).

glaville commented Mar 8, 2011

Sinatra supports patterns as parameters for filters (http://www.sinatrarb.com/intro#Filters).

The following code should allow your "Sinatra's philosophy" syntax (by overriding the Sinatra method in the "array passed as criteria" case) :

def add_filter(type, path = nil, options = {}, &block)
  if path.respond_to?(:each)
    path.each { |e| super(type, e, options, block) }
  else
    super
  end
end

A custom matcher seems however the way to go :
http://www.sinatrarb.com/intro.html#Custom%20Route%20Matchers (the previous code requires to be included at the class level, and not as an helper).

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 17, 2011

Member

Thanks for the info. We already support controller level before filters (that only run in a given controller context). The additional extension makes a lot of sense.

Member

nesquena commented Mar 17, 2011

Thanks for the info. We already support controller level before filters (that only run in a given controller context). The additional extension makes a lot of sense.

@ghost ghost assigned joshbuddy and nesquena Apr 10, 2011

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 2, 2011

I needed this for a project and after peeking around the padrino and http_router code for awhile I was able to come up with a solution.

  1. Made Padrino::Routing::ClassMethods::current_controller a public method.
  2. Created a Custom Route Matcher.
    The route matcher is here; https://gist.github.com/1003798.

It seems like the route matcher is far messier than it needs to be. My knowledge of Padrino internals is very limited; I'm betting the route matcher can be cleaned up significantly by someone who knows what they're doing.

k2052 commented Jun 2, 2011

I needed this for a project and after peeking around the padrino and http_router code for awhile I was able to come up with a solution.

  1. Made Padrino::Routing::ClassMethods::current_controller a public method.
  2. Created a Custom Route Matcher.
    The route matcher is here; https://gist.github.com/1003798.

It seems like the route matcher is far messier than it needs to be. My knowledge of Padrino internals is very limited; I'm betting the route matcher can be cleaned up significantly by someone who knows what they're doing.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 2, 2011

Member

@bookworm Thanks for creating a working solution! @joshbuddy @skade What are your thoughts about the feasability of adding this feature (in terms of difficulty)?

Member

nesquena commented Jun 2, 2011

@bookworm Thanks for creating a working solution! @joshbuddy @skade What are your thoughts about the feasability of adding this feature (in terms of difficulty)?

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Jun 3, 2011

Contributor

I have some working code for this .. will commit very soon.

Contributor

joshbuddy commented Jun 3, 2011

I have some working code for this .. will commit very soon.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jun 5, 2011

Member

@joshbuddy thanks so much!

Member

DAddYE commented Jun 5, 2011

@joshbuddy thanks so much!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

Ok this appears to be in master. @DAddYE @achiu @bookworm @activestylus Can you guys test this and confirm? Examples of usage in this commit: 434c4be

Member

nesquena commented Jun 8, 2011

Ok this appears to be in master. @DAddYE @achiu @bookworm @activestylus Can you guys test this and confirm? Examples of usage in this commit: 434c4be

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 8, 2011

@nesquena I saw that this morning and tested, it does not work as of yet. Assumed @joshbuddy still had some work to do. What happens is that Padrino::Filter.apply? should be called to check the request and at least for now it doesn't get called.
@joshbuddy's updates to the routing system seem to have broken Custom Route Matchers on before filters; so it would be nice if controller filters made it into 0.9.30. If not, then I'll have to keep using a fork of 0.9.29 on my current project.

k2052 commented Jun 8, 2011

@nesquena I saw that this morning and tested, it does not work as of yet. Assumed @joshbuddy still had some work to do. What happens is that Padrino::Filter.apply? should be called to check the request and at least for now it doesn't get called.
@joshbuddy's updates to the routing system seem to have broken Custom Route Matchers on before filters; so it would be nice if controller filters made it into 0.9.30. If not, then I'll have to keep using a fork of 0.9.29 on my current project.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

What type of custom route matchers are broken? We should probably add tests for that. Can you give us a simple example?

Member

nesquena commented Jun 8, 2011

What type of custom route matchers are broken? We should probably add tests for that. Can you give us a simple example?

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 8, 2011

@nesquena Here you go https://gist.github.com/1013668 . This should match for any route but on padrino edge it doesn't match for anything.

k2052 commented Jun 8, 2011

@nesquena Here you go https://gist.github.com/1013668 . This should match for any route but on padrino edge it doesn't match for anything.

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 8, 2011

Oops my apologies the tests pass for me now. I think i had messed up a merge when I messing around with changes on my local copy. My changes had caused a failure. On a fresh clone the tests passed just fine.

However, there seems to a problem with mapped routes. https://gist.github.com/1013688

k2052 commented Jun 8, 2011

Oops my apologies the tests pass for me now. I think i had messed up a merge when I messing around with changes on my local copy. My changes had caused a failure. On a fresh clone the tests passed just fine.

However, there seems to a problem with mapped routes. https://gist.github.com/1013688

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

Yes, I created a failing test here: e5cb991 Thanks for confirming

Member

nesquena commented Jun 8, 2011

Yes, I created a failing test here: e5cb991 Thanks for confirming

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

Looks like Josh fixed it here (Thanks!), can you confirm @bookworm?

Member

nesquena commented Jun 8, 2011

Looks like Josh fixed it here (Thanks!), can you confirm @bookworm?

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 8, 2011

@nesquena Looks like its fixed. Its working in my projects and the tests all pass. Thanks allot Josh!

k2052 commented Jun 8, 2011

@nesquena Looks like its fixed. Its working in my projects and the tests all pass. Thanks allot Josh!

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Jun 8, 2011

Contributor

cool! lemme know if you run into more issues.

Contributor

joshbuddy commented Jun 8, 2011

cool! lemme know if you run into more issues.

@joshbuddy joshbuddy closed this Jun 8, 2011

@k2052

This comment has been minimized.

Show comment
Hide comment
@k2052

k2052 Jun 10, 2011

It looks like I found another issue. Doesn't work with multiple routes when in a controller. Looks like Filter.to_proc gets called only once when the filter is in a controller. Wrote a test here https://gist.github.com/1018033

k2052 commented Jun 10, 2011

It looks like I found another issue. Doesn't work with multiple routes when in a controller. Looks like Filter.to_proc gets called only once when the filter is in a controller. Wrote a test here https://gist.github.com/1018033

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 10, 2011

Member

Thanks man for catching this

Member

nesquena commented Jun 10, 2011

Thanks man for catching this

@DAddYE DAddYE reopened this Jun 10, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 11, 2011

Member

I assume this fixes this issue? @bookworm any more testing you can help us do is appreciated

Member

nesquena commented Jun 11, 2011

I assume this fixes this issue? @bookworm any more testing you can help us do is appreciated

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Jun 11, 2011

Contributor

Fixed in 1f3f534

Contributor

joshbuddy commented Jun 11, 2011

Fixed in 1f3f534

@joshbuddy joshbuddy closed this Jun 11, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment