Added: DefaultResultFactory and MultipleResultFactory classes - Fix #223

[toin0u]

Hi,

What do you think @willdurand @Baachi @mrkrstphr ?
Thanks to @Baachi for his idea :)

Btw tests are fixed by #228

[Baachi]

I think, mark the method as final is not really necessary. WDY @willdurand @toin0u

[toin0u]

I kept this because of @willdurand's comment: #182 (comment)

[Baachi]

But you wrote here that the use should extend from this class to implement his own logic, but this isn't possible with final.

[toin0u]

Yes createFromArray can't be extended but newInstance can as I did here: https://github.com/toin0u/Geotools/blob/master/src/Geotools/Batch/BatchResult.php

[willdurand]

The idea is to provide its own ResultInterface instance, not to change the data type returned by the factory. So I think it's correct.

[Baachi]

@willdurand Then it would be better to mark the class as final and write it down to the docs?

[toin0u]

Well there is now ResultFactoryInterface so it's possible to mark DefaultResultFactory and MultipleResultFactory as final but it will introduce a BC break.

[Baachi]

@toin0u Good catch! Its not worth enough to introduce a BC break, but we should update the docs.

[toin0u]

@Baachi You're totaly right ! We need to update the docs 👍

[toin0u]

Docs updated. Is it good enough ?

[willdurand]

Perfect, thank you!

[toin0u]

@Baachi @willdurand Thanks :)

[Baachi]

Perfect 👍

[willdurand]

mmh, we should update the providers, isn'it?

[toin0u]

Yes... And we should update the doc because not every providers return multilple results.
Sorry, my bad :S

[toin0u]

Ok I'm looking for this and we have a problem when:

• the provider returns one result and we use MultileResultFactory
• the provider returns multiple results and we use DefaultResultFactory

Should we throw an exception in the factory ? Or should factories be more similar by using \SplObjectStorage(BC break) ? @willdurand @Baachi WDYT ?

[Baachi]

I would avoid a BC break. Maybe exceptions are the way to go.

[willdurand]

I don't understand.

the provider returns one result and we use MultileResultFactory

then, there will be a collection with one item, no problem

the provider returns multiple results and we use DefaultResultFactory

then, the first one will be used, which is the current behavior

[toin0u]

I understand what you mean but the problem is:

• the DefaultResultFactory expects an array (gist)
• the MultipleResultFactory expects an array of arrays (gist)

This GoogleMapsProvoder returns:

• one result for 10 rue gambetta, Paris, france
• multiple results for Paris

So if you got one result with MultipleResultFactory, we got a fatal error here. And multiple results with DefaultResultFactory, we got a null Geocoded object.

I hope I was clear enough and you can see this point :)

[willdurand]

No I still don't understand, each provider should return an array of arrays.

[toin0u]

It's true, it's my mistake, I should wrote array of arrays and array of associate arrays.

Single result:

Array
(
    [latitude] => 48.8234055
    [longitude] => 2.3072664
    ...
)

Multiple results:

Array
(
    [0] => Array
        (
            [latitude] => 48.8234055
            [longitude] => 2.3072664
            ...
        )
    ...
)

[willdurand]

Providers HAVE TO return all results. The Factory makes the final decision.

[toin0u]

I understand what you meant now.

We misunderstood each other because I was exposing a problem when providers return one or multiple results.. And you already assumed that providers return all results :)