search

alcohol / iso3166

A PHP library providing ISO 3166-1 data.
https://iso3166.thephpleague.com
MIT License
640 stars 59 forks source link

Empty string search returns Afghanistan because it is a very first value in country list #103

Closed brzuchal closed 2 months ago

brzuchal commented 2 months ago

There is a bug in the lookup method https://github.com/thephpleague/iso3166/blob/main/src/ISO3166.php#L163-L165 Look at https://3v4l.org/evSRJ this shows that the second part of the if condition is true for empty string input, this is effectively always true causing the lookup to always return Afghanistan when searching by empty string. I haven't checked if this works for all fields, but I guess it could, because why not?!

alcohol commented 2 months ago

Aside from name(), the other public methods implement guards. Perhaps the same could be done for name(). Although I am not sure if this is really an issue to begin with. There is no contract or definition for what should be returned if you search for an empty string. Since name() functions somewhat like an autocomplete, returning the first result from the set for an empty search string could be considered a graceful way to "fail".

brzuchal commented 2 months ago

@alcohol so if I search for a string, should I also make sure that the country I end up with is actually the one I was looking for? I'm afraid that approach seems like a recipe for messing things up without really trying.

alcohol commented 2 months ago

I do not follow. If you search for a string through the name() method, you can expect a result in an autocomplete like manner. If you want to be sure of the returned value, I suggest working with alpha 2 or 3 codes instead. They always map 1:1 to their designated value.

brzuchal commented 2 months ago

@alcohol sorry but I don't follow, what autocomplete? This is not a JS frontend library we would use for autocompletion, instead we use it in integration systems to lookup for appropriate country details based on user input. Therefore you can't just state that this is not a bug rather a feature, this is at least inappropriate! IMO the return should be null in this case or at least throw an exception.

amodliszewski commented 2 months ago

@alcohol Following description of method.

@throws \League\ISO3166\Exception\OutOfBoundsException if key does not exist in dataset

I believe empty string is non-existing key, also any one letter key should then end up on that exception, as I'm quite sure it does not exist same as empty string. It should be strict behaviour.