Skip to content
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

Improve Nominatim Provider #806

Merged
merged 9 commits into from Jan 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
177 changes: 177 additions & 0 deletions src/Provider/Nominatim/Model/NominatimAddress.php
@@ -0,0 +1,177 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Geocoder package.
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @license MIT License
*/

namespace Geocoder\Provider\Nominatim\Model;

use Geocoder\Model\Address;

/**
* @author Jonathan Beliën <jbe@geo6.be>
*/
final class NominatimAddress extends Address
{
/**
* @var string|null
*/
private $attribution;

/**
* @var string|null
*/
private $class;

/**
* @var string|null
*/
private $displayName;

/**
* @var string|null
*/
private $osmType;

/**
* @var int|null
*/
private $osmId;

/**
* @var string|null
*/
private $type;

/**
* @return null|string
*/
public function getAttribution()
{
return $this->attribution;
}

/**
* @param null|string $attribution
*
* @return NominatimAddress
*/
public function withAttribution(string $attribution = null): self
{
$new = clone $this;
$new->attribution = $attribution;

return $new;
}

/**
* @return null|string
*/
public function getClass()
{
return $this->class;
}

/**
* @param null|string $class
*
* @return NominatimAddress
*/
public function withClass(string $class = null): self
{
$new = clone $this;
$new->class = $class;

return $new;
}

/**
* @return null|string
*/
public function getDisplayName()
{
return $this->displayName;
}

/**
* @param null|string $displayName
*
* @return NominatimAddress
*/
public function withDisplayName(string $displayName = null): self
{
$new = clone $this;
$new->displayName = $displayName;

return $new;
}

/**
* @return null|int
*/
public function getOSMId()
{
return $this->osmId;
}

/**
* @param null|int $osmId
*
* @return NominatimAddress
*/
public function withOSMId(int $osmId = null): self
{
$new = clone $this;
$new->osmId = $osmId;

return $new;
}

/**
* @return null|string
*/
public function getOSMType()
{
return $this->osmType;
}

/**
* @param null|string $osmType
*
* @return NominatimAddress
*/
public function withOSMType(string $osmType = null): self
{
$new = clone $this;
$new->osmType = $osmType;

return $new;
}

/**
* @return null|string
*/
public function getType()
{
return $this->type;
}

/**
* @param null|string $type
*
* @return NominatimAddress
*/
public function withType(string $type = null): self
{
$new = clone $this;
$new->type = $type;

return $new;
}
}
28 changes: 23 additions & 5 deletions src/Provider/Nominatim/Nominatim.php
Expand Up @@ -22,10 +22,12 @@
use Geocoder\Query\ReverseQuery;
use Geocoder\Http\Provider\AbstractHttpProvider;
use Geocoder\Provider\Provider;
use Geocoder\Provider\Nominatim\Model\NominatimAddress;
use Http\Client\HttpClient;

/**
* @author Niklas Närhinen <niklas@narhinen.net>
* @author Jonathan Beliën <jbe@geo6.be>
*/
final class Nominatim extends AbstractHttpProvider implements Provider
{
Expand Down Expand Up @@ -80,6 +82,7 @@ public function geocodeQuery(GeocodeQuery $query): Collection
}

$searchResult = $doc->getElementsByTagName('searchresults')->item(0);
$attribution = $searchResult->getAttribute('attribution');
$places = $searchResult->getElementsByTagName('place');

if (null === $places || 0 === $places->length) {
Expand All @@ -88,7 +91,7 @@ public function geocodeQuery(GeocodeQuery $query): Collection

$results = [];
foreach ($places as $place) {
$results[] = $this->xmlResultToArray($place, $place);
$results[] = $this->xmlResultToArray($place, $place, $attribution, false);
}

return new AddressCollection($results);
Expand All @@ -111,10 +114,11 @@ public function reverseQuery(ReverseQuery $query): Collection
}

$searchResult = $doc->getElementsByTagName('reversegeocode')->item(0);
$attribution = $searchResult->getAttribute('attribution');
$addressParts = $searchResult->getElementsByTagName('addressparts')->item(0);
$result = $searchResult->getElementsByTagName('result')->item(0);

return new AddressCollection([$this->xmlResultToArray($result, $addressParts)]);
return new AddressCollection([$this->xmlResultToArray($result, $addressParts, $attribution, true)]);
}

/**
Expand All @@ -123,7 +127,7 @@ public function reverseQuery(ReverseQuery $query): Collection
*
* @return Location
*/
private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressNode): Location
private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressNode, string $attribution, bool $reverse): Location
{
$builder = new AddressBuilder($this->getName());

Expand All @@ -138,6 +142,7 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN
if (!empty($postalCode)) {
$postalCode = current(explode(';', $postalCode));
}
$builder->setPostalCode($postalCode);

$localityFields = ['city', 'town', 'village', 'hamlet'];
foreach ($localityFields as $localityField) {
Expand All @@ -149,7 +154,6 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN
}
}

