Skip to content

Conversation

AlfonsoUceda
Copy link
Contributor

fix #93

This PR fixes a problem if a person does a redirect_to inside a before callback the call method in an action is called.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be good candidate for blank? method for utils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runlevel5
Copy link

Looks good to me

@AlfonsoUceda
Copy link
Contributor Author

@donperi mmmm maybe we need, what do you think guys?

@runlevel5
Copy link

@AlfonsoUceda I have no strong opinion on that, I leave it to @jodosha

@gotjosh
Copy link

gotjosh commented Mar 11, 2015

I think it should not be called. If you have an expensive operation that should be called after, it makes no sense to execute that if you redirected.

To me, you shouldn't get to that stage where you halted the execution chain yet you had a callback fired for no reason.

@jodosha
Copy link
Member

jodosha commented Mar 11, 2015

@AlfonsoUceda Thanks for taking care of this 😸

My opinion on this is to keep callbacks as simple as possible.
What if we do:

def redirect_to(url, status: 302)
  headers[LOCATION] = url
  halt(status)
end

This will prevent #call to be invoked, but it keeps the callbacks as they are.

@AlfonsoUceda
Copy link
Contributor Author

ok!, I'm goint to try that ;)

@AlfonsoUceda
Copy link
Contributor Author

@jodosha there is a problem doing that.

This test [1] fails, errors aren't passed

[1] https://github.com/lotus/controller/blob/master/test/integration/full_stack_test.rb#L26 with

  1. Failure:
    Full stack application#test_0003_in case of redirect and invalid params, it passes errors in session and then deletes them [/home/alfonso/Proyectos/lotus/forks/controller/test/integration/full_stack_test.rb:31]:
    Expected "FullStack::Controllers::Books::Index {:params=>#<FullStack::Controllers::Books::Index::Params:0x00000002f021f0 @env={"rack.version"=>[1, 3], "rack.input"=>#StringIO:0x00000002f03668, "rack.errors"=>#StringIO:0x00000002f03708, "rack.multithread"=>true, "rack.multiprocess"=>true, "rack.run_once"=>false, "REQUEST_METHOD"=>"GET", "SERVER_NAME"=>"example.org", "SERVER_PORT"=>"80", "QUERY_STRING"=>"", "PATH_INFO"=>"", "rack.url_scheme"=>"http", "HTTPS"=>"off", "SCRIPT_NAME"=>"/books", "CONTENT_LENGTH"=>"0", "rack.test"=>true, "REMOTE_ADDR"=>"127.0.0.1", "HTTP_REFERER"=>"http://example.org/books\", "HTTP_HOST"=>"example.org", "HTTP_COOKIE"=>"rack.session=BAh7BkkiD3Nlc3Npb25faWQGOgZFVEkiRWM4Y2Y1MWU0N2M5YTM1ZmJkNjcx%0AZDQ4YzJjMDllMGY3OTEzMDEzZGVmZDYyNjc4ZTFkNjZmYzAwZmVkZjg1NjgG%0AOwBG%0A--255d74ddc88955ac6299e6cf8b2934e95cde2410", "rack.session"=>{"session_id"=>"c8cf51e47c9a35fbd671d48c2c09e0f7913013defd62678e1d66fc00fedf8568"}, "rack.session.options"=>{:path=>"/", :domain=>nil, :expire_after=>nil, :secure=>false, :httponly=>true, :defer=>false, :renew=>false, :sidbits=>128, :secure_random=>SecureRandom, :secret=>"4efacb79d69e9eb1d6594acaa618033b", :coder=>#Rack::Session::Cookie::Base64::Marshal:0x00000002b355b8}, "router.request"=>#<HttpRouter::Request:0x00000002f02da8 @rack_request=#<Rack::Request:0x00000002f02dd0 @env={...}>, @path=[], @extra_env={}, @params=[], @acceptable_methods=#<Set: {"GET", "HEAD"}>>, "router.params"=>{}, "rack.request.query_string"=>"", "rack.request.query_hash"=>{}, "rack.request.cookie_hash"=>{"rack.session"=>"BAh7BkkiD3Nlc3Npb25faWQGOgZFVEkiRWM4Y2Y1MWU0N2M5YTM1ZmJkNjcx\nZDQ4YzJjMDllMGY3OTEzMDEzZGVmZDYyNjc4ZTFkNjZmYzAwZmVkZjg1NjgG\nOwBG\n--255d74ddc88955ac6299e6cf8b2934e95cde2410"}, "rack.request.cookie_string"=>"rack.session=BAh7BkkiD3Nlc3Npb25faWQGOgZFVEkiRWM4Y2Y1MWU0N2M5YTM1ZmJkNjcx%0AZDQ4YzJjMDllMGY3OTEzMDEzZGVmZDYyNjc4ZTFkNjZmYzAwZmVkZjg1NjgG%0AOwBG%0A--255d74ddc88955ac6299e6cf8b2934e95cde2410", "rack.session.unpacked_cookie_data"=>{"session_id"=>"c8cf51e47c9a35fbd671d48c2c09e0f7913013defd62678e1d66fc00fedf8568"}}, @raw=#<Lotus::Utils::Attributes:0x00000002f01e08 @attributes={}>, @attributes=#<Lotus::Utils::Attributes:0x00000002f01e08 @attributes={}>, @errors=#<Lotus::Validations::Errors:0x00000002f00df0 @errors={}>>, :errors=>#<Lotus::Validations::Errors:0x00000002f00df0 @errors={}>, :format=>:all}" to include "@Actual=""".

@AlfonsoUceda
Copy link
Contributor Author

@jodosha nevermind I got it, I'll update the PR

@runlevel5
Copy link

👍

runlevel5 pushed a commit that referenced this pull request Mar 15, 2015
Fix redirect_to inside callback and doesn't call `call` method in action
@runlevel5 runlevel5 merged commit 550f913 into hanami:master Mar 15, 2015
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlfonsoUceda Can you please ensure that the body isn't send, as it's useless?

last_response.body.must_equal ''

Hint: status(302, nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok @jodosha ;) I'll do another PR

@gotjosh
Copy link

gotjosh commented Mar 15, 2015

This looks great to me!

jodosha added a commit that referenced this pull request Mar 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#call method is called even when a before callback does a redirect_to.
4 participants