[HttpFoundation] [Response] Precondition Failed #9803
Conversation
| * | ||
| * @return Response | ||
| * | ||
| * @api |
There was a problem hiding this comment.
IIRC setting @api tags on non LTS releases is wrong.
see: #9808 (comment) (+1 squashed commit) Squashed commits: [7566e01] Use self:: instead of static:: - and I should consider stopping writhing code at midnight after a long workday ;) (+1 squashed commit) Squashed commits: [ea1e670] hasPreconditionFailed() and setPreconditionFailed()
| * | ||
| * @return Response | ||
| */ | ||
| public function setPreconditionFailed() |
There was a problem hiding this comment.
I'm not sure we really need this method
There was a problem hiding this comment.
@lucasmichot Can you remove this method, which does nothing more than setting the status code? The setNotModified() method is useful as it does more than just setting the status code.
|
@lucasmichot just thought about the use-case... is there a difference in case a system requests using a etag and the server responds with either 200 ok or 412 precondtion failed? |
|
@staabm yes there is. such Etag-based preconditions are meant to be used on non-safe requests, and a 412 would indicate that your edition has been rejected because someone else edited it concurrently already (causing the mismatch). If it responds with 200, you expect the form to be saved |
|
ah ok, though this PR is all about using ETAG in a "write"-context (POST, PUT) instead of "read" (GET)? |
|
@staabm It is about write contexts. For a read context, you will not send a precondition-failed response when the ETag does not match. You will send the updated content instead of a 304 (and this is already supported since 2.0) |
|
right, .. was just to make sure that I got it. thanks for the explanation, never used etag in a write context. |
|
looks good to me. @lucasmichot Can you remove the getter before we merge? |
|
Oh, and the tests are broken. |
|
@lucasmichot Do you have some time to finish this PR? |
| $preconditionFailed = !in_array($this->getEtag(), $etags); | ||
|
|
||
| $lastModified = $request->headers->get('If-Modified-Since', false); | ||
| $preconditionFailed |= $lastModified !== $this->headers->get('Last-Modified'); |
There was a problem hiding this comment.
You should cast to boolean, it's currently an integer. The doc block says it's a boolean
|
@lucasmichot Can you finish this PR? |
| * | ||
| * @see http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 | ||
| */ | ||
| public function hasPreconditionFailed(Request $request) |
There was a problem hiding this comment.
A hasser method should not modify state IMHO. Since this method might change the status code, it's better to name it checkPreconditionFailed.
|
Closing this PR as there is no feedback. Feel free to reopen it if you decide to finish it. Thanks. |
This feature adds the counterpart functions of
isNotModified()andsetNotModified().When a Request header ETag is provided and doesn't match the Response header Etag
AND/OR
When a Request header If-Modified-Since is provided and doesn't match the Response header Last-Modified
This allows to comply to RFC2616 specifications (see http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26).
This feature comes with the two following functions:
hasPreconditionFailed() setPreconditionFailed()This might ease up conditional response management, especially when using this feature within a REST server.