Fixed onrequest path-related issue #292

Merged
merged 1 commit into from Nov 27, 2012

Projects

None yet

3 participants

@saleyn
saleyn commented Oct 18, 2012

No description provided.

@yrashk
yrashk commented Nov 17, 2012

I am wondering what's the exact "issue" this patch is solving?

@essen
Member
essen commented Nov 17, 2012

Path can change in onrequest.

@essen essen commented on an outdated diff Nov 27, 2012
src/cowboy_protocol.erl
@@ -449,10 +449,10 @@ request(Buffer, State=#state{socket=Socket, transport=Transport,
-spec onrequest(cowboy_req:req(), #state{}, binary(), binary()) -> ok.
onrequest(Req, State=#state{onrequest=undefined}, Host, Path) ->
dispatch(Req, State, Host, Path);
-onrequest(Req, State=#state{onrequest=OnRequest}, Host, Path) ->
+onrequest(Req, State=#state{onrequest=OnRequest}, Host, _Path) ->
@essen
essen Nov 27, 2012 Nine Nines member

Instead of not using it here can you remove this argument entirely? I'll merge when that's done. Thanks!

@essen
Member
essen commented Nov 27, 2012

Just a small change and it's good!

@saleyn
saleyn commented Nov 27, 2012

I am a little confused by your two comments above. Do you mean that you want me to remove the forth argument Path in the onrequest call entirely (which is a backward-compatibility issue), or that the change is good the way it is?

@essen
Member
essen commented Nov 27, 2012

Yes, remove, there's no backward there, it's an internal function. Just remove it on the caller too.

@saleyn
saleyn commented Nov 27, 2012

ok, cool.

@saleyn
saleyn commented Nov 27, 2012

Check it now.

@essen essen merged commit d0f3372 into ninenines:master Nov 27, 2012
@essen
Member
essen commented Nov 27, 2012

Merged, thanks!

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