Merge pull request #961 from ga-devfront/fix/module_migration_methods

Fix: module migration method on fly
PrestaShop · Oct 21, 2024 · 02aa4b3 · 02aa4b3
2 parents 73bb700 + 2df6966
commit 02aa4b3
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 36 deletions.
diff --git a/classes/Task/Update/UpdateModules.php b/classes/Task/Update/UpdateModules.php
@@ -69,7 +69,7 @@ public function run(): int
         $moduleSourceList = new ModuleSourceAggregate($this->container->getModuleSourceProviders());
         $moduleDownloader = new ModuleDownloader($this->translator, $this->logger, $this->container->getProperty(UpgradeContainer::TMP_PATH));
         $moduleUnzipper = new ModuleUnzipper($this->translator, $this->container->getZipAction(), $modulesPath);
-        $moduleMigration = new ModuleMigration($this->translator, $this->logger);
+        $moduleMigration = new ModuleMigration($this->translator, $this->logger, $this->container->getProperty(UpgradeContainer::TMP_PATH));
 
         if ($listModules->getRemainingTotal()) {
             $moduleInfos = $listModules->getNext();

diff --git a/classes/UpgradeTools/Module/ModuleMigration.php b/classes/UpgradeTools/Module/ModuleMigration.php
@@ -41,10 +41,14 @@ class ModuleMigration
     /** @var Logger */
     private $logger;
 
-    public function __construct(Translator $translator, Logger $logger)
+    /** @var string */
+    private $sandboxFolder;
+
+    public function __construct(Translator $translator, Logger $logger, string $sandboxFolder)
     {
         $this->translator = $translator;
         $this->logger = $logger;
+        $this->sandboxFolder = $sandboxFolder;
     }
 
     public function needMigration(ModuleMigrationContext $moduleMigrationContext): bool
@@ -105,23 +109,8 @@ public function runMigration(ModuleMigrationContext $moduleMigrationContext): vo
 
             $methodName = $this->getUpgradeMethodName($migrationFilePath);
 
-            // check if function already exists to prevent upgrade crash
-            if (function_exists($methodName)) {
-                $moduleMigrationContext->getModuleInstance()->disable();
-                throw (new UpgradeException($this->translator->trans('[WARNING] Method %s already exists. Migration for module %s aborted, you can try again later on the module manager. Module %s disabled.', [$methodName, $moduleMigrationContext->getModuleName(), $moduleMigrationContext->getModuleName()])))->setSeverity(UpgradeException::SEVERITY_WARNING);
-            }
-
-            include $migrationFilePath;
-
-            // @phpstan-ignore booleanNot.alwaysTrue (we ignore this error because we load a file with methods)
-            if (!function_exists($methodName)) {
-                $moduleMigrationContext->getModuleInstance()->disable();
-                throw (new UpgradeException($this->translator->trans('[WARNING] Method %s does not exist. Module %s disabled.', [$methodName, $moduleMigrationContext->getModuleName()])))->setSeverity(UpgradeException::SEVERITY_WARNING);
-            }
-
-            // @phpstan-ignore deadCode.unreachable (we ignore this error because the previous if can be true or false)
             try {
-                if (!$methodName($moduleMigrationContext->getModuleInstance())) {
+                if (!$this->loadAndCallFunction($migrationFilePath, $methodName, $moduleMigrationContext)) {
                     throw (new UpgradeException($this->translator->trans('[WARNING] Migration failed while running the file %s. Module %s disabled.', [basename($migrationFilePath), $moduleMigrationContext->getModuleName()])))->setSeverity(UpgradeException::SEVERITY_WARNING);
                 }
             } catch (UpgradeException $e) {
@@ -154,4 +143,28 @@ private function getUpgradeMethodName(string $filePath): string
 
         return 'upgrade_module_' . $version;
     }
+
+    /**
+     * Loads the migration file and calls the specified method using eval.
+     *
+     * @throws UpgradeException
+     */
+    private function loadAndCallFunction(string $filePath, string $methodName, ModuleMigrationContext $moduleMigrationContext): bool
+    {
+        $uniqueMethodName = $moduleMigrationContext->getModuleName() . '_' . $methodName;
+
+        $sandboxedFilePath = $this->sandboxFolder . DIRECTORY_SEPARATOR . $uniqueMethodName . '.php';
+        $pushedFileContents = file_put_contents($sandboxedFilePath, str_replace($methodName, $uniqueMethodName, file_get_contents($filePath)));
+
+        if ($pushedFileContents === false) {
+            throw (new UpgradeException($this->translator->trans('[WARNING] Could not write temporary file %s.', [$sandboxedFilePath])))->setSeverity(UpgradeException::SEVERITY_WARNING);
+        }
+
+        require_once $sandboxedFilePath;
+        if (!function_exists($uniqueMethodName)) {
+            throw (new UpgradeException($this->translator->trans('[WARNING] Method %s does not exist. Module %s disabled.', [$uniqueMethodName, $moduleMigrationContext->getModuleName()])))->setSeverity(UpgradeException::SEVERITY_WARNING);
+        }
+
+        return call_user_func($uniqueMethodName, $moduleMigrationContext->getModuleInstance());
+    }
 }
diff --git a/tests/unit/UpgradeTools/Module/ModuleMigrationTest.php b/tests/unit/UpgradeTools/Module/ModuleMigrationTest.php
@@ -42,6 +42,14 @@ class ModuleMigrationTest extends TestCase
      */
     private $moduleMigration;
 
+    private static $fixtureFolder;
+
+    public static function setUpBeforeClass()
+    {
+        self::$fixtureFolder = sys_get_temp_dir() . '/fakeMigrationFilesDestination';
+        @mkdir(self::$fixtureFolder);
+    }
+
     protected function setUp(): void
     {
         if (!defined('_PS_MODULE_DIR_')) {
@@ -58,7 +66,7 @@ protected function setUp(): void
             });
 
         $this->logger = $this->createMock(Logger::class);
-        $this->moduleMigration = new ModuleMigration($translator, $this->logger);
+        $this->moduleMigration = new ModuleMigration($translator, $this->logger, self::$fixtureFolder);
     }
 
     public function testNeedMigrationWithSameVersion()
