Implement a Query object #583

Nyholm · 2017-02-19T16:26:10Z

This is an implementation of a query object suggested in #574. Ping @unkind

Changes:

All providers are stateless (removed LocaleTrait)
Geocoder extends Provider
Removed LocaleAwareProvider
Added LocaleAwareGeocoder (no methods)
Added StatefulGeocoder (this will make migrating easy)
Added GeocodeQuery, ReverseQuery
Added Provider::geocodeQuery,Provider::reverseQuery

Only GoogleMaps and Bing are using the new Provider interface.

Why we need this

The Query objects keeps providers stateless. The query object is easily extendable with extra data like bounds, locale, limit and also arbitrary data. Allowing arbitrary data will solve #546.

Using an query object over setters on the providers keeps the public interface of the providers more clean.

This PR will also solve #501.

TODO:

Make sure all providers are using the new Query objects
Update the tests
Update ProviderAggregator
Update change log

unkind · 2017-02-19T17:20:19Z

src/Model/Query/GeocodeQuery.php

+     */
+    public function withBounds(Bounds $bounds)
+    {
+        $this->bounds = $bounds;


I suggest to make query object immutable, i.e.

$query = clone $this; $query->bounds = $bounds;

GeocodeQuery is a value object.

I was thinking about making them immutable. I will make the query objects easier to reused and you will not get any unexpected behavior. Though, they are "builder". I compare the GeocodeQuery to Doctrines QueryBuilder. It would not make much sense to have the QueryBuilder immutable, would it? Or maybe Im missing something.

It would not make much sense to have the QueryBuilder immutable, would it?

I'm not that familiar with Doctrine's QB, but as I can see:

query builder is not perfect match for GeocoderQuery, query builder is not a query, it is a service; query, on other hand, is a message, queries should be compared by their values -> equality is not based on identity;

query builder probably was designed in the mutable way because of performance reasons, because cloning that object is more expensive than small object like GeocoderQuery; and number of method calls is much more higher than in your case: for complex query you may trigger clone 20-30 times very easy.

However, even for SQL query builder it probably makes sense to keep it immutable too. Sometimes you need to execute very similar queries, but with little difference (e.g. simple query + the same with COUNT(*)). AFAIU, Doctrine suggests to clone QB explicitly when you need it, but it looks like a trade-off.

I'd rather compare your query with something like Request from PSR-7 if you are in doubt. Request and Query are very similar concepts. Whenever your modify your request/query/command/event, you create different message.

Okey, Thanks =)

Thanks for the review.

unkind · 2017-02-19T17:23:01Z

src/Model/Query/Query.php

+/**
+ * @author Tobias Nyholm <tobias.nyholm@gmail.com>
+ */
+interface Query


I'm not sure if this interface is necessary.

You are correct.

unkind · 2017-02-19T17:27:33Z

src/Provider/BingMaps.php

+     * @return \Geocoder\Collection
     */
-    private function executeQuery($query)
+    private function executeQuery(Query $query, $url)


I'd pass locale and limit variables or even copy-paste URL generation for both queries.

unkind · 2017-02-20T12:34:31Z

src/Model/Query/GeocodeQuery.php

+
+    public function __clone()
+    {
+        $this->bounds = clone $this->bounds;


Bounds is an immutable structure: sharing same instance of Bounds is safe.

Nyholm · 2017-02-20T13:26:32Z

Thank you @unkind. I've update the PR.

Nyholm · 2017-04-09T12:51:24Z

Today Ive done nothing to this PR but adding tests and making sure all providers implements the new interfaces.

sagikazarmark · 2017-04-15T00:33:51Z

src/GeocoderTrait.php

+    {
+        return $this->reverseQuery(ReverseQuery::fromCoordinates($latitude, $longitude));
+    }
+}


Missing newline

sagikazarmark · 2017-04-15T00:34:35Z

src/Model/Query/GeocodeQuery.php

+<?php
+
+/**
+ * This file is part of the Geocoder package.


Is this header mandatory? If so, it's missing from a few places.

sagikazarmark · 2017-04-15T00:35:29Z

src/Model/Query/GeocodeQuery.php

+    private $data;
+
+    /**
+     * @param Coordinates $coordinates


This does not seem to be correct

sagikazarmark · 2017-04-15T00:36:13Z

src/Model/Query/GeocodeQuery.php

+    }
+
+    /**
+     * @param $text


sagikazarmark · 2017-04-15T00:38:34Z

src/Model/Query/ReverseQuery.php

+    {
+        return new self($coordinates);
+    }
+    /**


Missing newline

sagikazarmark · 2017-04-15T00:40:30Z

src/Model/Query/ReverseQuery.php

+    /**
+     * @param int $limit
+     *
+     * @return $this


Should be the class name

sagikazarmark · 2017-04-15T00:43:58Z

src/Provider/FreeGeoIp.php

     */
    const ENDPOINT_URL = 'https://freegeoip.net/json/%s';
