Merge pull request #291 from JeroenDeDauw/fix-qp-geoservice

Fixed geoservice usage for center parameter in the map printer

README.md

@@ -61,6 +61,8 @@ On [Packagist](https://packagist.org/packages/mediawiki/maps):
 
 ### Running the tests
 
+As setup, run `composer install` inside of the Maps root directory.
+
 You can run the MediaWiki independent tests by changing into the Maps root directory and running
 
     phpunit
@@ -69,11 +71,13 @@ This is possible without having a MediaWiki installation or webserver. A clone o
 
 To run the tests with MediaWiki, change into `tests/phpunit` of your MediaWiki installation and run
 
-    php phpunit.php --wiki wiki --configuration suite.xml ../../extensions/Maps/tests/
+    php phpunit.php --wiki wiki -c ../../extensions/Maps/phpunit.xml.dist
 
 Where you either update `wiki` to match your wikis name, or drop the parameter. The above command
 works without modification if you are using the [MediaWiki Vagrant](https://www.mediawiki.org/wiki/MediaWiki-Vagrant).
 
+Beware that due to severe technical debt, some tests access the network.
+
 ## Links
 
 * [Maps examples](https://www.semantic-mediawiki.org/wiki/Maps_examples)

SemanticMaps/src/queryprinters/SM_MapPrinter.php

@@ -3,7 +3,9 @@
 use Maps\Elements\Location;
 use Maps\Element;
 use Maps\Elements\BaseElement;
+use Maps\LocationParser;
 use ParamProcessor\ParamDefinition;
+use ValueParsers\ParserOptions as ValueParserOptions;
 
 /**
  * Query printer for maps. Is invoked via SMMapper.
@@ -19,6 +21,11 @@ class SMMapPrinter extends SMW\ResultPrinter {
 
 	private static $services = [];
 
+	/**
+	 * @var LocationParser
+	 */
+	private $locationParser;
+
 	/**
 	 * @since 3.4
 	 * FIXME: this is a temporary hack that should be replaced when SMW allows for dependency
@@ -67,7 +74,7 @@ private function getParameterInfo() {
 		$this->service->addParameterInfo( $params );
 
 		$params['staticlocations'] = [
-			'type' => 'mapslocation',
+			'type' => 'mapslocation', // FIXME: geoservice is not used
 			'aliases' => [ 'locations', 'points' ],
 			'default' => [],
 			'islist' => true,
@@ -151,8 +158,10 @@ public final function getResultText( SMWQueryResult $res, $outputmode ) {
 
 		$params = $this->params;
 
+		$this->initializeLocationParser( $params );
+
 		$queryHandler = new SMQueryHandler( $res, $outputmode );
-		$queryHandler->setLinkStyle($params['link']);
+		$queryHandler->setLinkStyle( $params['link'] );
 		$queryHandler->setHeaderStyle($params['headers']);
 		$queryHandler->setShowSubject( $params['showtitle'] );
 		$queryHandler->setTemplate( $params['template'] );
@@ -161,10 +170,11 @@ public final function getResultText( SMWQueryResult $res, $outputmode ) {
 		$queryHandler->setActiveIcon( $params['activeicon'] );
 
 		$this->handleMarkerData( $params, $queryHandler );
-		$locationAmount = count( $params['locations'] );
 
 		$params['ajaxquery'] = urlencode( $params['ajaxquery'] );
 
+		$locationAmount = count( $params['locations'] );
+
 		if ( $locationAmount > 0 ) {
 			// We can only take care of the zoom defaulting here,
 			// as not all locations are available in whats passed to Validator.
@@ -195,6 +205,58 @@ public final function getResultText( SMWQueryResult $res, $outputmode ) {
 		}
 	}
 
+	private function initializeLocationParser( array $params ) {
+		$this->locationParser = new LocationParser( new ValueParserOptions( [
+			'geoService' => $params['geoservice']
+		] ) );
+	}
+
+	/**
+	 * Converts the data in the coordinates parameter to JSON-ready objects.
+	 * These get stored in the locations parameter, and the coordinates on gets deleted.
+	 *
+	 * @param array &$params
+	 * @param SMQueryHandler $queryHandler
+	 */
+	private function handleMarkerData( array &$params, SMQueryHandler $queryHandler ) {
+		$params['centre'] = $this->getCenter( $params['centre'] );
+
+		$iconUrl = MapsMapper::getFileUrl( $params['icon'] );
+		$visitedIconUrl = MapsMapper::getFileUrl( $params['visitedicon'] );
+
+		$params['locations'] = $this->getJsonForStaticLocations(
+			$params['staticlocations'],
+			$params,
+			$iconUrl,
+			$visitedIconUrl
+		);
+
+		unset( $params['staticlocations'] );
+
+		$this->addShapeData( $queryHandler->getShapes(), $params, $iconUrl, $visitedIconUrl );
+
+		if ( $params['format'] === 'openlayers' ) {
+			$params['layers'] = MapsDisplayMapRenderer::evilOpenLayersHack( $params['layers'] );
+		}
+	}
+
+	private function getCenter( $coordinatesOrAddress ) {
+		if ( $coordinatesOrAddress === false ) {
+			return false;
+		}
+
+		try {
+			// FIXME: a Location makes no sense here, since the non-coordinate data is not used
+			$location = $this->locationParser->stringParse( $coordinatesOrAddress );
+		}
+		catch ( \Exception $ex ) {
+			// TODO: somehow report this to the user
+			return false;
+		}
+
+		return $location->getJSONObject();
+	}
+
 	/**
 	 * Returns the HTML to display the map.
 	 *
@@ -232,37 +294,6 @@ private function getMapHTML( array $params, Parser $parser, $mapName ) {
 	private function getJSONObject( array $params, Parser $parser ) {
 		return $params;
 	}
-
-	/**
-	 * Converts the data in the coordinates parameter to JSON-ready objects.
-	 * These get stored in the locations parameter, and the coordinates on gets deleted.
-	 * 
-	 * @param array &$params
-	 * @param SMQueryHandler $queryHandler
-	 */
-	private function handleMarkerData( array &$params, SMQueryHandler $queryHandler ) {
-		if ( is_object( $params['centre'] ) ) {
-			$params['centre'] = $params['centre']->getJSONObject();
-		}
-
-		$iconUrl = MapsMapper::getFileUrl( $params['icon'] );
-		$visitedIconUrl = MapsMapper::getFileUrl( $params['visitedicon'] );
-
-		$params['locations'] = $this->getJsonForStaticLocations(
-			$params['staticlocations'],
-			$params,
-			$iconUrl,
-			$visitedIconUrl
-		);
-
-		unset( $params['staticlocations'] );
-
-		$this->addShapeData( $queryHandler->getShapes(), $params, $iconUrl, $visitedIconUrl );
-
-		if ( $params['format'] === 'openlayers' ) {
-			$params['layers'] = MapsDisplayMapRenderer::evilOpenLayersHack( $params['layers'] );
-		}
-	}
 
 	private function getJsonForStaticLocations( array $staticLocations, array $params, $iconUrl, $visitedIconUrl ) {
 		/**

includes/Maps_DisplayMapRenderer.php

@@ -2,6 +2,7 @@
 
 use Maps\Element;
 use Maps\Elements\Location;
+use Maps\LocationParser;
 use ValueParsers\ParserOptions as ValueParserOptions;
 
 /**
@@ -15,6 +16,11 @@ class MapsDisplayMapRenderer {
 
 	private $service;
 
+	/**
+	 * @var LocationParser
+	 */
+	private $locationParser;
+
 	public function __construct( MapsMappingService $service ) {
 		$this->service = $service;
 	}
@@ -29,6 +35,8 @@ public function __construct( MapsMappingService $service ) {
 	 * @return string
 	 */
 	public final function renderMap( array $params, Parser $parser ) {
+		$this->initializeLocationParser( $params );
+
 		$this->handleMarkerData( $params, $parser );
 
 		$mapName = $this->service->getMapId();
@@ -47,6 +55,12 @@ public final function renderMap( array $params, Parser $parser ) {
 		return $output;
 	}
 
+	private function initializeLocationParser( array $params ) {
+		$this->locationParser = new LocationParser( new ValueParserOptions( [
+			'geoService' => $params['geoservice']
+		] ) );
+	}
+
 	/**
 	 * Returns the HTML to display the map.
 	 *
@@ -77,9 +91,7 @@ protected function getMapHTML( array $params, $mapName ) {
 	 * These get stored in the locations parameter, and the coordinates on gets deleted.
 	 */
 	private function handleMarkerData( array &$params, Parser $parser ) {
-		if ( is_object( $params['centre'] ) ) {
-			$params['centre'] = $params['centre']->getJSONObject();
-		}
+		$params['centre'] = $this->getCenter( $params['centre'] );
 
 		$parserClone = clone $parser;
 
@@ -98,19 +110,32 @@ private function handleMarkerData( array &$params, Parser $parser ) {
 		}
 	}
 
+	private function getCenter( $coordinatesOrAddress ) {
+		if ( $coordinatesOrAddress === false ) {
+			return false;
+		}
+
+		try {
+			// FIXME: a Location makes no sense here, since the non-coordinate data is not used
+			$location = $this->locationParser->stringParse( $coordinatesOrAddress );
+		}
+		catch ( \Exception $ex ) {
+			// TODO: somehow report this to the user
+			return false;
+		}
+
+		return $location->getJSONObject();
+	}
+
 	private function getLocationJson( array $params, $parserClone ) {
 		$iconUrl = MapsMapper::getFileUrl( $params['icon'] );
 		$visitedIconUrl = MapsMapper::getFileUrl( $params['visitedicon'] );
 
-		$parser = new \Maps\LocationParser( new ValueParserOptions( [
-			'geoService' => $params['geoservice']
-		] ) );
-
 		$locationJsonObjects = [];
 
 		foreach ( $params['coordinates'] as $coordinatesOrAddress ) {
 			try {
-				$location = $parser->stringParse( $coordinatesOrAddress );
+				$location = $this->locationParser->stringParse( $coordinatesOrAddress );
 			}
 			catch ( \Exception $ex ) {
 				// TODO: somehow report this to the user

includes/Maps_Mapper.php

@@ -104,7 +104,7 @@ public static function getCommonParameters() {
 		// TODO$manipulation->toJSONObj = true;
 
 		$params['centre'] = [
-			'type' => 'mapslocation',
+			'type' => 'string',
 			'aliases' => [ 'center' ],
 			'default' => false,
 			'manipulatedefault' => false,

includes/Maps_MappingService.php

@@ -229,4 +229,6 @@ public function getEarthZoom() {
 		return 1;
 	}
 
+	public abstract function getMapId( $increment = true );
+
 }

includes/parserhooks/Maps_Coordinates.php

@@ -1,4 +1,5 @@
 <?php
+
 use DataValues\Geo\Formatters\GeoCoordinateFormatter;
 
 /**

includes/parserhooks/Maps_Finddestination.php

@@ -1,4 +1,5 @@
 <?php
+
 use DataValues\Geo\Formatters\GeoCoordinateFormatter;
 
 /**
@@ -10,7 +11,6 @@
  * @licence GNU GPL v2+
  * @author Jeroen De Dauw < [email protected] >
  */
-
 class MapsFinddestination extends ParserHook {
 
 	/**
@@ -41,7 +41,7 @@ protected function getParameterInfo( $type ) {
 
 		$params['location'] = [
 			'dependencies' => [ 'mappingservice', 'geoservice' ],
-			'type' => 'mapslocation',
+			'type' => 'mapslocation', // FIXME: geoservice is not used
 		];
 
 		$params['format'] = [

includes/parserhooks/Maps_Geodistance.php

@@ -9,7 +9,6 @@
  * @licence GNU GPL v2+
  * @author Jeroen De Dauw < [email protected] >
  */
-
 class MapsGeodistance extends ParserHook {
 
 	/**
@@ -61,13 +60,13 @@ protected function getParameterInfo( $type ) {
 		];
 
 		$params['location1'] = [
-			'type' => 'mapslocation',
+			'type' => 'mapslocation', // FIXME: geoservice is not used
 			'aliases' => 'from',
 			'dependencies' => [ 'mappingservice', 'geoservice' ],
 		];
 
 		$params['location2'] = [
-			'type' => 'mapslocation',
+			'type' => 'mapslocation', // FIXME: geoservice is not used
 			'aliases' => 'to',
 			'dependencies' => [ 'mappingservice', 'geoservice' ],
 		];