Return exit code on unhandled exceptions, and allow process to go on when upgrading a module fails

classes/ErrorHandler.php

@@ -70,6 +70,7 @@ public function exceptionHandler(Throwable $e): void
     {
         $message = get_class($e) . ': ' . $e->getMessage();
         $this->report($e->getFile(), $e->getLine(), Logger::CRITICAL, $message, $e->getTraceAsString(), true);
+        $this->terminate(64);
     }
 
     /**
@@ -158,4 +159,12 @@ protected function report(string $file, int $line, int $type, string $message, s
             fclose($fd);
         }
     }
+
+    /**
+     * @return never
+     */
+    public function terminate(int $code)
+    {
+        exit($code);
+    }
 }

classes/Exceptions/UpgradeException.php

@@ -27,10 +27,9 @@
 
 namespace PrestaShop\Module\AutoUpgrade\Exceptions;
 
-/**
- * Todo: Should we create a UpgradeWarning class instead of setting the severity here?
- */
-class UpgradeException extends \Exception
+use Exception;
+
+class UpgradeException extends Exception
 {
     const SEVERITY_ERROR = 1;
     const SEVERITY_WARNING = 2;
@@ -50,6 +49,13 @@ class UpgradeException extends \Exception
      */
     public function getQuickInfos(): array
     {
+        if ($this->getPrevious()) {
+            return array_merge(
+                [(string) $this->getPrevious()],
+                $this->quickInfos
+            );
+        }
+
         return $this->quickInfos;
     }
 

classes/Task/Upgrade/UpgradeModules.php

@@ -78,9 +78,9 @@ public function run(): int
             do {
                 $module_info = array_pop($listModules);
                 try {
-                    $this->logger->debug($this->translator->trans('Upgrading module %module%...', ['%module%' => $module_info['name']]));
+                    $this->logger->debug($this->translator->trans('Updating module %module%...', ['%module%' => $module_info['name']]));
                     $this->container->getModuleAdapter()->upgradeModule($module_info['id'], $module_info['name'], !empty($module_info['is_local']));
-                    $this->logger->debug($this->translator->trans('The files of module %s have been upgraded.', [$module_info['name']]));
+                    $this->logger->info($this->translator->trans('The module %s has been updated.', [$module_info['name']]));
                 } catch (UpgradeException $e) {
                     $this->handleException($e);
                     if ($e->getSeverity() === UpgradeException::SEVERITY_ERROR) {
@@ -173,9 +173,6 @@ public function warmUp(): int
 
     private function handleException(UpgradeException $e): void
     {
-        foreach ($e->getQuickInfos() as $log) {
-            $this->logger->debug($log);
-        }
         if ($e->getSeverity() === UpgradeException::SEVERITY_ERROR) {
             $this->next = 'error';
             $this->setErrorFlag();
@@ -184,5 +181,9 @@ private function handleException(UpgradeException $e): void
         if ($e->getSeverity() === UpgradeException::SEVERITY_WARNING) {
             $this->logger->warning($e->getMessage());
         }
+
+        foreach ($e->getQuickInfos() as $log) {
+            $this->logger->warning($log);
+        }
     }
 }

classes/UpgradeTools/ModuleAdapter.php

@@ -33,6 +33,7 @@
 use PrestaShop\PrestaShop\Adapter\Module\Repository\ModuleRepository;
 use Symfony\Component\Filesystem\Exception\IOException;
 use Symfony\Component\Filesystem\Filesystem;
+use Throwable;
 
 class ModuleAdapter
 {
@@ -221,12 +222,12 @@ public function upgradeModule(int $id, string $name, bool $isLocalModule = false
             // file_get_contents can return false if https is not supported (or warning)
             $content = Tools14::file_get_contents($addons_url, false, $context);
             if (empty($content) || substr($content, 5) == '<?xml') {
-                $msg = '<strong>' . $this->translator->trans('[ERROR] No response from Addons server.') . '</strong>';
+                $msg = $this->translator->trans('[ERROR] No response from Addons server.');
                 throw new UpgradeException($msg);
             }
 
             if (false === (bool) file_put_contents($zip_fullpath, $content)) {
-                $msg = '<strong>' . $this->translator->trans('[ERROR] Unable to write module %s\'s zip file in temporary directory.', [$name]) . '</strong>';
+                $msg = $this->translator->trans('[ERROR] Unable to write module %s\'s zip file in temporary directory.', [$name]);
                 throw new UpgradeException($msg);
             }
         }
@@ -236,15 +237,13 @@ public function upgradeModule(int $id, string $name, bool $isLocalModule = false
         }
         // unzip in modules/[mod name] old files will be conserved
         if (!$this->zipAction->extract($zip_fullpath, $this->modulesPath)) {
-            throw (new UpgradeException('<strong>' . $this->translator->trans('[WARNING] Error when trying to extract module %s.', [$name]) . '</strong>'))->setSeverity(UpgradeException::SEVERITY_WARNING);
+            throw (new UpgradeException($this->translator->trans('[WARNING] Error when trying to extract module %s.', [$name])))->setSeverity(UpgradeException::SEVERITY_WARNING);
         }
         if (file_exists($zip_fullpath)) {
             unlink($zip_fullpath);
         }
 
-        if (!$this->doUpgradeModule($name)) {
-            throw (new UpgradeException('<strong>' . $this->translator->trans('[WARNING] Error when trying to upgrade module %s.', [$name]) . '</strong>'))->setSeverity(UpgradeException::SEVERITY_WARNING)->setQuickInfos(\Module::getInstanceByName($name)->getErrors());
-        }
+        $this->doUpgradeModule($name);
     }
 
     private function getLocalModuleZip(string $name): ?string
@@ -259,24 +258,33 @@ private function getLocalModuleZip(string $name): ?string
         return null;
     }
 
-    private function doUpgradeModule(string $name): bool
+    private function doUpgradeModule(string $name): void
     {
         $version = \Db::getInstance()->getValue(
             'SELECT version FROM `' . _DB_PREFIX_ . 'module` WHERE name = "' . $name . '"'
         );
         $module = \Module::getInstanceByName($name);
-        if ($module instanceof \Module) {
-            $module->installed = !empty($version);
-            $module->database_version = $version ?: 0;
-
-            if (\Module::initUpgradeModule($module)) {
-                $module->runUpgradeModule();
-                \Module::upgradeModuleVersion($name, $module->version);
+        if (!($module instanceof \Module)) {
+            return;
+        }
+        $module->installed = !empty($version);
+        $module->database_version = $version ?: 0;
 
-                return !count($module->getErrors());
+        try {
+            if (!\Module::initUpgradeModule($module)) {
+                return;
             }
+
+            $module->runUpgradeModule();
+            \Module::upgradeModuleVersion($name, $module->version);
+        } catch (Throwable $t) {
+            throw (new UpgradeException($this->translator->trans('[WARNING] Error when trying to upgrade module %s.', [$name]), 0, $t))->setSeverity(UpgradeException::SEVERITY_WARNING);
         }
 
-        return true;
+        $errorsList = $module->getErrors();
+
+        if (count($errorsList)) {
+            throw (new UpgradeException($this->translator->trans('[WARNING] Error when trying to upgrade module %s.', [$name])))->setSeverity(UpgradeException::SEVERITY_WARNING)->setQuickInfos($errorsList);
+        }
     }
 }

cli-rollback.php

@@ -24,6 +24,9 @@
  * @copyright Since 2007 PrestaShop SA and Contributors
  * @license   https://opensource.org/licenses/AFL-3.0 Academic Free License 3.0 (AFL-3.0)
  */
+
+use PrestaShop\Module\AutoUpgrade\ErrorHandler;
+
 if (PHP_SAPI !== 'cli') {
     echo 'This script must be called from CLI';
     exit(1);
@@ -32,7 +35,9 @@
 require_once realpath(dirname(__FILE__) . '/../../modules/autoupgrade') . '/ajax-upgradetabconfig.php';
 $container = autoupgrade_init_container(dirname(__FILE__));
 
-$container->setLogger(new PrestaShop\Module\AutoUpgrade\Log\StreamedLogger());
+$logger = new PrestaShop\Module\AutoUpgrade\Log\StreamedLogger();
+$container->setLogger($logger);
+(new ErrorHandler($logger))->enable();
 $controller = new \PrestaShop\Module\AutoUpgrade\Task\Runner\AllRollbackTasks($container);
 $controller->setOptions(getopt('', ['backup::']));
 $controller->init();

cli-upgrade.php

@@ -24,6 +24,9 @@
  * @copyright Since 2007 PrestaShop SA and Contributors
  * @license   https://opensource.org/licenses/AFL-3.0 Academic Free License 3.0 (AFL-3.0)
  */
+
+use PrestaShop\Module\AutoUpgrade\ErrorHandler;
+
 if (PHP_SAPI !== 'cli') {
     echo 'This script must be called from CLI';
     exit(1);
@@ -40,6 +43,7 @@
 
 $logger = new PrestaShop\Module\AutoUpgrade\Log\StreamedLogger();
 $container->setLogger($logger);
+(new ErrorHandler($logger))->enable();
 $controller = new \PrestaShop\Module\AutoUpgrade\Task\Runner\AllUpgradeTasks($container);
 $controller->setOptions(getopt('', ['action::', 'channel::', 'data::']));
 $controller->init();
@@ -54,7 +58,7 @@
 
 if ($chain && strpos($logger->getLastInfo(), 'cli-upgrade.php') !== false) {
     $new_string = str_replace('INFO - $ ', '', $logger->getLastInfo());
-    system('php ' . $new_string . '  2>&1', $exitCode);
+    system('php ' . $new_string, $exitCode);
 }
 
 exit($exitCode);

tests/unit/ErrorHandlerTest.php

@@ -37,7 +37,10 @@ protected function setUp()
     {
         parent::setUp();
         $this->logger = new LegacyLogger();
-        $this->errorHandler = new ErrorHandler($this->logger);
+        $this->errorHandler = $this->getMockBuilder(ErrorHandler::class)
+            ->setConstructorArgs([$this->logger])
+            ->setMethods(['terminate'])
+            ->getMock();
     }
 
     public function testDefaultContentIsEmpty()