-
+    


sagikazarmark · 2017-04-15T00:46:03Z

src/Provider/GoogleMaps.php

+     *
+     * @return GoogleMaps
+     */
    public function setRegion($region)


Is the set prefix preserved for BC reason?

No, it is a typo

sagikazarmark · 2017-04-15T00:49:02Z

src/Provider/TomTom.php

-     * @param string     $locale A locale (optional).
     */
-    public function __construct(HttpClient $client, $apiKey, $locale = null)
+    public function __construct(HttpClient $client, $apiKey)


IIRC this was $httpClient a few thousand lines above.

I do not understand. We always refer to the HttpClient as $client

sagikazarmark · 2017-04-15T00:50:54Z

src/StatefulGeocoder.php

+    /**
+     * @var Bounds
+     */
+    private $bounds;


Currently this cannot be set at all

Added a bunch of setter.

sagikazarmark · 2017-04-15T00:55:49Z

I like the idea of immutability in Providers.

As for the queries: in case of the reverse query it seems to be a bit of overkill: basically coordinates are packed into an object, which gets unpacked in the provider as is. If it's a matter of consistency and/or probable future extension then it's probably fine.

Also, it feels weird that the limit can be passed in the query as well. One could say limit is a Provider configuration. I don't have too much context, so I might be wrong about it.

Nyholm · 2017-04-15T06:32:25Z

Thank you for the review. Really appreciated.

As for the queries: in case of the reverse query it seems to be a bit of overkill: basically coordinates are packed into an object, which gets unpacked in the provider as is. If it's a matter of consistency and/or probable future extension then it's probably fine.

Yes, it is a matter of consistency and about being able to pass arbitrary data to the provider.

Also, it feels weird that the limit can be passed in the query as well. One could say limit is a Provider configuration.

Hm, I've never considered that.. I compare it to Doctrine. The Limit it part of the MySQL query and not config.

willdurand · 2017-05-02T11:57:51Z

Good job!

Implement a Query object

1feefdb

willdurand requested a review from Baachi February 19, 2017 16:26

Nyholm added this to the 4.0.0 milestone Feb 19, 2017

unkind reviewed Feb 19, 2017

View reviewed changes

Nyholm added 2 commits February 19, 2017 20:39

Make queries immutable

4aa6eef

removed query interface

40bdb8d

unkind reviewed Feb 20, 2017

View reviewed changes

Removed __clone function

30b358f

Nyholm mentioned this pull request Apr 3, 2017

Release 3.4 ? #594

Closed

Nyholm added 8 commits April 9, 2017 12:51

Updated all providers

4272dbe

Updated ProviderAggregator

35e0824

Updated geocoders

9bb7ffa

Fixing tests

a852f9a

Updating tests

bde3d88

Working on the tests

e5bc94c

Updated readme and changelog

d123ffd

Wohoo all tests pass

377a0c1

Nyholm changed the title ~~[WIP] Implement a Query object~~ Implement a Query object Apr 9, 2017

Nyholm added 3 commits April 9, 2017 14:24

Minor fixes

7fa7739

adding locale on all queries

7304af7

Added caches for open cache reverse with locale

52c7809

mikebronner mentioned this pull request Apr 13, 2017

Monitor Merge of Localization Functionality In Parent Project geocoder-php/GeocoderLaravel#75

Closed

sagikazarmark reviewed Apr 15, 2017

View reviewed changes

Bugfixes

449f0a2

Nyholm added 3 commits April 15, 2017 08:38

Removed limit from abstract provider

95e43d9

Removed Provider::MAX_RESULTS

f95438a

Minor fix

9953ad9

Nyholm merged commit 24fc43a into master Apr 28, 2017

Nyholm deleted the issue-574 branch April 28, 2017 15:01

Implement a Query object #583

Implement a Query object #583

Uh oh!

Conversation

Nyholm commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Why we need this

TODO:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkind Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

unkind Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkind Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Apr 9, 2017

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!

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!

sagikazarmark commented Apr 15, 2017

Uh oh!

Nyholm commented Apr 15, 2017

Uh oh!

willdurand commented May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Nyholm commented Feb 19, 2017 •

edited

Loading

unkind Feb 19, 2017 •

edited

Loading

unkind Feb 19, 2017 •

edited

Loading

unkind Feb 20, 2017 •

edited

Loading

Nyholm commented Feb 20, 2017 •

edited

Loading