@@ -179,22 +187,6 @@ public function testRunMigrationWithXYZDifferentFiles()
         $this->moduleMigration->runMigration($moduleMigrationContext);
     }
 
-    public function testRunMigrationWithSameInstanceThrowDuplicateMethod()
-    {
-        $mymodule = new \fixtures\mymodule\mymodule();
-        $mymodule->version = '1.1.1';
-        $dbVersion = '0.0.9';
-
-        $moduleMigrationContext = new ModuleMigrationContext($mymodule, $dbVersion);
-
-        $this->moduleMigration->needMigration($moduleMigrationContext);
-
-        $this->expectException(\PrestaShop\Module\AutoUpgrade\Exceptions\UpgradeException::class);
-        $this->expectExceptionMessage('[WARNING] Method upgrade_module_1 already exists. Migration for module mymodule aborted, you can try again later on the module manager. Module mymodule disabled.');
-
-        $this->moduleMigration->runMigration($moduleMigrationContext);
-    }
-
     public function testRunMigrationWithBadUpgradeMethodName()
     {
         $mymodule = new \fixtures\mymodule\mymodule();
@@ -206,7 +198,7 @@ public function testRunMigrationWithBadUpgradeMethodName()
         $this->moduleMigration->needMigration($moduleMigrationContext);
 
         $this->expectException(\PrestaShop\Module\AutoUpgrade\Exceptions\UpgradeException::class);
-        $this->expectExceptionMessage('[WARNING] Method upgrade_module_1_2_0 does not exist. Module mymodule disabled.');
+        $this->expectExceptionMessage('[WARNING] Method mymodule_upgrade_module_1_2_0 does not exist. Module mymodule disabled.');
 
         $this->moduleMigration->runMigration($moduleMigrationContext);
     }