Authorize empty content put in custom operations #1274
Conversation
thomasglachant
force-pushed
the
authorize-empty-content-put
branch
from
a2ccd4f to
84fd4a6
Compare
| @@ -51,6 +51,7 @@ public function onKernelRequest(GetResponseEvent $event) | |||
| || $request->isMethod(Request::METHOD_DELETE) | |||
| || !($attributes = RequestAttributesExtractor::extractAttributes($request)) | |||
| || !$attributes['receive'] | |||
| || ($request->isMethod(Request::METHOD_PUT) && $request->getContent() === '') | |||
There was a problem hiding this comment.
Can you store the result of $request->getContent() in a variable to avoid 2 (costly) method calls (the next one is line 70)?
Also, please use Yoda styl ('' === $content).
| @@ -52,7 +52,7 @@ public function onKernelRequest(GetResponseEvent $event) | |||
| || $request->isMethod(Request::METHOD_DELETE) | |||
| || !($attributes = RequestAttributesExtractor::extractAttributes($request)) | |||
| || !$attributes['receive'] | |||
| || ($request->isMethod(Request::METHOD_PUT) && '' === $requestContent) | |||
| || ($request->isMethod(Request::METHOD_PUT) && '' === $requestContent) | |||
There was a problem hiding this comment.
May I suggest:
|| ($request->isMethod(Request::METHOD_PUT) && '' === ($requestContent = $request->getContent()))
It will avoid to always retrieve the body (that can be huge) in most cases.
There was a problem hiding this comment.
Ok, I can do it, but it's a little more complicated because it's a "&&" and for a POST we don't initialize the variable.
So I need to add $requestContent ?? $request->getContent() in the deserialize line.
It will be faster but less clear.
There was a problem hiding this comment.
As this listener will be executed for every request (even if its a page not managed by API Platform), let's use the fastest alternative.
There was a problem hiding this comment.
done ! I add a behat test with the empty body ... don't know if it's necessary ...
But in reality it will work for all |
|
What's the behavior with this patch when an empty request is sent to a standard PUT route? |
|
We have a 200 with the unmodified resource instead of a 400. |
Nothing (200) ... I wrote a behat test to check. It's like a PUT request with an empty array in body. |
|
It looks acceptable to me. WDYT @api-platform/core-team? |
|
Ok to me. |
|
It looks like a BC break to have a 200 instead of a 400 now (for a standard PUT). Nop? |
|
Lot of overhead if we need to check that it's a custom operation no? |
|
It's borderline, but it looks acceptable. Who would rely on an empty request throwing a 400? |
| if ( | ||
| $request->isMethodSafe(false) | ||
| || $request->isMethod(Request::METHOD_DELETE) | ||
| || !($attributes = RequestAttributesExtractor::extractAttributes($request)) | ||
| || !$attributes['receive'] | ||
| || ($request->isMethod(Request::METHOD_PUT) && '' === ($requestContent = $request->getContent())) |
There was a problem hiding this comment.
If you invert the conditions (('' === $requestContent = $request->getContent()) && $request->isMethod(Request::METHOD_PUT)), you don't need the trick with ?? anymore because the variable will always be defined. WDYT?
There was a problem hiding this comment.
Good idea, clean and fast 👍
thomasglachant
force-pushed
the
authorize-empty-content-put
branch
from
208695c to
b55eb08
Compare
| @@ -395,6 +395,41 @@ Feature: Create-Retrieve-Update-Delete | |||
| } | |||
| """ | |||
|
|
|||
| Scenario: Update a resource with empty body | |||
There was a problem hiding this comment.
@dunglas Do you think it's necessary to add this test ? I'm not convinced ...
There was a problem hiding this comment.
There are no useless integration test ;).
|
It's not like someone should rely on a 400 return. LGTM |
|
Thanks @thomasglachant ! |
|
-1 I really do not understand why this needs to be specially handled. What happened to the PR(s) on allowing empty request body? |
|
Empty request body does make sense only with PUT / DELETE requests no? |
* Authorize empty request content for PUT in a custom action * Apply php-cs-fixer * Apply review changes * Apply review changes * Switch to fastest alternative. Add behat test for with empty body on PUT. * Refacto after dunglas review
This PR allows to send empty request content for PUT in a custom operation.
Use case :
I have a custom Action (ex : "reset") associated to an ApiResource "Job".
I don't have any request content to send when I send a PUT request to
/jobs/1234/reset.But I've a SerializerError because the request content is required.