composer / package-versions-deprecated

:package: Composer addon to efficiently get installed packages' version numbers
MIT License
1.51k stars 9 forks source link

OutOfBoundsException Package "vimeo/psalm" is not installed. Fallback to versions available in this package when Composer\InstalledVersions check fails. #27

Closed JParkinson1991 closed 3 years ago

JParkinson1991 commented 3 years ago

Hello,

Trying to install and use vimeo/psalm in a local project but keep getting OutOfBoundsException when trying to access it's binary.

Composer Version: 1.10.22 PHP Version: 7.3.28 | 7.4.20

Having dug into this a little further it seems to be the cause of the issue is in 'composer/composer'. (So apologies if this is not the right place)

Steps To Reproduce

$ composer require --dev composer/composer:^2.0
$ composer require --dev vimeo/psalm
$ ./vendor/bin/psalm
Uncaught OutOfBoundsException: Package "vimeo/psalm" is not installed in...

Working Example

$ composer require --dev composer/composer:^1.10
$ composer require --dev vimeo/psalm
$ ./vendor/bin/psalm

Is it the case that psalm is not compatible with composer/compose 2? Anything that we can do here?

JParkinson1991 commented 3 years ago

Looking into this, it seems the package version does exist as expected in \PackageVersions\Versions.

However it's call to the Composer\InstalledVersions class methods are where these exceptions are originating from.

Could we not catch these exceptions, and fallback to available versions within this package returning those where available or allowing the exception to bubble where they're not?

Something akin to..

final class Versions
{
   ...

    /**
     * @throws OutOfBoundsException If a version cannot be located.
     *
     * @psalm-param key-of<self::VERSIONS> $packageName
     * @psalm-pure
     *
     * @psalm-suppress ImpureMethodCall we know that {@see InstalledVersions} interaction does not
     *                                  cause any side effects here.
     */
    public static function getVersion(string $packageName): string
    {
        if (class_exists(InstalledVersions::class, false) && (method_exists(InstalledVersions::class, 'getAllRawData') ? InstalledVersions::getAllRawData() : InstalledVersions::getRawData())) {
            try {
                return InstalledVersions::getPrettyVersion($packageName)
                    . '@' . InstalledVersions::getReference($packageName);
            }
            catch (OutOfBoundsException $e) {
                if (isset(self::VERSIONS[$packageName])) {
                    return self::VERSIONS[$packageName];
                }

                throw $e;
            }
        }

        if (isset(self::VERSIONS[$packageName])) {
            return self::VERSIONS[$packageName];
        }

        throw new OutOfBoundsException(
            'Required package "' . $packageName . '" is not installed: check your ./vendor/composer/installed.json and/or ./composer.lock files'
        );
    }
}

This seems to fix the issue, but approach could probably be rewritten/reworked.

Seldaek commented 3 years ago

I cannot reproduce this with Composer 2. I'd definitely like to understand the exact problem before merging such a hacky-looking PR.

I think probably what happens is that you run Composer 1, but have Composer 2 installed, so PackageVersions\Versions sees Composer\InstalledVersions is there and it uses it, but it's actually not been dumped correctly because you installed with Composer 1.

IMO the solution here is to upgrade to Composer 2. Or possibly to avoid requiring composer/composer, I'm not sure why you have it in your vendor dir but it's usually a bad idea.

JParkinson1991 commented 3 years ago

This is very much an issue with Composer 1, the issue does not occur when running Composer 2.

I came across this issue when developing a plugin (see https://github.com/JParkinson1991/composer-linker-plugin/tree/feature/actions_fix) that was to be compatible with both Composer 1 and Composer 2. When developing the plugin it is useful to require (dev) composer/composer so that IDE's etc can type hint against internal composer classes.

The problem seems to be isolated to using Composer at version 1, but having composer/composer installed at version 2. This is very much an edge case but one that we should account for where we can, avoiding these exceptions being thrown.

Seldaek commented 3 years ago

Ok, sorry but I'd rather not add the catch there as I feel it makes the flow even harder to understand if something gets returned which is not what you expect. Plus this package should anyway long term not be used if you are using Composer 2.

JParkinson1991 commented 3 years ago

No problem at all, long term moving to composer 2 should always be the aim.

That being said, can you elaborate on the following:

I feel it makes the flow even harder to understand if something gets returned which is not what you expect

I'm confused as to what is being returned that the method caller wouldn't expect? Calling getVersion either returns the package version (prettyversion@reference) or throws an OutOfBoundsException, this PR does not change that. Or am i missing something?

Seldaek commented 3 years ago

What scares me a little is this only happens when two versions of composer run concurrently in the same process to some extent, which means possibly some things are out of sync and you may get data from a vendor dir you aren't expecting or I don't know what exactly. It seems like failing hard is the safer and better choice here.

mr-feek commented 3 years ago

Just wanted to chime in as I just ran into this as well --

I think probably what happens is that you run Composer 1, but have Composer 2 installed, so PackageVersions\Versions sees Composer\InstalledVersions is there and it uses it, but it's actually not been dumped correctly because you installed with Composer 1.

Can confirm that I have both versions of composer installed in my docker container (as I use the same image for various apps, some which have migrated to composer 2 already and some that have not)


Did some debugging

\Composer\InstalledVersions::getAllRawData();
=> [
     [],
   ]

However, the package is listed in Versions::versions array.

Perhaps we could expand this condition to check that the array isn't empty? Just a thought.

Seldaek commented 3 years ago

Ok I did fix this I believe in https://github.com/composer/package-versions-deprecated/commit/b8d9f630a0cfa8c708b8845c7265fa6ebecefe1e - if one of you can please try it out using composer/package-versions-deprecated dev-master, I'd rather know for sure it works for you before I make a release.

JParkinson1991 commented 3 years ago

@Seldaek just tested this all seems to be working as expected.

As previously mentioned it seemed issues was isolated to a composer 1 binary install and a package/library composer/composer install of > 2. Using the below i am unable to reproduce the original errors.

composer.json

{
    "name": "composer/package-versions-deprecated-dev-master-test",
    "type": "library",
    "require": {
        "composer/package-versions-deprecated": "dev-master",
        "composer/composer": "^2.0",
        "vimeo/psalm": "^4.8"
    }
}

CLI Output (post library install):

$ composer --version
      Composer version 1.10.22 2021-04-27 13:10:45
$ ./vendor/bin/psalm
      Could not locate a config XML file in path /Users/joshuaparkinson/Sites/test/. Have you run 'psalm --init' ?
$ composer self-update --2
      Updating to version 2.1.6 (2.x channel).
$ composer --version
      Composer version 2.1.6 2021-08-19 17:11:08
$ ./vendor/bin/psalm
      Could not locate a config XML file in path /Users/joshuaparkinson/Sites/test/. Have you run 'psalm --init' ?
Seldaek commented 3 years ago

OK thanks for the confirmation, released as 1.11.99.3