Add Here Provider #844

sheub · 2018-04-15T13:25:06Z

Happy to contribute with the Here provider.

Nyholm

This looks great. Good job!

We just need to add some tests. It is integration tests. So you basically need to select an address and verify the result is as you expect.
When you run the tests (with your API key) some files in .cached_responses will be created. These should also be checked in to the repo.

You run the test simply by

cd src/Procider/Here
composer update
composer test

Nyholm · 2018-04-16T06:20:18Z

src/Provider/Here/CHANGELOG.md

+
+The change log describes what is "Added", "Removed", "Changed" or "Fixed" between each release.
+
+## 4.0.0


Lets do 1.0.0 for a new provider.

Nyholm · 2018-04-16T06:20:58Z

src/Provider/Here/Here.php

+    private $appCode = null;
+
+    /**
+     * @param HttpAdapterInterface $adapter An HTTP adapter.


This is wrong. This should be HttpClient

@param

+ * @param string $apoCode An App code.

Mechazawa · 2018-04-16T12:27:47Z

Thanks for making a PR @sheub! I've been needing this provider support for a while.

sheub · 2018-04-18T08:21:42Z

So, if I skip the Ipv4 Test ($testIpv4 = false;) I get results:

OK, but incomplete, skipped, or risky tests!
Tests: 24, Assertions: 109, Skipped: 2.

Baachi · 2018-04-19T11:40:33Z

src/Provider/Here/Here.php

+     * @param string     $appId   An App ID.
+     * @param string     $apoCode An App code.
+     */
+    public function __construct(HttpClient $client, $appId, $appCode)


I would typehint the $appId and appCode parameters as string. Than we can remove the exception in the constructor.

Baachi · 2018-04-19T11:41:23Z

src/Provider/Here/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) 2011 — William Durand <william.durand1@gmail.com>


Maybe you want to change it to your name?

Baachi · 2018-04-19T11:42:50Z

src/Provider/Here/Tests/HereTest.php

+        $provider = new Here(
+            $this->getMockedHttpClient(
+                '{
+					    "type": {


You should use spaces instead of tabs.

Baachi · 2018-04-19T11:45:28Z

src/Provider/Here/Tests/IntegrationTest.php

+use Http\Client\HttpClient;
+
+/**
+ * @author Sébastien Barré <sebastien@sheub.eu>


In the Here class you annotate the class with info@zoestha.de and here with sebastien@sheub.eu. Is this intended or do you want to fix this?

Thanks for the Review. I have corrected the Here Class.

Nyholm · 2018-04-23T07:12:06Z

Thank you.
You should add an entry for HERE_APP_ID in phpunit.xml.dist. See https://github.com/geocoder-php/Geocoder/blob/master/phpunit.xml.dist#L27

sheub · 2018-05-07T08:54:16Z

Hey there,
still get InvalidCredentials.
please let me know what should be done before the merge.

sheub · 2018-05-27T11:26:38Z

Can anyone give me instructions on how to finalize this PR? In advance Thank you.

Baachi · 2018-05-28T06:27:06Z

Hey @sheub,

the TravisCI checks must be passed. A failing PR can't get merged.

sheub · 2018-05-28T08:38:10Z

Hey @Baachi,
Thanks for your reaction.

The errors from Travis are :
Geocoder\Exception\InvalidCredentials.

But he integration tests passed when I run them at home with my credentials.

I have commit all the files created in Tests/.cached_responses and I have added

<server name="HERE_APP_ID" value="YOUR_APP_ID" />
<server name="HERE_APP_CODE" value="YOUR_APP_CODE" />

in phpunit.xml.dist

I see also (at home) that when I rerun the composer test, the execution is much faster, so I suppose that the tests is running on the ./cached_responses.

I don't understand why Travis is not executing the tests on the ./cached_responses
I am missing something? Thanks for your support.

sheub · 2018-05-29T08:02:25Z

Hi, it seems that the files created in the ./cached_responses will get a different Hash code for each HERE_APP_ID/HERE_APP_CODE pair.

see:
geocoder.api.here.com_62eb9554b9240ece07c143250a367bf2a534ac37
and
geocoder.api.here.com_194f5df1c578dfb8b688908c8e87c11d2afdcf9d
if I run with a real credentials or with YOUR_APP_ID YOUR_APP_CODE

For that reason the tests is not run on the ./cached_responses.

Any Idea? Thanks for your support.

sheub · 2018-05-31T07:48:45Z

Hi @Nyholm ,
May I kindly ask you to point me out in the right direction so I can resolve this issue.
I can't see why the tests won't run on the ./cached_responses
Many thanks for your time.

sheub · 2018-06-04T10:54:29Z

Hi @Nyholm ,

I could find out why the tests wouldn't run on the cached files.
The problem is that the HERE provider requires 2 Credential keys in the url: the app_id & the app_code.

and only one apiKey is searched for and replaced in the body part of the cacheKey:
Geocoder\IntegrationTest\CachedResponseClient

     public function sendRequest(RequestInterface $request)
    {
        $url = (string) $request->getUri();
        $host = (string) $request->getUri()->getHost();
        if (!empty($this->apiKey)) {
            $url = str_replace($this->apiKey, '[apikey]', $url);
        }

        $file = sprintf('%s/%s_%s', $this->cacheDir, $host, sha1($url));
        if (is_file($file) && is_readable($file)) {
            return new Response(200, [], (new StreamFactory())->createStream(unserialize(file_get_contents($file))));
        }

        $response = $this->delegate->sendRequest($request);
        file_put_contents($file, serialize($response->getBody()->getContents()));

        return $response;
    }

I could get it work properly at home, but I had to (slightly) modify three files from geocoder-php/provider-integration-tests:

Geocoder\IntegrationTest\ProviderIntegrationTest,
Geocoder\IntegrationTest\CachedResponseClient
Geocoder\IntegrationTest\BaseTestCase

In a way that both the app_id & the app_code are replaced in the url.

This solution is working, but it requires that the geocoder-php/provider-integration-tests will be modified...

jbelien · 2018-06-04T11:10:12Z

Hello @sheub ,

I didn't check in details but Google Maps provider also requires 2 parameters (ClientId and APIKey) ; so I guess it should also work with HERE without having to modify the Geocoder\IntegrationTest.

Have a look at https://github.com/geocoder-php/Geocoder/blob/master/src/Provider/GoogleMaps/

sheub · 2018-06-04T11:58:01Z

I'll have a look, but I am using the Google Maps provider with only one key (GOOGLEMAP_API_KEY)

sheub · 2018-06-12T17:45:32Z

Maybe someone could tell me if it is for sure possible to get the caching to work with the 2 Credential keys, before I spend some more hours on it. Thank you.

…ys (appID and AppCode) of the Here Provider

…erseQuery and testReverseQueryWithNoResults

sheub · 2018-06-22T10:25:28Z

So, The TravisCI build have passed :).

I still had to do an override of the class CachedResponseClient to add the appCode variable which is needed for the Here provider. The override HereCachedResponseClient.php file is loaded in Here\Tests\IntegrationTest with require_once.

This override could be omitted simply by adding the variable to Geocoder\IntegrationTest\CachedResponseClient directly. This is a slight modification which would not change anything for the other providers.
This would be my prefered solution as it would work also for all other providers which need 2 keys (e.g. Algolia places).

Nyholm

Great work!

Thanks for all the reviews and your work

Nyholm · 2018-06-26T15:36:06Z

src/Provider/Here/Here.php

+    /**
+     * @var string
+     */
+    private $appCode = null;


You do not need the = null

Nyholm · 2018-06-26T15:37:17Z

src/Provider/Here/Readme.md

+```bash
+composer require geocoder-php/here-provider
+```
+### Note


This line should be removed.

Nyholm · 2018-06-26T15:37:32Z

src/Provider/Here/Readme.md

+composer require geocoder-php/here-provider
+```
+### Note
+## App Id and App Code


Use ### instead of ##

Nyholm · 2018-06-26T15:37:36Z

src/Provider/Here/Readme.md

+## App Id and App Code
+Get your Here credentials at https://developer.here.com/
+
+## Language parameter


Use ### instead of ##

Nyholm · 2018-06-26T15:38:04Z

src/Provider/Here/Tests/HereCachedResponseClient.php

+ *
+ * @author Sébastien Barré <sebastien@sheub.eu> override the provider-integration-tests: CachedResponseClient class
+ */
+class HereCachedResponseClient extends CachedResponseClient


This is the best solution I could think of. I suggest to modify the CachedResponseClient only if other providers also need this functionality.

Nyholm · 2018-06-26T15:40:59Z

I added some notes. These are just minors. I will merge this PR now and prepare the package.

Nyholm · 2018-06-26T17:10:01Z

The package is added: https://packagist.org/packages/geocoder-php/here-provider

You could improve your tests now when 1.1.0 is released of the integration tests. That version includes your PR.

Let me know when I should tag version 1.0.0 of this new package. (Ie after you test it a little bit)

@param

* Add Here Provider * Rename here.php to Here.php * Update Here.php * Update HereAddress.php * Update Here.php * Update HereAddress.php * Update Here.php * Update Here.php * Review Corrections * + * @param string $appId An App ID. + * @param string $apoCode An App code. * Add Tests files * Add phpunit.xml.dist * Corrections after running tests * use{} * FIX IT! * Few Styles #844 * HereMaps->Here * .Some more tests * style * Build Test OK if $testIpv4 = false; * Update HereTest.php * Code review from Baachi * Update Here.php * Update Readme.md * updated Here_Test and cached_responses * Add <server name="HERE_APP_ID" value="YOUR_APP_ID" /> <server name="HERE_APP_CODE" value="YOUR_APP_CODE" /> * Update Readme.md * Reworking of the Tests with adjustments to fit the two Credentials keys (appID and AppCode) of the Here Provider * style-ci correction + override testGeocodeQueryWithNoResults, testReverseQuery and testReverseQueryWithNoResults * Styles * Update IntegrationTest.php * Correct the extends and the naming (to HereCachedResponseClient) * Remove Overriding file HereCachedResponseClient.php * update cached_responses * set provider-integration-tests requirement 1.1.0 * Update Readme.md * Update Readme.md * Update composer.json

sheub and others added 2 commits April 15, 2018 15:11

Add Here Provider

48cb39f

Rename here.php to Here.php

5de7717

willdurand self-requested a review April 15, 2018 13:25

sheub added 6 commits April 15, 2018 15:36

Update Here.php

97a4224

Update HereAddress.php

d643c17

Update Here.php

174a3ed

Update HereAddress.php

da878f6

Update Here.php

a1a1ef7

Update Here.php

89f167c

Nyholm reviewed Apr 16, 2018

View reviewed changes

sheub added 2 commits April 16, 2018 09:52

Review Corrections

1b14a89

+ * @param string $appId An App ID.

fe848a4

+ * @param string $apoCode An App code.

sheub and others added 11 commits April 17, 2018 21:05

Add Tests files

89375e8

Add phpunit.xml.dist

1b16397

Corrections after running tests

fc7042a

use{}

b05c424

FIX IT!

26924bf

Few Styles #844

1d8a48d

HereMaps->Here

75ba5b2

.Some more tests

04aac0a

style

13560f3

Build Test OK if $testIpv4 = false;

f0a5388

Update HereTest.php

693de43

Baachi reviewed Apr 19, 2018

View reviewed changes

Code review from Baachi

fbe308f

Merge branch 'master' of https://github.com/sheub/Geocoder

4a17fa5

Add

51b0867

<server name="HERE_APP_ID" value="YOUR_APP_ID" /> <server name="HERE_APP_CODE" value="YOUR_APP_CODE" />

sheub closed this May 27, 2018

sheub reopened this May 27, 2018

sheub and others added 8 commits June 20, 2018 19:39

Update Readme.md

2f9edda

Reworking of the Tests with adjustments to fit the two Credentials ke…

23c1d76

…ys (appID and AppCode) of the Here Provider

Merge branch 'master' of https://github.com/sheub/Geocoder

30c5319

style-ci correction + override testGeocodeQueryWithNoResults, testRev…

016d991

…erseQuery and testReverseQueryWithNoResults

Styles

9d1e4c7

Update IntegrationTest.php

e0ee829

Correct the extends and the naming (to HereCachedResponseClient)

cbeed07

Merge branch 'master' of https://github.com/sheub/Geocoder

c31e8cd

sheub mentioned this pull request Jun 22, 2018

Add appCode to CachedResponseClient geocoder-php/provider-integration-tests#14

Merged

Nyholm approved these changes Jun 26, 2018

View reviewed changes

Nyholm merged commit a76ed30 into geocoder-php:master Jun 26, 2018


		The change log describes what is "Added", "Removed", "Changed" or "Fixed" between each release.

		## 4.0.0

		@@ -0,0 +1,21 @@
		The MIT License (MIT)

		Copyright (c) 2011 — William Durand <william.durand1@gmail.com>

Add Here Provider #844

Add Here Provider #844

Uh oh!

Conversation

sheub commented Apr 15, 2018

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mechazawa commented Apr 16, 2018

Uh oh!

sheub commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Apr 23, 2018

Uh oh!

sheub commented May 7, 2018

Uh oh!

sheub commented May 27, 2018

Uh oh!

Baachi commented May 28, 2018

Uh oh!

sheub commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sheub commented May 29, 2018

Uh oh!

sheub commented May 31, 2018

Uh oh!

sheub commented Jun 4, 2018

Uh oh!

jbelien commented Jun 4, 2018

Uh oh!

sheub commented Jun 4, 2018

Uh oh!

sheub commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sheub commented Jun 22, 2018

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jun 26, 2018

Uh oh!

Nyholm commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sheub commented May 28, 2018 •

edited

Loading

sheub commented Jun 12, 2018 •

edited

Loading