Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

composer update with plugin-installer v0.3.6 installed can cause plugin config files to be deleted #53

Closed
gurnec opened this issue Apr 23, 2024 · 2 comments · Fixed by #54

Comments

@gurnec
Copy link

gurnec commented Apr 23, 2024

I haven't had much time to try to debug this, so some of what's below might be wrong/incomplete, but I wanted to get this issue opened ASAP given its nature... "can cause deletion" probably means "will cause" for those who use composer to keep plugins up to date.

To reproduce, I believe the steps are:

  1. Ensure plugin-installer v0.3.5 or earlier is installed: composer require --update-no-dev roundcube/plugin-installer:0.3.5.
  2. Ensure that other packages are up to date (just to avoid confusion): composer update --no-dev.
  3. Require/install a plugin which uses a config.inc.php file, for example: composer require --update-no-dev radialapps/banner-ics.
  4. Modify its config.inc.php file.
  5. Switch back to the latest version of plugin-installer, but don't install it yet: composer require --no-install roundcube/plugin-installer.
  6. Note that the current state is one which will be common for those who use composer to keep packages up to date, but haven't yet updated to the latest plugin-installer.
  7. Update plugin-installer: composer update --no-dev.
  8. Run composer update --no-dev again. Nothing should happen, since an update was just completed above. Instead all plugin(s) installed with plugin-installer v0.3.5 or earlier are reinstalled from scratch:
    1. Something checks for the existence of vendor/radialapps/banner-ics which does not exist.
    2. plugins/banner-ics is recursively deleted (including any modified / config files).
    3. plugins/banner-ics is created and populated. vendor/radialapps/banner-ics is also created (empty).
    4. Any on-first-install tasks are executed (Do you want to activate the plugin?, creating a new config.inc.php file, SQL, etc.).
  9. Note that the plugin config.inc.php file(s) have been deleted and replaced by initial configs.

Future runs of composer update --no-dev do not reinstall the plugin(s) apparently because vendor/authorname/pluginname now exists.

I'm guessing this is caused by #51, but I'm not at all sure.

@gurnec
Copy link
Author

gurnec commented Apr 24, 2024

From what I can tell, what I wrote above is more-or-less correct.

When composer update starts, it updates its concept of currently-installed packages here which eventually calls ExtensionInstaller::getInstallPath() from LibraryInstaller::isInstalled() here.

getInstallPath() returns the vendor/authorname/pluginname directory since setRoundcubemailInstallPath() hasn't yet been called, and isInstalled() returns false if that directory doesn't exist, which will be the case if the plugin was installed with a version of plugin-installer prior to v0.3.6.

This later causes composer to call ExtensionInstaller::install() (instead of either nothing or ExtensionInstaller::update()) which doesn't preserve config files.

I'm not sure what the right fix is, since I don't really understand what #51 is improving (sending a mention to @mvorisek). Update ExtensionInstaller::getInstallPath()? Override isInstalled() in ExtensionInstaller or PluginInstaller?

@mvorisek
Copy link
Contributor

#51 fixes real install path detection as described in the PR description. Previously, the path was assumed, but real/installed could be different.

PR with fix is highly welcomed. Feel free to also update the CI test to prevent this issue to happen again.

Optimal fix should not require https://github.com/mvorisek/plugin-installer/blob/d66380fb175928a88982a7e440531c5a647c6ca1/src/ExtensionInstaller.php#L53 but I am not sure if that is easily possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants