-
Notifications
You must be signed in to change notification settings - Fork 57
FinalHandler should return new responses #12
FinalHandler should return new responses #12
Conversation
@@ -23,33 +25,54 @@ class FinalHandler | |||
private $options; | |||
|
|||
/** | |||
* Original response provided to the middleware. | |||
* | |||
* @var null|Http\Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be null | ResponseInterface
Ping @ezimuel — this is the feature we discussed. |
49556ca
to
f147390
Compare
If no error is present, and the response passed on invocation is different than the response at initialization, the `FinalHandler` should return that response intact. This allows middleware to create a response, but pass it on to `$next()` for post-processing further up the chain. In particular, this is useful for things like routing middleware, so as to allow for application-wide concerns such as logging, finalizing sessions, etc., without requiring onion-style middleware for these behaviors.
f147390
to
e13dd06
Compare
@weierophinney I just tested this PR with this gist but it's not working. It seems that the condition here is not satisfied. Changing the body of the original response, using |
- Per @ezimuel, when the original response body has been written to, `FinalHandler` was still returning the 404 response. This patch ensures that if the body size changes, the response will still be returned.
@ezimuel I've just pushed a change that checks to see if the response body size has changed; if it has, then |
@weierophinney I'm not sure that checking the response body size will solve the issue, imagine that you change another value of the response, like the HTTP status, without changing the body, I think we should return the response as well. |
@ezimuel You cannot change the HTTP status without creating a new instance; the response object itself is immutable. The body is the only aspect of the response that is not immutable, and this is due to the fact that we cannot enforce immutability on what the Stream instance wraps (usually a PHP stream resource). So, in this case, our options are:
The only other condition that could indicate a change might be changes to stream metadata; however, I would consider that an edge case. |
@weierophinney ok, it sound reasonable. I don't know if we can find a better way to check against different PHP stream. Anyway, I checked your last commit with my gist example and it works fine! |
If no error is present, and the response passed on invocation is different than the response at initialization, the
FinalHandler
should return that response intact.This allows middleware to create a response, but pass it on to
$next()
for post-processing further up the chain. In particular, this is useful for things like routing middleware, so as to allow for application-wide concerns such as logging, finalizing sessions, etc., without requiring onion-style middleware for these behaviors.This change requires loosening the typehinting to allow any PSR-7 response on invocation. In most cases, PSR-7 requests and responses are adequate, and having the decorated versions present in Stratigility simply provides some convenience. As such, this patch also loosens most typehints to hint on the PSR-7 interfaces; this does not present a BC break, however, as the original instances also pass this hint.
Essentially, this change allows something like the following:
Prior to this patch, while you could do the above, you would end up returning a 404 response at the end. The reason is that the middleware stack would become exhausted, leading to invocation of the
FinalHandler
; in the absence of an error, theFinalHandler
returns a 404. With the patch, because the response passed toFinalHandler
will be the response created and passed from the first middleware,FinalHandler
will return that response instead.