Extension conflict check ignores version

[donquixote]

Bug report

Question	Answer
Box version	main
PHP version	8.3
Platform with version	Ubuntu
Github Repo	..

I first noticed this issue with grumphp.
phpro/grumphp-shim#30

We have a box with ext-redis 6.3, the grumphp phar has a package (symfony/cache) which declares "conflict" with ext-redis < 6.1. This causes The package "symfony/cache" conflicts with the extension "redis". in the box requirements check, even though the package only has a problem with lower versions of redis.

I then looked into the code in this project in main branch.
The code that collects version conflicts only collects package names, but not the actual version constraints.

I am not sure yet how I would reproduce this with just box, but the code itself already looks wrong.
E.g. here in AppRequirementsFactory::collectComposerLockExtensionRequirements():

            foreach ($packageInfo->getConflictingExtensions() as $extension) {
                $conflicts[$extension][] = $packageInfo->getName();
            }

The $conflicts array has no place for the extension version constraint.

[theofidry]

Indeed, I plan to address this.

Related to #1243.

[FlorentTorregrosa]

Hi,

Also encountering this issue.

if there is anything I can do to help with the resolution, please tell me.

[theofidry]

Hi @FlorentTorregrosa,

It was mentioned in #1243 that the way the extensions version is retrieved may be tricky and is likely to be the hardest part, and actually not really Box specific.

I plan to dive into PHP-Scoper/Box in January (I have a few other packages to fix for PHP 8.5/Symfony 8 I need to fix first). If that part is figured out that would tremendously speed things up.

[FlorentTorregrosa]

Ok, regarding the issue summary AppRequirementsFactory::collectComposerLockExtensionRequirements(), when I cloned the repository, I obtain 3 collectComposerLockExtensionRequirements methods (2 in a fixture folder so wondering what it is for). So:

• which ones needs to be updated?
• detecting correctly installed version aside, is the current codebase ready to take it into account or should it also be updated?

Like:

foreach ($packageInfo->getConflictingExtensions() as $extension) {
    $conflicts[$extension][] = $packageInfo->getName();
}

To be changed into something like (I checked, there is no getVersion method on this object, just to discuss an idea of the goal), and it will be enough or more changes will be needed:

foreach ($packageInfo->getConflictingExtensions() as $extension) {
    $conflicts[$extension][$packageInfo->getVersion()] = $packageInfo->getName();
}

[theofidry]

I believe two of the fixtures ones may be for an old format. The method you describe otherwise does need to be updated: it's the "easy" part which is getting the constraint from the composer files.

The harder part is in the requirement checker: you know which version you want, but you need to check which one there is on the system.

I'll try to have a look once I get hold of my laptop as commenting from memory has its limits 😅

[theofidry]

So there is two places:

• AppRequirementsFactory which collects the requirements of the app. The main test to adapt is AppRequirementsFactoryTest.
• IsExtensionFulfilled is the problematic part: right now we only check that the extension is loaded, but not the version of the extension.

[auroraeosrose]

So it really shouldn't be terribly hard to see the existing version - after the extension_loaded check run

$version = phpversion($this->requiredExtension);

Then you'll have the loaded extension version to do whatever comparisons you need to verify the version

[theofidry]

I was under the impression it required more work than that from #1243 (comment), but it would be nice if it isn't

[auroraeosrose]

If an extension uses the proper macro in their header for version it is that easy - you'll see that composer even uses that basic check in the code to get their version :)

https://github.com/composer/composer/blob/main/src/Composer/Platform/Runtime.php#L84

However the value you get back from phpversion('extname') may not be what you think

Core extensions (extensions shipped with php - whether built in or as a an .so/.dll) will always return the current php version string when called (this didn't use to be the case but the cleanup of PHP source code is an ongoing adventure - not sure how far back you're supporting but if it's post 7.5 you should be ok)

All that extra code you're seeing is adding additional "libraries" for composer checks - which is yes distressingly per extension and fiddly

https://github.com/composer/composer/blob/main/src/Composer/Repository/PlatformRepository.php#L205

for example pgsql gets lib-pdo_pgsql-libpq added as "library" to version on
lib-zip-libzip gets added as version as well

This is not a well documented part of the platform support from composer :)

a pity that composer and php extensions haven't hashed out a better way to grab linked library versions than parsing phpinfo
And that the code in composer doesn't have lib parsing separated out better

I'm not sure how close to composer checking you want for box, but at the very least the ext version checking should be fine
the library checking might be a bigger ball of fish to fry

Really.... PHP should have a get_extension_libraries .... that sounds like an RFC adventure

[Seldaek]

Composer does support runtime checks too but for perf reasons we also do not check versions, only ext presence https://getcomposer.org/doc/07-runtime.md#platform-check

And tbh because most exts people rely on (I can hardly imagine a phar requiring a non-bundled extension) just follow the php version, checking that is usually enough.

Sure it would be convenient if lib versions were accessible nicely through reflection i guess but I also don't really think it's worth the effort (both in terms of engineering and politics).

[veewee]

Since the composer autoloader does the platform checks nowadays, would it make sense to drop the requirement checks from this project altogether @theofidry ?

[theofidry]

I will double check if that is sufficient.

[acrobat]

Is there anyway we can help move this forward? I also encountered this problem lately