Skip AbstractProvider when generating provider list

[leafnode]

In some cases, when filesystem iterator returned AbstractProvider.php after USA.php, provider list generator returned AbstractProvider as the provider for USA, as abstract provider has the ID constant set to US (I'm unsure why it has this constant at all). The mechanism is simple: loop checks USA.php, adds 'US' => 'USA' to the array, then checks AbstractProvider, and overwrites the USA provider by adding 'US' => 'AbstractProvider' to the output.

To overcome this issue, I've expanded the condition checking if the class is a proper provider by checking if it's a subclass of AbstractProvider. That's why AbstractProvider won't be taken into consideration at all.

[stelgenhof]

Thank you for your PR. Can you maybe share your code or an example where this issue appears? I tried to replicate it myself with the following simple script, but couldn't see any issue:

// Require the composer auto loader
require 'vendor/autoload.php';

$providers = Yasumi::getProviders();
ksort($providers);
print_r($providers);

Output:

Array
(
    [AT] => Austria
    [AU] => Australia
    [AU-VIC] => Australia/Victoria
    [BA] => Bosnia
    [BE] => Belgium
    [BR] => Brazil
    [CH] => Switzerland
    [CH-AG] => Switzerland/Aargau
    [CH-AI] => Switzerland/AppenzellInnerrhoden
    [CH-AR] => Switzerland/AppenzellAusserrhoden
    [CH-BE] => Switzerland/Bern
    [CH-BL] => Switzerland/BaselLandschaft
    [CH-BS] => Switzerland/BaselStadt
    [CH-FR] => Switzerland/Fribourg
    [CH-GE] => Switzerland/Geneva
    [CH-GL] => Switzerland/Glarus
    [CH-GR] => Switzerland/Grisons
    [CH-JU] => Switzerland/Jura
    [CH-LU] => Switzerland/Lucerne
    [CH-NE] => Switzerland/Neuchatel
    [CH-NW] => Switzerland/Nidwalden
    [CH-OW] => Switzerland/Obwalden
    [CH-SG] => Switzerland/StGallen
    [CH-SH] => Switzerland/Schaffhausen
    [CH-SO] => Switzerland/Solothurn
    [CH-SZ] => Switzerland/Schwyz
    [CH-TG] => Switzerland/Thurgau
    [CH-TI] => Switzerland/Ticino
    [CH-UR] => Switzerland/Uri
    [CH-VD] => Switzerland/Vaud
    [CH-VS] => Switzerland/Valais
    [CH-ZG] => Switzerland/Zug
    [CH-ZH] => Switzerland/Zurich
    [CZ] => CzechRepublic
    [DE] => Germany
    [DE-BB] => Germany/Brandenburg
    [DE-BE] => Germany/Berlin
    [DE-BW] => Germany/BadenWurttemberg
    [DE-BY] => Germany/Bavaria
    [DE-HB] => Germany/Bremen
    [DE-HE] => Germany/Hesse
    [DE-HH] => Germany/Hamburg
    [DE-MV] => Germany/MecklenburgWesternPomerania
    [DE-NI] => Germany/LowerSaxony
    [DE-NW] => Germany/NorthRhineWestphalia
    [DE-RP] => Germany/RhinelandPalatinate
    [DE-SH] => Germany/SchleswigHolstein
    [DE-SL] => Germany/Saarland
    [DE-SN] => Germany/Saxony
    [DE-ST] => Germany/SaxonyAnhalt
    [DE-TH] => Germany/Thuringia
    [DK] => Denmark
    [EE] => Estonia
    [ES] => Spain
    [ES-AN] => Spain/Andalusia
    [ES-AR] => Spain/Aragon
    [ES-AS] => Spain/Asturias
    [ES-CB] => Spain/Cantabria
    [ES-CE] => Spain/Ceuta
    [ES-CL] => Spain/CastileAndLeon
    [ES-CM] => Spain/CastillaLaMancha
    [ES-CN] => Spain/CanaryIslands
    [ES-CT] => Spain/Catalonia
    [ES-EX] => Spain/Extremadura
    [ES-GA] => Spain/Galicia
    [ES-IB] => Spain/BalearicIslands
    [ES-MC] => Spain/RegionOfMurcia
    [ES-MD] => Spain/CommunityOfMadrid
    [ES-ML] => Spain/Melilla
    [ES-NC] => Spain/Navarre
    [ES-PV] => Spain/BasqueCountry
    [ES-RI] => Spain/LaRioja
    [ES-VC] => Spain/ValencianCommunity
    [FI] => Finland
    [FR] => France
    [FR-57] => France/Moselle
    [FR-67] => France/BasRhin
    [FR-68] => France/HautRhin
    [GB] => UnitedKingdom
    [GR] => Greece
    [HR] => Croatia
    [HU] => Hungary
    [IE] => Ireland
    [IT] => Italy
    [JP] => Japan
    [LT] => Lithuania
    [LV] => Latvia
    [NL] => Netherlands
    [NO] => Norway
    [NZ] => NewZealand
    [PL] => Poland
    [PT] => Portugal
    [RO] => Romania
    [RU] => Russia
    [SE] => Sweden
    [SK] => Slovakia
    [UA] => Ukraine
    [US] => USA
    [ZA] => SouthAfrica
)

[leafnode]

I was thinking about making a dedicated test for it, but it'd require quite a lot of refactoring. To replicate this bug, you'd have to replace filesystem iterators with a method call, and inject some mock here, that would return predefined file list.

But for the sake of example, I'd hardcode it into Yasumi. Let's hardcode normal file order into getProviders method:

//        $filesIterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator(
//            __DIR__ . $ds . 'Provider',
//            FilesystemIterator::SKIP_DOTS
//        ), RecursiveIteratorIterator::SELF_FIRST);

        $filesIterator = [
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/AbstractProvider.php'),
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/Belgium.php'),
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/USA.php'),
        ];

Now let's see what's inside the array that was returned:

        $providers = Yasumi::getProviders();
        var_dump($providers);

array(2) {
  ["US"]=>
  string(3) "USA"
  ["BE"]=>
  string(7) "Belgium"
}

Everything seems fine. But now let's change the order in which filesystem returned file list:

        $filesIterator = [
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/Belgium.php'),
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/USA.php'),
            new \SplFileInfo('/Users/leafnode/var/devel/yasumi/src/Yasumi/Provider/AbstractProvider.php'),
        ];

And now the output is:

array(2) {
  ["BE"]=>
  string(7) "Belgium"
  ["US"]=>
  string(16) "AbstractProvider"
}

Why? Because the pair 'US' => 'AbstractProvider' has overwritten the pair 'US' => 'USA'.

We could fix this issue in many different ways. One of them is to sort the input iterator by filename, but that's kind of a naive approach that relies on believing that we get the input in a specific order. My idea was to make sure that we load only actual providers basing on one thing we know: it has to extend AbstractProvider class, as well as contain ID constant. I was thinking about removing the constant from the AbstractProvider, but I'm not that accustomed with Yasumi's internals, and maybe there's some requirement for that constant to be there.

[stelgenhof]

Thanks for the example! Totally understand the issue now. So it looks like the FilesystemIterator may return an array of the files in an order that is not always guaranteed to be the same.

The ID constant acts merely as an identifier for the provider. I believe I used 'US' as a default for no particular reason :)

