Conversation
| * @return string | ||
| */ | ||
| public function getApiUrl() { | ||
| public function getApiUrl(): string |
There was a problem hiding this comment.
There's quite a few changes in this PR that are the result of hitting the "Cleanup code" button in PHPStorm which basically amounts to adding a return type signature to functions. Just a heads up.
| "php": ">=5.4", | ||
| "guzzle/http": ">=3.6.0,<4" | ||
| "guzzlehttp/guzzle": "~6.3.3", | ||
| "ext-json": "*" |
There was a problem hiding this comment.
Even though ext-json is present by default, it is apparently best practice to explicitly list it as a requirement if you use things like json_decode.
TomasRebro
left a comment
TomasRebro
left a comment
There was a problem hiding this comment.
LGTM 👍 I just have one question to better understand that specific change.
| $response = $this->getHttpClient()->post($path, [ | ||
| 'body' => $body, | ||
| 'headers' => [ | ||
| 'Content-Type' => $contentType ?: 'application/octet-stream' |
There was a problem hiding this comment.
Was this default content type automatically set behind the scenes before? 🤔
There was a problem hiding this comment.
I don't actually know, because the API docs for guzzle3 have gone offline, I can't find a version on archive.org that has documentation for Guzzle's version of addPostFile, and the the old PHP docs for the built-in version of addPostFile mark contentType as optional. BUT, I do see in the archived Guzzle docs that it had a protected guessContentType() function, so my best guess is that yes, it was at least trying to set something behind the scenes. And application/octet-stream is the content type for "I don't really know what this is, here's a bunch of binary data", so it seemed like the most appropriate option.
Either way, pragmatically, it doesn't seem like we're actually going to run into a case where contentType is not supplied to this function.
2 participants
No description provided.