-
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. |
|
||
case 'application/json': | ||
$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)