Header callback implementation for Request_Client - completes #4353 #229

Merged
merged 7 commits into from Mar 20, 2012

Conversation

Projects
None yet
4 participants
@acoulton
Contributor

acoulton commented Mar 5, 2012

@kiall, I think I've implemented the remaining work on http://dev.kohanaframework.org/issues/4353 - see the update there for the details of what I've done.

The code is unit tested and passes phpcs but there may be remaining formatting issues around the nested arrays particularly. It's a bit complex in places, but I think this is a slightly complex requirement to implement.

Comments very welcome!

acoulton added some commits Mar 4, 2012

Refactor Kohana_Request_ClientTest for more powerful mocking [refs #4…
…353]

Implement a dummy controller and route to allow test cases to execute internal
requests and specify controller response through the URL. Provides more
flexibility than mocking the Request/Request_Client and allows for nested
requests.
Request_Client triggers callbacks on presence of specified headers [r…
…efs #4353]

Request_Client checks the server response and triggers user-definable callbacks
when specified headers are present. The existing logic for following redirects
is implemented as a callback - other uses would include for eg dealing with
a WWW-Authenticate header to refresh an OAuth token.

Callbacks can return a request (which will be executed with the same callbacks
and client parameters as the original request) or a response.
Request callbacks are protected from too much recursion [refs #4353]
Request_Client takes a max_callback_depth property (default 5) and tracks
how many requests have been triggered by header callbacks - if the request
has too much recursion (eg an infinite redirect) the request is aborted with
a Request_Client_Recursion_Exception to allow this case to be handled by the
application developer.
Request_Client::callback_params() carries arbitrary data for header c…
…allbacks [refs #4353]

Request_Client::callback_params() provides a simple key-value store for
parameters that should be passed on to header callbacks. Parameter values
are copied into sub-requests triggered by a callback.
@Kohana-Builds

This comment has been minimized.

Show comment Hide comment
@Kohana-Builds

Kohana-Builds Mar 5, 2012

Build Scheduled

Build Scheduled

@Kohana-Builds

This comment has been minimized.

Show comment Hide comment
@kiall

This comment has been minimized.

Show comment Hide comment
@kiall

kiall Mar 5, 2012

Contributor

A quick read of the code, and it looks good.

I'll have a chance to test this later this evening - cmon 3.3! ;)

Contributor

kiall commented Mar 5, 2012

A quick read of the code, and it looks good.

I'll have a chance to test this later this evening - cmon 3.3! ;)

@Kohana-Builds

This comment has been minimized.

Show comment Hide comment
@Kohana-Builds

Kohana-Builds Mar 6, 2012

Build Scheduled

Build Scheduled

@Kohana-Builds

This comment has been minimized.

Show comment Hide comment
@kemo

This comment has been minimized.

Show comment Hide comment
@kemo

kemo Mar 6, 2012

if ($this->callback_depth() > $this->max_callback_depth())

I think this should be

if ($this->callback_depth() === $this->max_callback_depth() - 1)

I didn't really read the whole code but this caught my attention

kemo commented on 4e00844 Mar 6, 2012

if ($this->callback_depth() > $this->max_callback_depth())

I think this should be

if ($this->callback_depth() === $this->max_callback_depth() - 1)

I didn't really read the whole code but this caught my attention

@acoulton

This comment has been minimized.

Show comment Hide comment
@acoulton

acoulton Mar 6, 2012

Contributor

@kemo I'd need to look again and execute the relevant test, but I think that's correct - the check is executed as part of the next request execution (eg after $this->callback_depth() has been incremented and before the next recursion actually triggers a request).

Although thinking about it, it might be safer to use >=, in case the callback_depth parameter is incremented by more than one for any reason.

Contributor

acoulton commented Mar 6, 2012

@kemo I'd need to look again and execute the relevant test, but I think that's correct - the check is executed as part of the next request execution (eg after $this->callback_depth() has been incremented and before the next recursion actually triggers a request).

Although thinking about it, it might be safer to use >=, in case the callback_depth parameter is incremented by more than one for any reason.

Request_Client counts callback_depth from 1 for clarity [refs #4353]
callback_depth now begins at 1 (for the original request) rather than
zero. The property value is therefore directly equivalent to the number
of requests executed. For eg, after a standard request - redirect - request
pair, callback_depth will be equal to 2. The recursion limiting test case
has been updated to independently verify that the correct number of
requests were executed before the exception was thrown.
@acoulton

This comment has been minimized.

Show comment Hide comment
@acoulton

acoulton Mar 6, 2012

Contributor

@kemo sorry, I hadn't spotted earlier that I was using > rather than === already. I think this is safer - if a callback either accidentally increments by more than one, or executes a couple of additional requests before returning the exception will still be thrown when control eventually returns to that line.

I have however updated the test case to specifically verify the number of requests executed before the exception (rather than just checking the exception is thrown eventually). I have also changed callback_depth to start at 1 for the original request rather than 0 which I think makes more sense (so following a traditional request/redirect pair it will be 2, etc).

Contributor

acoulton commented Mar 6, 2012

@kemo sorry, I hadn't spotted earlier that I was using > rather than === already. I think this is safer - if a callback either accidentally increments by more than one, or executes a couple of additional requests before returning the exception will still be thrown when control eventually returns to that line.

I have however updated the test case to specifically verify the number of requests executed before the exception (rather than just checking the exception is thrown eventually). I have also changed callback_depth to start at 1 for the original request rather than 0 which I think makes more sense (so following a traditional request/redirect pair it will be 2, etc).

@Kohana-Builds

This comment has been minimized.

Show comment Hide comment
@Kohana-Builds

Kohana-Builds Mar 6, 2012

Build Scheduled

Build Scheduled

@kemo

This comment has been minimized.

Show comment Hide comment
@kemo

kemo Mar 6, 2012

Member

@acoulton looks great, keep up with good work!

Member

kemo commented Mar 6, 2012

@acoulton looks great, keep up with good work!

@kiall

This comment has been minimized.

Show comment Hide comment
@kiall

kiall Mar 14, 2012

Contributor

I will get to this today! I will. (Tell myself that enough times and its bound to happen, right?)

Anyway - I'm pretty sure I'm happy with it, and will merge later this afternoon unless I uncover something unexpected when fully testing it out..

Contributor

kiall commented Mar 14, 2012

I will get to this today! I will. (Tell myself that enough times and its bound to happen, right?)

Anyway - I'm pretty sure I'm happy with it, and will merge later this afternoon unless I uncover something unexpected when fully testing it out..

kiall added a commit that referenced this pull request Mar 20, 2012

@kiall kiall merged commit 46b0662 into kohana:3.3/develop Mar 20, 2012

@kiall

This comment has been minimized.

Show comment Hide comment
@kiall

kiall Mar 20, 2012

Contributor

Today turned into next week, but still. Done!

Looks good @acoulton - Merged, Thanks!

Contributor

kiall commented Mar 20, 2012

Today turned into next week, but still. Done!

Looks good @acoulton - Merged, Thanks!

@acoulton

This comment has been minimized.

Show comment Hide comment
@acoulton

acoulton Mar 20, 2012

Contributor

Great! Thanks @kiall - roll on 3.3! :)

Contributor

acoulton commented Mar 20, 2012

Great! Thanks @kiall - roll on 3.3! :)

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