Verify size of installed packages array

[flancer64]

self::getInstalled() can return -empty array- array with empty array. We should validate key existence to prevent exception like:

Exception: Notice: Undefined index: versions in /.../vendor/composer/composer/src/Composer/InstalledVersions.php on line 54 in /.../vendor/magento/framework/App/ErrorHandler.php:61

[stof]

This does not make sense. A foreach loop over an empty array.

and this Undefined index: versions error does not happen if self::getInstalled() returns an empty array (as that code would not execute at all if the loop runs 0 times). It would happen if it returns a non-empty array containing arrays without a versions key.

[flancer64]

Yes, you are right. My first fix was:

if (isset($installed['versions'])) 
    $packages[] = array_keys($installed['versions']);

but I had an error in PHPStan verification:

Offset 'versions' on array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: array, dev: bool}, versions: array<string, array{pretty_version?: string, version?: string, reference?: string|null, type?: string, install_path?: string, aliases?: array, dev_requirement: bool, replaced?: array, ...}>} in isset() always exists and is not nullable.

I don't know how to fix PHPStan rules :(

[stof]

Those @psalm-return annotations should probably be turned into @phpstan-return instead, so that phpstan actually reports failures when the code does not respect them. I'm quite sure it would have reported that bug in that case.
For projects consuming the class, both Psalm and phpstan will read the more detailled types based on annotations using their own prefix or the other prefix. But they won't fail the code analysis based on other tool annotations of your own class.

[Seldaek]

Well I am still not sure what this fixes. It seems what @stof said above is still in the PR, I don't see a problem returning an empty array here. Please explain the intent or let's close this.

[flancer64]

Well I am still not sure what this fixes. It seems what @stof said above is still in the PR, I don't see a problem returning an empty array here. Please explain the intent or let's close this.

Empty array will throw an exception (Undefined index: versions) in 'array_keys($installed['versions'])' in case of $installed is an empty array:

    public static function getInstalledPackages()
    {
        $packages = array();
        foreach (self::getInstalled() as $installed) {
            $packages[] = array_keys($installed['versions']);
        }

        if (1 === \count($packages)) {
            return $packages[0];
        }

        return array_keys(array_flip(\call_user_func_array('array_merge', $packages)));
    }

[Seldaek]

No? Empty array will never enter the foreach?

[flancer64]

Yes, you are right. I did not explain the problem correctly. It was too long time ago :)

In this place self::$installed can be an empty array, and the result will be an array with empty array

private static function getInstalled()
    ...
        if (null === self::$installed) {
            ...
            if (...) {
                ...
            } else {
                self::$installed = array();
            }
        }
        $installed[] = self::$installed;
    ...
}

If result of getInstalledPackages() will be an array with empty array, then an exception will be thrown in getInstalledPackages().

[Seldaek]

Sorry I just still don't get it, here is the code from getInstalledPackages:

        $packages = array();
        foreach (self::getInstalled() as $installed) {
            $packages[] = array_keys($installed['versions']);
        }

        if (1 === \count($packages)) {
            return $packages[0];
        }

        return array_keys(array_flip(\call_user_func_array('array_merge', $packages)));

Now if we replace getInstalled by an empty array we get:

        $packages = array();
        foreach (array() as $installed) {
            $packages[] = array_keys($installed['versions']);
        }

        if (1 === \count($packages)) {
            return $packages[0];
        }

        return array_keys(array_flip(\call_user_func_array('array_merge', $packages)));

So foreach of an empty array does nothing, so we can remove the foreach:

        $packages = array();
        if (1 === \count($packages)) {
            return $packages[0];
        }

        return array_keys(array_flip(\call_user_func_array('array_merge', $packages)));

Then we get an empty $packages, so the if here does not match for sure so we remove it:

        $packages = array();
        return array_keys(array_flip(\call_user_func_array('array_merge', $packages)));

And now we're left with this which evaluates fine: https://3v4l.org/ss0f9

So I don't understand where you get an exception.

[vtsykun]

Probably it's related with #9937 when uses composer/composer 1

if (null === self::$installed) {
    // only require the installed.php file if this file is loaded from its dumped location,
    // and not from its source location in the composer/composer package, see https://github.com/composer/composer/issues/9937
    if (substr(__DIR__, -8, 1) !== 'C') {
        self::$installed = require __DIR__ . '/installed.php';
    } else {
        self::$installed = array();
    }
}
$installed[] = self::$installed;

If the 1st condition is true and the 2nd condition = false

self::$installed = array();
$installed[] = self::$installed;

Return result is

$installed = [[]]

Now if we replace getInstalled we get:

foreach ([[]] as $installed) {
    $packages[] = array_keys($installed['versions']);
}

[flancer64]

Thanks, @vtsykun , exactly:

foreach ([[]] as $installed) {
    $packages[] = array_keys($installed['versions']);
}

[Seldaek]

Oh ok ok now I see it thanks :D I'll fix this