$builder->setPostalCode($postalCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this moved?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed more logical to regroup postal code process ;
But I really don't mind putting it back to line 152 ^_^

$builder->setStreetName($this->getNodeValue($addressNode->getElementsByTagName('road')) ?: $this->getNodeValue($addressNode->getElementsByTagName('pedestrian')));
$builder->setStreetNumber($this->getNodeValue($addressNode->getElementsByTagName('house_number')));
$builder->setSubLocality($this->getNodeValue($addressNode->getElementsByTagName('suburb')));
Expand All @@ -160,6 +164,7 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN
$countryCode = strtoupper($countryCode);
}
$builder->setCountryCode($countryCode);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why what this blank line added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment, seemd to make more sense to create a "block" with country code process ;
But again, really don't mind putting that back as it was :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey. I just saw a weird diff. This is fine

$builder->setCoordinates($resultNode->getAttribute('lat'), $resultNode->getAttribute('lon'));

$boundsAttr = $resultNode->getAttribute('boundingbox');
Expand All @@ -169,7 +174,20 @@ private function xmlResultToArray(\DOMElement $resultNode, \DOMElement $addressN
$builder->setBounds($bounds['south'], $bounds['west'], $bounds['north'], $bounds['east']);
}

return $builder->build();
$location = $builder->build(NominatimAddress::class);
$location = $location->withAttribution($attribution);
$location = $location->withOSMId(intval($resultNode->getAttribute('osm_id')));
$location = $location->withOSMType($resultNode->getAttribute('osm_type'));

if (false === $reverse) {
$location = $location->withClass($resultNode->getAttribute('class'));
$location = $location->withDisplayName($resultNode->getAttribute('display_name'));
$location = $location->withType($resultNode->getAttribute('type'));
} else {
$location = $location->withDisplayName($resultNode->nodeValue);
}

return $location;
}

/**
Expand Down
@@ -0,0 +1,4 @@
s:1431:"<?xml version="1.0" encoding="UTF-8" ?>
<searchresults timestamp='Fri, 29 Dec 17 15:02:42 +0000' attribution='Data © OpenStreetMap contributors, ODbL 1.0. http://www.openstreetmap.org/copyright' querystring='35 avenue jean de bologne 1020 bruxelles' polygon='false' exclude_place_ids='120528821' more_url='https://nominatim.openstreetmap.org/search.php?q=35+avenue+jean+de+bologne+1020+bruxelles&amp;exclude_place_ids=120528821&amp;addressdetails=1&amp;format=xml'>
<place place_id='120528821' osm_type='way' osm_id='220754533' place_rank='30' boundingbox="50.896294,50.8963934,4.3604816,4.3607151" lat='50.89634435' lon='4.36059848989059' display_name='35, Avenue Jean de Bologne - Jean de Bolognelaan, Heysel - Heizel, Laeken / Laken, Ville de Bruxelles - Stad Brussel, Brussel-Hoofdstad - Bruxelles-Capitale, Région de Bruxelles-Capitale - Brussels Hoofdstedelijk Gewest, 1020, België / Belgique / Belgien' class='building' type='yes' importance='0.721'>
<house_number>35</house_number><road>Avenue Jean de Bologne - Jean de Bolognelaan</road><suburb>Heysel - Heizel</suburb><city_district>Laeken / Laken</city_district><city>Ville de Bruxelles - Stad Brussel</city><county>Brussel-Hoofdstad - Bruxelles-Capitale</county><state>Région de Bruxelles-Capitale - Brussels Hoofdstedelijk Gewest</state><postcode>1020</postcode><country>België / Belgique / Belgien</country><country_code>be</country_code></place></searchresults>";
27 changes: 27 additions & 0 deletions src/Provider/Nominatim/Tests/NominatimTest.php
Expand Up @@ -111,4 +111,31 @@ public function testGetNodeStreetName()
$this->assertInstanceOf('\Geocoder\Model\Address', $result);
$this->assertEquals('Rue Quincampoix', $result->getStreetName());
}

public function testGeocodeWithRealAddress()
{
$provider = Nominatim::withOpenStreetMapServer($this->getHttpClient());
$results = $provider->geocodeQuery(GeocodeQuery::create('35 avenue jean de bologne 1020 bruxelles'));

$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(50.896344, $result->getCoordinates()->getLatitude(), '', 0.01);
$this->assertEquals(4.3605984, $result->getCoordinates()->getLongitude(), '', 0.01);
$this->assertEquals('Avenue Jean de Bologne - Jean de Bolognelaan', $result->getStreetName());
$this->assertEquals('1020', $result->getPostalCode());
$this->assertEquals('Ville de Bruxelles - Stad Brussel', $result->getLocality());
$this->assertEquals('Heysel - Heizel', $result->getSubLocality());
$this->assertEquals('BE', $result->getCountry()->getCode());

$this->assertEquals('Data © OpenStreetMap contributors, ODbL 1.0. http://www.openstreetmap.org/copyright', $result->getAttribution());
$this->assertEquals('building', $result->getClass());
$this->assertEquals('35, Avenue Jean de Bologne - Jean de Bolognelaan, Heysel - Heizel, Laeken / Laken, Ville de Bruxelles - Stad Brussel, Brussel-Hoofdstad - Bruxelles-Capitale, Région de Bruxelles-Capitale - Brussels Hoofdstedelijk Gewest, 1020, België / Belgique / Belgien', $result->getDisplayName());
$this->assertEquals(220754533, $result->getOSMId());
$this->assertEquals('way', $result->getOSMType());
$this->assertEquals('yes', $result->getType());
}
}