achiurizo / rack-recaptcha

Rack Middleware for CAPTCHA verification via Recaptcha API
twitter.com/arthur_chiu
MIT License
151 stars 23 forks source link

Rack-recaptcha 0.6.0: undefined local variable or method `request' #12

Closed eric-hu closed 13 years ago

eric-hu commented 13 years ago

One of my integration tests fails with this error:

  undefined local variable or method `request' for #<ActionView::Helpers::FormBuilder:0x7fd39d2120f0> (ActionView::Template::Error)

Reverting to rack-recaptcha 0.5.0 resolves this error.

Gist of stacktrace

Let me know if you'd like me to test anything related to this issue

achiurizo commented 13 years ago

thanks for the note. i'll look into this shortly. seems like there probably needs to be a better way of extracting the request for different versions of rails(2.x,3.x) and other rack apps.

achiurizo commented 13 years ago

i've pushed a new version with a fix. let me know if this resolves this issue.

eric-hu commented 13 years ago

Hm, I'm getting the same error. The problem is with line 37 in helpers.rb

error_message = request.env['recaptcha.msg']

I tried using env['recaptcha.msg'] instead, as you did with recaptcha_valid? in your patch, but that gives

undefined local variable or method `env'

The strange thing is that I dropped a debugger above line 37 and request exists for other integration tests (though env does not).

I'm looking into why this line fails for this specific test case.

achiurizo commented 13 years ago

i pushed another version here: f909629a573f03f65ced that addresses the request in the error messges as well. let me know if this fixes that.

eric-hu commented 13 years ago

I actually tried that in a monkey patch and the env variable isn't defined (at least in the test case). I pulled and verified, though:

undefined local variable or method `env' for #<ActionView::Helpers::FormBuilder:0x7fd4ec6c7640> (ActionView::Template::Error)

This error also comes up when I run a server and visit the page in a browser.

My integration tests lead me to believe that the request object you're using in 0.6.0 will actually exist in most cases. The specific case when it doesn't exist is when called on a FormBuilder. For instance, this is my code calling it:

form_for(@resource, :as =>resource_name, :url => registration_path(resource_name)) do |f|
...
  raw f.recaptcha_tag(:challenge)

I used debugger to check out the global, local and instance variables within the scope of a FormBuilder. There doesn't seem to be anything that's specific to a particular request. I played around with the idea of a class variable inside Rack::Recaptcha along with get and set methods. This would break as soon as the server has 2 concurrent requests with different error messages--one would overwrite the other.

Might I suggest something like this:

    error_message = request.env['recaptcha.msg'] if request

It'll do what you want to in some cases, and basically revert to 0.5.0 behavior otherwise.

achiurizo commented 13 years ago

hm yeah that sounds reasonable. did you want to send a pull request? I be happy to pull a patch in.

eric-hu commented 13 years ago

I'd be happy to. At the moment, I'm stuck on creating a suitable test, as mocking the request object as nil doesn't work. It's a conflict with HelperTest::Request taking precedence.

I can mock request.env as nil, but changing the error check to if request.env breaks many of your error message tests. I can modify those to be compatible, if that's ok with you.

Otherwise I can submit a pull request without testing the no-request-available case (gasp!)

achiurizo commented 13 years ago

you could separate the context of the request.env mocking from the rest of the error message tests. otherwise, just send a pull request with the patch and i'll deal with the tests.

eric-hu commented 13 years ago

I was able to figure out how to do it by leaving your tests in place. Pull request sent:

https://github.com/achiu/rack-recaptcha/pull/13

My first pull request still had an artifact from my debugging (ruby-debug19 dependency), but I think the pull request is updated to fix that.