Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpClient] add ResponseInterface::toArray() #30499

Merged
merged 1 commit into from Mar 10, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 9, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I'd like we discuss adding a toArray() method to ResponseInterface.

JSON responses are so common when doing server-side requests that this may help remove boilerplate - especially the logic dealing with errors.

WDYT?

(about flags, I don't think we should make them configurable: if one really needs to deal with custom flags, there's always ResponseInterface::getContent() - but it should be very rare.).

}
if (!\is_array($content)) {
throw new JsonException(sprintf('Response returned %s while an array was expected.', \gettype($content)));

This comment has been minimized.

@fabpot

fabpot Mar 9, 2019

Member

quoted %s

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 9, 2019

Author Member

updated to:
JSON content was expected to decode to an array, %s returned.

@fabpot

fabpot approved these changes Mar 9, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:hc-response-toarray branch from 117a1f6 to aabd1d4 Mar 9, 2019

@@ -59,6 +59,18 @@ public function getHeaders(bool $throw = true): array;
*/
public function getContent(bool $throw = true): string;
/**
* Gets the response body decoded as array, typically from a JSON payload.

This comment has been minimized.

@ro0NL

ro0NL Mar 9, 2019

Contributor

does this mean component will deal with e.g. XML if requested? What if ppl want to leverage a serializer for this?

Honestly, do want the API? :D

From #30502

ppl can always encode/decode on their own if they really want precise control on these

yet, we kinda want this 30+ line method :) should it be util instead?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 9, 2019

Author Member

Integration with Serializer should be provided by decoration.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 9, 2019

Author Member

Note I see no reasons why integration with Serializer should implement HttpClientInterface: turning strings (even when coming from response bodies) to objects is not the domain of an http client to me.

This comment has been minimized.

@ro0NL

ro0NL Mar 9, 2019

Contributor

In that case i think we need to update the description to something like Gets a decoded body array representation (typically ...).

Let's say i decorate this toArray to mutate data, but im passing this client globally around in my app; some services might be effected by this.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 9, 2019

Author Member

🤔 I'm not sure to spot the difference, semantically :)

This comment has been minimized.

@ro0NL

ro0NL Mar 9, 2019

Contributor

Missed your comment, you're right we can expect HTTP domain; thus the body decoded as-is.

👍 with a simple decoration solution for extension in the future.

This comment has been minimized.

@ro0NL

ro0NL Mar 9, 2019

Contributor

that would also help #30494 btw

@nicolas-grekas nicolas-grekas merged commit aabd1d4 into symfony:master Mar 10, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 10, 2019

feature #30499 [HttpClient] add ResponseInterface::toArray() (nicolas…
…-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[HttpClient] add ResponseInterface::toArray()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I'd like we discuss adding a `toArray()` method to `ResponseInterface`.

JSON responses are so common when doing server-side requests that this may help remove boilerplate - especially the logic dealing with errors.

WDYT?

(about flags, I don't think we should make them configurable: if one really needs to deal with custom flags, there's always `ResponseInterface::getContent()` - but it should be very rare.).

Commits
-------

aabd1d4 [HttpClient] add ResponseInterface::toArray()

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:hc-response-toarray branch Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.