humhub / updater

Upgrade your HumHub installation to the latest version in a few clicks.
8 stars 12 forks source link

Issue with Moving Directories During Update Package Installation #49

Open Dheyvidj opened 2 months ago

Dheyvidj commented 2 months ago

We are experiencing an issue with the update package installation process in our application. Specifically, the problem occurs when trying to move directories using the rename() function, which results in an error. The error message is as follows:

yii\base\ErrorException: rename(): The first argument to copy() function cannot be a directory

Detailed Problem:

The error occurs when attempting to move directories such as vendor, humhub, and static to the backup directory and replacing these directories with new ones from the update package. The current code uses the rename() function for this task, which does not support directory copy operations.

Steps to Reproduce:

  1. Execute the install() function from the UpdatePackage class.
  2. The code attempts to move the vendor, humhub, and static directories to the backup directory and replace them with new directories.
  3. Observe the error that occurs.

Impact:

Proposed Solution:

Replace the use of the rename() function with a method that correctly copies and removes directories. It is suggested to use the copyDirectory() function to copy the directories and then remove the old directories using FileHelper::removeDirectory().

Current Code (for reference):

// Current code using rename()
if (is_dir($this->getNewFileDirectory() . DIRECTORY_SEPARATOR . 'vendor')) {
    rename(Yii::getAlias('@webroot/protected/vendor'), $this->getBackupPath() . DIRECTORY_SEPARATOR . 'vendor_' . time());
    rename($this->getNewFileDirectory() . DIRECTORY_SEPARATOR . 'vendor', Yii::getAlias('@webroot/protected/vendor'));
}

Suggested Code (with fix):

// Suggested code using copyDirectory() and FileHelper::removeDirectory()
if (is_dir($this->getNewFileDirectory() . DIRECTORY_SEPARATOR . 'vendor')) {
    copyDirectory(Yii::getAlias('@webroot/protected/vendor'), $this->getBackupPath() . DIRECTORY_SEPARATOR . 'vendor_' . time());
    FileHelper::removeDirectory(Yii::getAlias('@webroot/protected/vendor'), ['traverseSymlinks' => true]);
    copyDirectory($this->getNewFileDirectory() . DIRECTORY_SEPARATOR . 'vendor', Yii::getAlias('@webroot/protected/vendor'));
}

The fix will allow proper movement of directories during the update process.

luke- commented 2 months ago

@Dheyvidj It is interesting that "rename" causes a "copy" on your installation. Are the directories on your installation maybe located on different file systems?

Dheyvidj commented 2 months ago

@Dheyvidj It is interesting that "rename" causes a "copy" on your installation. Are the directories on your installation maybe located on different file systems?

My installation is in a Docker container that uses several volumes. I believe these volumes might be directly influencing the issue. However, I made some modifications to the code that worked for me, but I am not certain about its effectiveness in different installation environments.

I suspect that the use of Docker volumes might be causing rename() to fail and triggering an internal copy() operation.

luke- commented 2 months ago

Ok, thanks for the details. Actually, I would like to stay with a move operation, as it happens almost instantly compared to a copy operation.

Maybe we could add a FileHelper::moveDirectory() functions, which:

  1. Tries to move via mv system command (if available) - it's safe to move via several volumes
  2. Tries to use PHP rename()
  3. Fallback to Copy
dreua commented 2 months ago

Sounds like a bug in yii PHP to me, unless it is clearly documented that it can't be used with directories across volumes. (At least the error message should be better.)

  1. Tries to move via mv system command (if available) - it's save to move via several volumes

Note that mv can't move between filesystems either, it will, however, fall back to copy and delete without errors in most cases. If you rely on an atomic move or persistent file pointers that may not work. (I'd need to check the documentation for details.)

dreua commented 2 months ago

Found the bug reported in 2011: PHP :: Bug #54097 :: rename() of dirs accross devices produces confusing copy error

Is this language actually maintained any more? :thinking: Loads of people must have wasted their time with this being unfixed and undocumented. Be it on Windows, with tmp file systems and especially since containerization started.