Split getUrlContents #864
Split getUrlContents #864
Conversation
Since `getUrlContents()` takes the url as a string argument, it is not possible to set headers, such as `Accept-Language`, for the request. Therefore I created `getRequest()` to generate the request based on the url string, but without fetching the contents. Then headers can be added to the request, before the request is passed to `getRequestContents()` which fetches the content.
| $request = $this->getMessageFactory()->createRequest('GET', $url); | ||
| $request = $this->getRequest($url); | ||
|
|
||
| return $this->getRequestContents($request); |
There was a problem hiding this comment.
Maybe rename this to parseResponse.
There was a problem hiding this comment.
See my reply to your other comment.
| * @throws InvalidServerResponse | ||
| */ | ||
| protected function getRequestContents(RequestInterface $request): string | ||
| { | ||
| $response = $this->getHttpClient()->sendRequest($request); |
There was a problem hiding this comment.
I don’t think this function should send the request.
There was a problem hiding this comment.
We could change getRequestContents(RequestInterface $request) to parseResponse(ResponseInterface $response), and call $response = $this->getHttpClient()->sendRequest($request) inside getUrlContents before calling parseResponse. The drawback would be that the request url would not be available inside parseResponse, so the method would require an extra parameter $url in order to throw InvalidServerResponse.
There was a problem hiding this comment.
Oh. I see. Hm. You could throw the exception and the catch it...
If you decide to still send the request in this function, I still think you should rename it. Maybe to “getparsedResponse” or something similar. A request does not really have “content”. It has a body of it is a POST request.
There was a problem hiding this comment.
Good point. I'll rename it. If a provider needs to access the response, it would be possible to move $response = $this->getHttpClient()->sendRequest($request); into a separate method sendRequest, that could be overridden by the provider.
* Use AbstractHttpProvider in FreeGeoIp Use `getRequest` and `getParsedResponse` from `AbstractHttpProvider` instead of duplicating code in `FreeGeoIp`. See #864. * Remove unnecessary uses * Update dependencies
* Use AbstractHttpProvider in FreeGeoIp Use `getRequest` and `getParsedResponse` from `AbstractHttpProvider` instead of duplicating code in `FreeGeoIp`. See geocoder-php/Geocoder#864. * Remove unnecessary uses * Update dependencies
* Use AbstractHttpProvider in FreeGeoIp Use `getRequest` and `getParsedResponse` from `AbstractHttpProvider` instead of duplicating code in `FreeGeoIp`. See geocoder-php/Geocoder#864. * Remove unnecessary uses * Update dependencies
As requested in #829. Since
getUrlContents()takes the url as a string argument, it is not possible to set headers, such asAccept-Language, for the request. Therefore I createdgetRequest()to generate the request based on the url string, but without fetching the contents. Then headers can be added to the request, before the request is passed togetRequestContents()which fetches the content.