Issue #41 - Fix skins installing into the wrong directory #42

buttercookie42 · 2023-12-01T19:35:02Z

The vendor directory into which extensions get installed is set once and subsequently cached in the ExtensionInstaller class, from which both the Plugin- and the SkinInstaller classes inherit.

This code used to work as intended, however PHP 8.1 changed the behaviour [1] and now all child classes share the static variable they inherited from their parent class. This means that when the plugin installer needs to install both regular plugins and skins within the same install session, the same path ends up being used both for skins and plugins, so one or the other ends up in the wrong directory.

The fix for this is based on the proposed workaround contained in the respective PHP RFC at [1].

[1] https://wiki.php.net/rfc/static_variable_inheritance

The vendor directory into which extensions get installed is set once and sub- sequently cached in the ExtensionInstaller class, from which both the Plugin- and the SkinInstaller classes inherit. This code used to work as intended, however PHP 8.1 changed the behaviour [1] and now all child classes share the static variable they inherited from their parent class. This means that when the plugin installer needs to install both regular plugins *and* skins within the same install session, the same path ends up being used both for skins and plugins, so one or the other ends up in the wrong directory. The fix for this is based on the proposed workaround contained in the respective PHP RFC at [1]. [1] https://wiki.php.net/rfc/static_variable_inheritance

buttercookie42 · 2023-12-01T19:40:50Z

Thinking about it – what is the minimum PHP version that the plugin installer still needs to support? If it's < 7.4, I guess I should throw out the null coalescing assignment operator again…

(Edit: Just noticed that Roundcube 1.6 only requires 7.3, so I need to change that bit)

MatthiasLohr · 2023-12-14T08:44:19Z

Thanks for raising this merge request!

MatthiasLohr · 2023-12-14T08:45:47Z

@thomascube, could you please check and merge? Thanks!

MatthiasLohr · 2024-01-30T12:54:04Z

@alecpl, @ChristophWurst, could you maybe take a look here?

mvorisek · 2024-04-07T08:18:46Z

src/Roundcube/Composer/ExtensionInstaller.php

-        static $vendorDir;
-        if ($vendorDir === null) {
-            $vendorDir = $this->getVendorDir();
+        static $vendorDir = [];


Currently, $this->getVendorDir() returns getcwd().

I belive we should use composer API instead and not cache the result at all (or cache it using all the variables that were used to parametrize the API).

Currently, $this->getVendorDir() returns getcwd().

In this class. But it's being overridden by both the PluginInstaller and the SkinInstaller subclasses, which do the actual work of plugin installation.

There is getcwd() too which is fragile. It would be much better to query composer API where roundcube/roundcube package is. Such result will be guaranteed to be "script runtime stable" and IMO fast enough to not need any caching.

Based on https://github.com/phpstan/extension-installer/blob/1.3.1/src/Plugin.php#L137 it should be easy as:

$composer = $event->getComposer(); $installationManager = $composer->getInstallationManager(); return $installationManager->getInstallPath('roundcube/roundcubemail');

It's roundcube/roundcubemail and it does not exist if you didn't install Roundcube as a dependency. SO, that wouldn't work. Am I right?

Thanks to #44 this is always satisfied. Root-project (in sense of composer terminology, ie. installed as main project/not as dependency) can be analysed using composer API perfectly as well :).

@mvorisek Thanks for the tip with the composer API, I will take a look.

Will be fixed in #47. Can you please test/review?

alecpl · 2024-04-10T10:39:36Z

I believe this is fixed with #47. Closing.

MatthiasLohr · 2024-04-11T07:23:48Z

Looks like it is even more broken than before:

In ExtensionInstaller.php line 39:
                                                                               
  [TypeError]                                                                  
  Roundcube\Composer\ExtensionInstaller::getInstallPath(): Argument #1 ($pack  
  age) must be of type Composer\Package\PackageInterface, null given, called   
  in /usr/src/roundcubemail/vendor/roundcube/plugin-installer/src/Roundcube/C  
  omposer/ExtensionInstaller.php on line 33                                    
                                                                               

Exception trace:
  at /usr/src/roundcubemail/vendor/roundcube/plugin-installer/src/Roundcube/Composer/ExtensionInstaller.php:39
 Roundcube\Composer\ExtensionInstaller->getInstallPath() at /usr/src/roundcubemail/vendor/roundcube/plugin-installer/src/Roundcube/Composer/ExtensionInstaller.php:33
 Roundcube\Composer\ExtensionInstaller->getRoundcubemailInstallPath() at /usr/src/roundcubemail/vendor/roundcube/plugin-installer/src/Roundcube/Composer/PluginInstaller.php:19
 Roundcube\Composer\PluginInstaller->getVendorDir() at /usr/src/roundcubemail/vendor/roundcube/plugin-installer/src/Roundcube/Composer/ExtensionInstaller.php:45
 Roundcube\Composer\ExtensionInstaller->getInstallPath() at phar:///usr/bin/composer/src/Composer/Installer/LibraryInstaller.php:112
 Composer\Installer\LibraryInstaller->download() at phar:///usr/bin/composer/src/Composer/Installer/InstallationManager.php:277
 Composer\Installer\InstallationManager->downloadAndExecuteBatch() at phar:///usr/bin/composer/src/Composer/Installer/InstallationManager.php:218
 Composer\Installer\InstallationManager->execute() at phar:///usr/bin/composer/src/Composer/Installer.php:804
 Composer\Installer->doInstall() at phar:///usr/bin/composer/src/Composer/Installer.php:629
 Composer\Installer->doUpdate() at phar:///usr/bin/composer/src/Composer/Installer.php:290
 Composer\Installer->run() at phar:///usr/bin/composer/src/Composer/Command/RequireCommand.php:490
 Composer\Command\RequireCommand->doUpdate() at phar:///usr/bin/composer/src/Composer/Command/RequireCommand.php:339
 Composer\Command\RequireCommand->execute() at phar:///usr/bin/composer/vendor/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at phar:///usr/bin/composer/vendor/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at phar:///usr/bin/composer/vendor/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at phar:///usr/bin/composer/src/Composer/Console/Application.php:382
 Composer\Console\Application->doRun() at phar:///usr/bin/composer/vendor/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at phar:///usr/bin/composer/src/Composer/Console/Application.php:145
 Composer\Console\Application->run() at phar:///usr/bin/composer/bin/composer:93
 require() at /usr/bin/composer:29

