-
Notifications
You must be signed in to change notification settings - Fork 9
Add JSON support to PUT requests #22
base: master
Are you sure you want to change the base?
Conversation
|
Is it identical to |
|
It seems correct but it changes the specifications. |
bfa0f51 to
0080da9
Compare
|
If @Metalaka has not time to be the reviewer, maybe @jubianchi could do the review. |
|
@jubianchi Yes, Can you do the review ? |
|
I'm on it |
|
|
||
| case 'application/json': | ||
| $input = file_get_contents('php://input'); | ||
|
|
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.
I would leave this line for clarity purpose.
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.
We can keep this empty line, yes.
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.
OK
|
The code seems ok to me. The rfc5789 does not talk about any restriction on PUT and PATCH request. They might both have a body with a appropriate content type. It's a 👍 for me. |
Runtime.php
Outdated
| $input = file_get_contents('php://input'); | ||
|
|
||
| if (true !== $extended || | ||
| true !== function_exists('json_decode')) { |
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.
is this check still required ? /cc @Hywan
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.
Yup, because ext/json is not always installed.
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.
So the behavior seems not correct: if the user passes $extended = true he expects that the JSON gets parsed. If there is no json_decode in this case I would rather throw an exception as the client code might get an unexpected string.
|
How shall you I process for the second review? |
|
@jubianchi ping? |
|
@1e1 amend and then force push is good. |
|
OK. |
|
@1e1 Your commit title is too long. Either you change it or you're allowing us to change it for you. @jubianchi Is the PR OK for you? |
If a PUT request is sent with the header "Content-Type: application/json" the input data will be json_decode as a POST request is decoded. (depends to the '$extended' parameter)
|
Done |
|
Waiting on @jubianchi :-). |
|
ping? |
|
up |
If a PUT request is sent with the header "Content-Type: application/json"
the input data will be json_decode as a POST request is decoded.
(depends to the '$extended' parameter)