Let me have a look in a bit more detail tomorrow; however your PR looks like a good solution.

[stelgenhof]

I've checked this and looks to address the issue. Would you mind adding an entry to the CHANGELOG.md file as part of this PR?

Once added, I will merge it.

[leafnode]

Added the changelog entry.

[norberttech]

stelgenhof added this to the v2.0.0 milestone 8 hours ago

Why 2.0.0? It looks like something that should be fixed in earliest possible version. It does not break anything, I dont think anybody was relying on random behavior.

[stelgenhof]

@norzechowicz v2.0.0 will be the next official release. This PR will be merged in the development branch, so anyone can use that branch if they like :)

[norberttech]

merge into develop does not sounds like a solution, my project relies on that library (and probably many others as well), it's also having composer.json in pretty good shape (meaning we can update dependencies without risk of breaking things), using not stable branches is against our dependencies policy. I would really appreciate if you could at least release 1.8.1 with this fix.
I'm also not sure what changes you are planning in version 2.0.0 but I can't allow to jump from 1.x to 2.x just like that, our dependencies are set to ^1.8, I hope you understand my concerns.
If you would need any help with what I'm asking for, just let me know what I can do.

[stelgenhof]

Sure, understand your concern. Let me see if I can create a 1.8 patched version. :)

Just FYI: v2.0.0 will be a new branch supporting PHP 7 only going forward. v1.8 will be the last version based on PHP 5 and will not include any new features. Not sure yet when I'm going to release v2.0.0 yet (kind of occupied with other projects).

[norberttech]

hey @stelgenhof any chance to release 1.8.1 anytime soon?

[stelgenhof]

@norzechowicz No. Do you experience any critical issues with the current release?

[norberttech]

Yes, random behavior that can't be predicted fixed by this particular PR

[stelgenhof]

@norzechowicz Perhaps I am not seeing the entire picture here, but could you describe your situation in more detail? That may give me more context. Are there also other unpredictable results besides the one described in this PR (USA example)?

Thanks!