Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Add JSON support to PUT requests #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

1e1
Copy link

@1e1 1e1 commented Mar 8, 2016

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)

@Hywan
Copy link
Member

Hywan commented Mar 8, 2016

Is it identical to POST? If yes, we can merge the case in the switch instead.

@1e1
Copy link
Author

1e1 commented Mar 8, 2016

It seems correct but it changes the specifications.
In the previous version PUT requests never read $_PUT data.
And I don't know how work PATCH requests. Can we send data?

@1e1 1e1 force-pushed the put-json branch 2 times, most recently from bfa0f51 to 0080da9 Compare March 8, 2016 13:15
@Hywan
Copy link
Member

Hywan commented Mar 8, 2016

If @Metalaka has not time to be the reviewer, maybe @jubianchi could do the review.

@Metalaka
Copy link
Member

Metalaka commented Mar 8, 2016

@jubianchi Yes, Can you do the review ?

@Hywan Hywan assigned jubianchi and unassigned Metalaka Mar 9, 2016
@jubianchi
Copy link
Member

I'm on it


case 'application/json':
$input = file_get_contents('php://input');

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@jubianchi
Copy link
Member

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.
Here we handle form-data and json correctly. Everything else is just read from the input and returned. Fine, we have a common solution to handle everything else.

It's a 👍 for me.


case 'application/json':
$input = file_get_contents('php://input');

if (true !== $extended ||
true !== function_exists('json_decode')) {
Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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.

@1e1
Copy link
Author

1e1 commented Mar 11, 2016

How shall you I process for the second review?
I push an amend? I push a new commit? I open another PR?

@1e1
Copy link
Author

1e1 commented Mar 18, 2016

@jubianchi ping?

@Metalaka
Copy link
Member

@1e1 amend and then force push is good.

@1e1
Copy link
Author

1e1 commented Mar 18, 2016

OK.
Done.

@Hywan
Copy link
Member

Hywan commented Mar 23, 2016

@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?

@1e1
Copy link
Author

1e1 commented Mar 23, 2016

@Hywan @ALL
Indeed! I have no computer near me. If title is not changed by then, I will take care of it.

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)
@1e1
Copy link
Author

1e1 commented Mar 23, 2016

Done

@Hywan
Copy link
Member

Hywan commented Apr 1, 2016

Waiting on @jubianchi :-).

@Hywan
Copy link
Member

Hywan commented May 9, 2016

ping?

@1e1
Copy link
Author

1e1 commented May 9, 2016

up

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants