-
Notifications
You must be signed in to change notification settings - Fork 523
Google component filters #466
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
14dc202
ce85718
6bc3b79
87390a6
c20c600
17d7dd0
64002bf
6909cdb
50dfe81
1824b3f
2d64fab
ac4921a
1fe3a88
c10cf73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,11 @@ final class GoogleMaps extends AbstractHttpProvider implements LocaleAwareProvid | |
| */ | ||
| private $region; | ||
|
|
||
| /** | ||
| * @var array | ||
| */ | ||
| private $componentFilters; | ||
|
|
||
| /** | ||
| * @var bool | ||
| */ | ||
|
|
@@ -133,13 +138,33 @@ public function getName() | |
| return 'google_maps'; | ||
| } | ||
|
|
||
| /** | ||
| * @param $region | ||
| * | ||
| * @return $this | ||
| */ | ||
| public function setRegion($region) | ||
| { | ||
| $this->region = $region; | ||
|
|
||
| return $this; | ||
| } | ||
|
|
||
| /** | ||
| * Add components for filtering. | ||
| * https://developers.google.com/maps/documentation/geocoding/#ComponentFiltering | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really good that you provide the link, but it should be updated. |
||
| * | ||
| * @param array $filters | ||
| * | ||
| * @return $this | ||
| */ | ||
| public function setComponentFilters(array $filters) | ||
| { | ||
| $this->componentFilters = $filters; | ||
|
|
||
| return $this; | ||
| } | ||
|
|
||
| /** | ||
| * @param string $query | ||
| * | ||
|
|
@@ -167,11 +192,21 @@ private function buildQuery($query) | |
| } | ||
| } | ||
|
|
||
| if (!empty($this->componentFilters)) { | ||
| $componentFilters = $this->componentFilters; | ||
|
|
||
| $query = sprintf('%s&key=%s&components=%s', $query, $this->apiKey, implode('|', array_map(function($key) use ($componentFilters) { | ||
| return $key.':'.urlencode($componentFilters[$key]); | ||
| }, array_keys($componentFilters)))); | ||
| } | ||
|
|
||
| return $query; | ||
| } | ||
|
|
||
| /** | ||
| * @param string $query | ||
| * @return \Geocoder\Model\AddressCollection | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this. It is not a part of the PR and it is no longer valid. |
||
| * @throws Exception | ||
| */ | ||
| private function executeQuery($query) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -427,4 +427,64 @@ public function testGeocodeWithInvalidClientIdAndKeyNoSsl() | |
| $provider = GoogleMaps::business($this->getAdapter(), 'foo', 'bogus', null, null, false); | ||
| $provider->geocode('Columbia University'); | ||
| } | ||
|
|
||
| /** | ||
| * @expectedException \Geocoder\Exception\NoResult | ||
| * @expectedExceptionMessage Could not execute query | ||
| */ | ||
| public function testGeocodeWithComponentFiltersInvalidCountry() | ||
| { | ||
| $provider = new GoogleMaps($this->getAdapter(), 'fr-FR', 'Île-de-France'); | ||
| $arrFilters = ['country' => 'ru', 'postal_code' => '75020']; | ||
| $provider->setComponentFilters($arrFilters); | ||
|
|
||
| $provider->geocode('10 avenue Gambetta, Paris, France'); | ||
| } | ||
|
|
||
| /** | ||
| * @expectedException \Geocoder\Exception\NoResult | ||
| * @expectedExceptionMessage Could not execute query | ||
| */ | ||
| public function testGeocodeWithComponentFiltersInvalidZipCode() | ||
| { | ||
| $provider = new GoogleMaps($this->getAdapter(), 'fr-FR', 'Île-de-France'); | ||
| $arrFilters = ['country' => 'fr', 'postal_code' => '00000']; | ||
| $provider->setComponentFilters($arrFilters); | ||
|
|
||
| $provider->geocode('10 avenue Gambetta, Paris, France'); | ||
| } | ||
|
|
||
| public function testGeocodeWithValidComponentFilters() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this test be different without setComponentFilters? I suggest that you only write a unit test that make sure the correct parameters is added on the URL. We already have functional tests that make sure the response handing is proper.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ComponentFilters are 100% optional - so without it this test will not be different, but it will fail 100% Not sure how i can redo this - any suggestions are welcome.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing this functional tests we could do unit tests that verifies that the query URL has changed when using setComponentFilters. We do not actually need to execute the query and verify the result.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are 100% right, but
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you mock the HttpClient and verify that the sendRequest function has a request with the preferred query?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be done, sure. Will do if no one was willing to disagree or suggest something better.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes it is outside the scope. I do not think you have to refactor anything. Just update the test code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mvcaaa i did something silimar for googles |
||
| { | ||
| $provider = new GoogleMaps($this->getAdapter(), 'fr-FR', 'Île-de-France'); | ||
| $arrFilters = ['country' => 'fr', 'postal_code' => '75020']; | ||
| $provider->setComponentFilters($arrFilters); | ||
|
|
||
| $results = $provider->geocode('10 avenue Gambetta, Paris, France'); | ||
|
|
||
| $this->assertInstanceOf('Geocoder\Model\AddressCollection', $results); | ||
| $this->assertCount(1, $results); | ||
|
|
||
| /** @var \Geocoder\Model\Address $result */ | ||
| $result = $results->first(); | ||
| $this->assertInstanceOf('\Geocoder\Model\Address', $result); | ||
| $this->assertEquals(48.8630462, $result->getLatitude(), '', 0.001); | ||
| $this->assertEquals(2.3882487, $result->getLongitude(), '', 0.001); | ||
| $this->assertTrue($result->getBounds()->isDefined()); | ||
| $this->assertEquals(48.8630462, $result->getBounds()->getSouth(), '', 0.001); | ||
| $this->assertEquals(2.3882487, $result->getBounds()->getWest(), '', 0.001); | ||
| $this->assertEquals(48.8630462, $result->getBounds()->getNorth(), '', 0.001); | ||
| $this->assertEquals(2.3882487, $result->getBounds()->getEast(), '', 0.001); | ||
| $this->assertEquals(10, $result->getStreetNumber()); | ||
| $this->assertEquals('Avenue Gambetta', $result->getStreetName()); | ||
| $this->assertEquals(75020, $result->getPostalCode()); | ||
| $this->assertEquals('Paris', $result->getLocality()); | ||
| $this->assertEquals('Paris', $result->getAdminLevels()->get(2)->getName()); | ||
| $this->assertEquals('Île-de-France', $result->getAdminLevels()->get(1)->getName()); | ||
| $this->assertEquals('France', $result->getCountry()->getName()); | ||
| $this->assertEquals('FR', $result->getCountry()->getCode()); | ||
| // not provided | ||
| $this->assertNull($result->getTimezone()); | ||
| } | ||
|
|
||
| } | ||
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.
This doc block is not part of the PR. You can keep this but try to avoid "code cleanups" in the same PR as features.