alecpl · 2024-04-11T07:49:37Z

What are the steps to reproduce?
What Roundcube version?

alecpl · 2024-04-11T07:50:01Z

And what Composer version?

alecpl · 2024-04-11T07:54:57Z

Ok, I just reproduced it by calling composer require roundcube/larry in Roundcube git-master and composer 2.7.2.

@mvorisek could you check that?

MatthiasLohr · 2024-04-11T07:56:37Z

Also happens with other packages beside larry, I tested using the 1.6.6 docker images, so it's compose 2.6.6 here.

mvorisek · 2024-04-11T08:01:20Z

@alecpl I somehow tested #47 wrongly and it should be fixed by #48.

Also, in composer/composer#11927 I am checking if there is even a better solution, but for standard Roundcubemail installed as a root package, the #48 should be working.

MatthiasLohr · 2024-04-11T08:05:42Z

Seems to be working for me.

alecpl · 2024-04-11T08:06:03Z

Fixed in 0.3.5.

ps. if you have plugins/skins installed you might need to remove them from composer.json to be able to update the plugin-installer.

buttercookie42 force-pushed the skininstall branch from afb0db2 to 74f8836 Compare December 1, 2023 19:52

mvorisek reviewed Apr 7, 2024

View reviewed changes

This was referenced Apr 9, 2024

Add real test and CS/PHPStan to CI #46

Merged

Replace getcwd() with Roundcubemail path from composer API #47

Merged

alecpl closed this Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #41 - Fix skins installing into the wrong directory #42

Issue #41 - Fix skins installing into the wrong directory #42

buttercookie42 commented Dec 1, 2023

buttercookie42 commented Dec 1, 2023 •

edited

Loading

MatthiasLohr commented Dec 14, 2023

MatthiasLohr commented Dec 14, 2023

MatthiasLohr commented Jan 30, 2024

mvorisek Apr 7, 2024

buttercookie42 Apr 7, 2024

mvorisek Apr 7, 2024 •

edited

Loading

mvorisek Apr 7, 2024 •

edited

Loading

alecpl Apr 7, 2024

mvorisek Apr 7, 2024

buttercookie42 Apr 8, 2024

mvorisek Apr 9, 2024 •

edited

Loading

alecpl commented Apr 10, 2024

MatthiasLohr commented Apr 11, 2024

alecpl commented Apr 11, 2024

alecpl commented Apr 11, 2024

alecpl commented Apr 11, 2024

MatthiasLohr commented Apr 11, 2024

mvorisek commented Apr 11, 2024

MatthiasLohr commented Apr 11, 2024

alecpl commented Apr 11, 2024

Issue #41 - Fix skins installing into the wrong directory #42

Issue #41 - Fix skins installing into the wrong directory #42

Conversation

buttercookie42 commented Dec 1, 2023

buttercookie42 commented Dec 1, 2023 • edited Loading

MatthiasLohr commented Dec 14, 2023

MatthiasLohr commented Dec 14, 2023

MatthiasLohr commented Jan 30, 2024

mvorisek Apr 7, 2024

Choose a reason for hiding this comment

buttercookie42 Apr 7, 2024

Choose a reason for hiding this comment

mvorisek Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

mvorisek Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

alecpl Apr 7, 2024

Choose a reason for hiding this comment

mvorisek Apr 7, 2024

Choose a reason for hiding this comment

buttercookie42 Apr 8, 2024

Choose a reason for hiding this comment

mvorisek Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

alecpl commented Apr 10, 2024

MatthiasLohr commented Apr 11, 2024

alecpl commented Apr 11, 2024

alecpl commented Apr 11, 2024

alecpl commented Apr 11, 2024

MatthiasLohr commented Apr 11, 2024

mvorisek commented Apr 11, 2024

MatthiasLohr commented Apr 11, 2024

alecpl commented Apr 11, 2024

buttercookie42 commented Dec 1, 2023 •

edited

Loading

mvorisek Apr 7, 2024 •

edited

Loading

mvorisek Apr 7, 2024 •

edited

Loading

mvorisek Apr 9, 2024 •

edited

Loading