based on #14 largely

[alcohol]

hopefully a decent compromise between requested/suggested changes and project goals

this resolves #7, closes #14

[nyamsprod]

I see that you are not validating the name and/or the currency index ? Does it mean that they are optional ?

[alcohol]

Yes I intentionally left those out, as I think this makes it easier for others to substitute them in whatever shape or form they would like to (nested object, multidimensional array, etc). As long as the lookup can be guaranteed to work through the presence of keys, we are OK I think.

[alcohol]

@nyamsprod I think this PR addresses most of your concerns in its current form now?

[nyamsprod]

One last remark about the guardAgainstInvalid* methods.

In my original proposal I had methods called validate* which were badly name but did 2 things

• validate the required key
• normalized the required key

A better name would have been filterAlpha2. The usage was as follow

$input = $this->filterAlpha2($input);
//triggers exception on failure
//return normalized key on success

The benefice of this was that you could in the class ISO3166 directly make a === comparison. With the current guardAgainstInvalid* methods. No normalization is done. So

$collection->getAlpha2('fr');
$collection->getAlpha2('FR');

won't give the same results one will trigger an exception and it would even introduced a BC break.

Apart from that everything else is :+1: for me

[frankdejonge]

@nyamsprod agreed on the filterX naming proposal 👍

[alcohol]

The thing is that if we allow custom datasets, people might have ones with lowercase keys. Which isn't necessarily disallowed. So if we use normalization by default, those would not work. Hence why I reconsidered. Otherwise we have to make the guards more strict, which would be a BC change sort of.

[nyamsprod]

If you don't normalize the input $countries data then you must at least normalize the value from the getBy method

    private function getBy($key, $value)
    {
        $value = strtoupper($value);
        foreach ($this->countries as $country) {
            if ($value === strtoupper($country[$key])) {
                return $country;
            }
        }
        throw new \OutOfBoundsException(sprintf('No "%s" key found matching: %s', $key, $value));
    }

bottom line you need to normalize the input at some point otherwise you introduce a BC break. Having said that the filter* behaviour is similar to how PHP's filter_var function works. On success the function may normalize/format the input whereas on failure the function returns false.

[alcohol]

I think you misunderstood the point I am making. Since we do not normalize input, we should not normalize queries.

[nyamsprod]

mmmh so for you it is normal that

$collection->getAlpha2('fr');
$collection->getAlpha2('FR');

returns different result ? If the answer is no. Then at some point you need to normalize some data.

[alcohol]

I would consider that normal in some other languages :p Definitely would have my preference.

But since I was using strcasecmp before I suppose it would be a bit of a backwards compatibility break. I can just replace the === with strcasecmp again.