Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpClient] add ResponseInterface::toArray()#30499
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| if (!\is_array($content)) { | ||
| thrownewJsonException(sprintf('Response returned %s while an array was expected.',\gettype($content))); |
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.
quoted %s
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.
updated to:JSON content was expected to decode to an array, %s returned.
117a1f6 toaabd1d4Compare| publicfunctiongetContent(bool$throw =true):string; | ||
| /** | ||
| * Gets the response body decoded as array, typically from a JSON payload. |
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.
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?
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.
Integration with Serializer should be provided by decoration.
nicolas-grekasMar 9, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Note I see no reasons why integration with Serializer should implementHttpClientInterface: turning strings (even when coming from response bodies) to objects is not the domain of an http client to me.
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.
In that case i think we need to update the description to something likeGets 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.
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'm not sure to spot the difference, semantically :)
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.
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.
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.
that would also help#30494 btw
…-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()
I'd like we discuss adding a
toArray()method toResponseInterface.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.).