Locale-aware FreeGeoIp provider

[Stadly]

Support for locales in the FreeGeoIp provider.

Based on #744

[Stadly]

Travis fails when running tests with the message Error: Call to undefined method Geocoder\Provider\FreeGeoIp\FreeGeoIp::getRequest(), but getRequest() is defined in Geocoder\Http\Provider\AbstractHttpProvider, which Geocoder\Provider\FreeGeoIp\FreeGeoIp extends. Why is that?

Is it possible that the tests are run using the latest version of Geocoder, without the changes done in this PR? I see that composer says - Installing willdurand/geocoder (4.0.0): Loading from cache. It seems like Geocoder\Provider\FreeGeoIp\FreeGeoIp contains the changes from this PR, while Geocoder\Http\Provider\AbstractHttpProvider does not contain the changes from this PR. Is that possible, or am I just missing a bug in my PR?

[jbelien]

Is that possible, or am I just missing a bug in my PR?

No, I think you're right !
Travis probably uses last released version of geocoder-php/free-geoip-provider which has not the getRequest() function.
I guess @Nyholm or @willdurand can confirm this :)

[willdurand]

mmh that would be a bit weird not to use the patched code on travis-ci.

[Nyholm]

Travis probably uses last released version of geocoder-php/free-geoip-provider which has not the getRequest() function.

For the high/low test, yes. But not for the "normal" tests. You need to make sure to save response cache after you run the tests locally.

[Stadly]

Now the "normal" tests work :)

[Nyholm]

Im reviewing this PR more carefully now. Why are these extra helper functions needed in the AbstractHttpProvider?

[Stadly]

Because getUrlContents() takes the url as a string argument, the Accept-Language header cannot be specified. Therefore I created getRequest() to generate the request based on the url string, but without fetching the contents. Then the Accept-Language header can be added to the request, before the request is passed to getRequestContents() which fetches the content.

[Nyholm]

Yeah, but coudn't you just use the request factory and send the request without getUrlContents?

[Stadly]

Do you mean to duplicate the code from getUrlContents() in FreeGeoIp::geocodeQuery() so that the header can be added? That would of course be an option. I thought it would be a good idea to have getRequestContents(), as it might also be useful for other providers that need to set headers.

getRequest() can of course be replaced by calling the request factory directly if you prefer.

[Nyholm]

At the moment we have 30+ provider and it is not useful for any of them. But of course it could be revisited in the future.

I think it would be cleaner not to edit the AbstractProvider at all.

[Stadly]

Ok. Should I change the PR to leave the AbstractHttpProvider alone, although it will lead to some code duplication?

[Nyholm]

Yes, it is just one or two rows that you duplicate.

[Stadly]

Sorry, then I'm not sure I understand what your idea is. I was thinking to make FreeGeoIp::geocodeQuery() like this:

    public function geocodeQuery(GeocodeQuery $query): Collection
    {
        $address = $query->getText();
        if (!filter_var($address, FILTER_VALIDATE_IP)) {
            throw new UnsupportedOperation('The FreeGeoIp provider does not support street addresses.');
        }

        if (in_array($address, ['127.0.0.1', '::1'])) {
            return new AddressCollection([$this->getLocationForLocalhost()]);
        }

        $uri = sprintf($this->baseUrl, $address);
        $request = $this->getMessageFactory()->createRequest('GET', $uri);

        if (null !== $query->getLocale()) {
            $request = $request->withHeader('Accept-Language', $query->getLocale());
        }

        // Start code duplication
        $response = $this->getHttpClient()->sendRequest($request);

        $statusCode = $response->getStatusCode();
        if (401 === $statusCode || 403 === $statusCode) {
            throw new InvalidCredentials();
        } elseif (429 === $statusCode) {
            throw new QuotaExceeded();
        } elseif ($statusCode >= 300) {
            throw InvalidServerResponse::create($uri, $statusCode);
        }

        $content = (string) $response->getBody();
        if (empty($content)) {
            throw InvalidServerResponse::emptyResponse($uri);
        }
        // End code duplication

        $data = json_decode($content, true);
        $builder = new AddressBuilder($this->getName());

        if (!empty($data['region_name'])) {
            $builder->addAdminLevel(1, $data['region_name'], $data['region_code'] ?? null);
        }

        if (0 !== $data['latitude'] || 0 !== $data['longitude']) {
            $builder->setCoordinates($data['latitude'] ?? null, $data['longitude'] ?? null);
        }
        $builder->setLocality(empty($data['city']) ? null : $data['city']);
        $builder->setPostalCode(empty($data['zip_code']) ? null : $data['zip_code']);
        $builder->setCountry(empty($data['country_name']) ? null : $data['country_name']);
        $builder->setCountryCode(empty($data['country_code']) ? null : $data['country_code']);
        $builder->setTimezone(empty($data['time_zone']) ? null : $data['time_zone']);

        return new AddressCollection([$builder->build()]);
    }

[Stadly]

Just let me know if the solution in the comment above is preferable to what is currently in the PR, or if you have another solution in mind.

[Stadly]

Friendly ping :)

[TerjeBr]

@Stadly is right, the getUrlContents method in Geocoder\Http\Provider\AbstractHttpProvider violates the first SOLID principle "Single point of responsibility". It has been given the responsibilty of too much action. That is why it needs to be divided up.

The method getUrlContents is doing 3 things:

1. it creates a http reqest from an url
2. it sends the request
3. it partially parses the response
   • checking the http status code
   • checking that the body is not empty

@Stadly here split it in two, by making getUrlContents just do 1 and then call the new method getRequestContents(RequestInterface $request): string that does 2 and 3.

I had a similar problem in #840 where I needed to be able to do POST request as well as GET requests. So I spilt the method getUrlContents by making it only do 1 and 2, and then call a new method parseHttpResponse(ResponseInterface $response, string $url): string that does 3.

And it is not just a couple of lines of code duplication, but around 16-17 lines. And if @Nyholm and @willdurand decides that we are not allowed to change AbstractHttpProvider, then both this provider and the MapQuest provider will have to duplicate the same code.

We need a decision from either @Nyholm or @willdurand. Are we allowed to change AbstractHttpProvider? And if so, what would be the best way to do it? The way @Stadly has done it in this PR or the way I have done it in PR #840?

[Stadly]

Based on the feedback both here and in #840, I have changed this PR to only alter FreeGeoIp files, and not touch anything else. So hopefully this PR can now be merged :)

[Nyholm]

Thank you

[Nyholm]

Thank you for this PR. As @TerjeBr mentioned and as @Stadly initially wrote, it is better to modify the AbstractHttpProvider. I was wrong before.

I will merge this PR now as it is super simple to review since you only modified one provider.

Could you create a PR that only modifies that AbstractHttpProvider package with the changes you initially made?