fix(upgrade): always execute module installation on upgrade (#271)

* fix(upgrade): always execute module installation on upgrade * test: change test that depends on removed upgrade file * build: remove upgrade folder from build process stuff * fix(database): handle errors in db migrations better * fix: fix errors on upgrade * test: fix test * fix: clean up entry file and remove created upgrade file after upgrading
myparcelnl · Nov 13, 2024 · 78b3657 · 78b3657
1 parent 165c91b
commit 78b3657
Show file tree

Hide file tree

Showing 22 changed files with 419 additions and 137 deletions.
diff --git a/.github/workflows/build-custom.yml b/.github/workflows/build-custom.yml
@@ -30,6 +30,5 @@ jobs:
             src/**/*
             config/**/*
             controllers/**/*
-            upgrade/**/*
             myparcelnl.php
             scoper.inc.php
diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml
@@ -38,6 +38,5 @@ jobs:
             src/**/*
             config/**/*
             controllers/**/*
-            upgrade/**/*
             myparcelnl.php
             scoper.inc.php
diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml
@@ -31,7 +31,6 @@ jobs:
             src/**/*
             config/**/*
             controllers/**/*
-            upgrade/**/*
             myparcelnl.php
             scoper.inc.php
 

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -31,7 +31,6 @@ jobs:
             src/**/*
             config/**/*
             controllers/**/*
-            upgrade/**/*
             myparcelnl.php
             scoper.inc.php
 

diff --git a/.gitignore b/.gitignore
@@ -19,6 +19,8 @@ junit/
 lib/
 node_modules/
 vendor/
+# Upgrade files are written dynamically. See MyParcelNL::loadUpgradeVersionList()
+upgrade/
 
 views/**/tsconfig.json
 

diff --git a/myparcelnl.php b/myparcelnl.php
@@ -8,6 +8,7 @@
 use MyParcelNL\Pdk\Facade\Logger;
 use MyParcelNL\Pdk\Facade\Pdk;
 use MyParcelNL\PrestaShop\Facade\MyParcelModule;
+use MyParcelNL\PrestaShop\Hooks\HasModuleUpgradeOverrides;
 use MyParcelNL\PrestaShop\Hooks\HasPdkCheckoutDeliveryOptionsHooks;
 use MyParcelNL\PrestaShop\Hooks\HasPdkCheckoutHooks;
 use MyParcelNL\PrestaShop\Hooks\HasPdkOrderGridHooks;
@@ -28,6 +29,11 @@
  */
 class MyParcelNL extends CarrierModule
 {
+    use HasModuleUpgradeOverrides;
+
+    /**
+     * Module hooks
+     */
     use HasPdkCheckoutDeliveryOptionsHooks;
     use HasPdkCheckoutHooks;
     use HasPdkOrderGridHooks;
@@ -38,22 +44,18 @@ class MyParcelNL extends CarrierModule
     use HasPsCarrierHooks;
     use HasPsShippingCostHooks;
 
-    /**
-     * @var bool
-     */
-    private $hasPdk;
+    private static ?string $versionFromComposer = null;
+
+    private bool           $hasPdk              = false;
 
-    /**
-     * @throws \Throwable
-     */
     public function __construct()
     {
         // Suppress deprecation warning from Pdk HasAttributes
         // todo: find a better solution
         error_reporting(error_reporting() & ~E_DEPRECATED);
 
         $this->name                   = 'myparcelnl';
-        $this->version                = $this->getVersionFromComposer();
+        $this->version                = self::getVersionFromComposer();
         $this->author                 = 'MyParcel';
         $this->author_uri             = 'https://myparcel.nl';
         $this->need_instance          = 1;
@@ -80,6 +82,22 @@ function () {
         );
     }
 
+    /**
+     * @return string
+     */
+    protected static function getVersionFromComposer(): string
+    {
+        if (! self::$versionFromComposer) {
+            $filename = __DIR__ . '/composer.json';
+            /** @noinspection JsonEncodingApiUsageInspection */
+            $composerData = json_decode(file_get_contents($filename), true);
+
+            self::$versionFromComposer = $composerData['version'];
+        }
+
+        return self::$versionFromComposer;
+    }
+
     /**
      * @param  bool $forceAll
      *
@@ -139,17 +157,6 @@ public function uninstall(): bool
             && parent::uninstall();
     }
 
-    /**
-     * @return string
-     */
-    protected function getVersionFromComposer(): string
-    {
-        $filename     = __DIR__ . '/composer.json';
-        $composerData = json_decode(file_get_contents($filename), true);
-
-        return $composerData['version'];
-    }
-
     /**
      * @return string
      */

diff --git a/scoper.inc.php b/scoper.inc.php
@@ -21,7 +21,7 @@
             ]),
         Finder::create()
             ->files()
-            ->in(['src', 'config', 'controllers', 'upgrade']),
+            ->in(['src', 'config', 'controllers']),
     ],
 
     'exclude-namespaces' => [

diff --git a/src/Database/AbstractDatabaseMigration.php b/src/Database/AbstractDatabaseMigration.php
@@ -8,6 +8,10 @@
 use MyParcelNL\Pdk\App\Installer\Contract\MigrationInterface;
 use MyParcelNL\Pdk\Facade\Logger;
 use MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface;
+use MyParcelNL\PrestaShop\Database\Sql\CreateIndexSqlBuilder;
+use MyParcelNL\PrestaShop\Database\Sql\CreateTableSqlBuilder;
+use MyParcelNL\PrestaShop\Database\Sql\DropTableSqlBuilder;
+use Throwable;
 
 abstract class AbstractDatabaseMigration implements MigrationInterface
 {
@@ -17,19 +21,123 @@ public function getVersion(): string
     }
 
     /**
-     * @param  string|\MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface $sql
+     * @param  string                                                              $table
+     * @param  callable<\MyParcelNL\PrestaShop\Database\Sql\CreateIndexSqlBuilder> $callable
+     *
+     * @return mixed
+     */
+    protected function createIndex(string $table, callable $callable)
+    {
+        $builder = $this->wrapBuilder($table, CreateIndexSqlBuilder::class, $callable);
+
+        try {
+            return $this->execute($builder);
+        } catch (Throwable $e) {
+            Logger::warning(
+                'Failed to create index.',
+                array_replace(['class' => static::class, 'table' => $table], ['error' => $e->getMessage()])
+            );
+
+            return false;
+        }
+    }
+
+    /**
+     * @param  string                                                              $table
+     * @param  callable<\MyParcelNL\PrestaShop\Database\Sql\CreateTableSqlBuilder> $callable
+     *
+     * @return mixed
+     */
+    protected function createTable(string $table, callable $callable)
+    {
+        $builder = $this->wrapBuilder($table, CreateTableSqlBuilder::class, $callable);
+
+        return $this->executeWithErrorHandling($builder);
+    }
+
+    /**
+     * @param  string $table
      *
      * @return void
      */
-    protected function execute($sql): void
+    protected function dropTable(string $table): void
+    {
+        $this->executeWithErrorHandling(new DropTableSqlBuilder($table));
+    }
+
+    /**
+     * @param  string|\MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface $sql
+     *
+     * @return mixed
+     * @throws \PrestaShopDatabaseException
+     * @throws \PrestaShopException
+     */
+    protected function execute($sql, bool $getResults = false)
+    {
+        $resolvedSql = $this->resolveSql($sql);
+        $instance    = Db::getInstance(_PS_USE_SQL_SLAVE_);
+
+        if ($getResults) {
+            $result = $instance->executeS($resolvedSql);
+        } else {
+            $result = $instance->execute($resolvedSql);
+        }
+
+        Logger::debug('Query executed', ['class' => static::class, 'sql' => $sql]);
+
+        return $result;
+    }
+
+    /**
+     * @param  string|\MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface $sql
+     * @param  bool                                                                    $getResults
+     *
+     * @return mixed
+     */
+    protected function executeWithErrorHandling($sql, bool $getResults = false)
+    {
+        $resolvedSql = $this->resolveSql($sql);
+
+        try {
+            return $this->execute($resolvedSql, $getResults);
+        } catch (Throwable $e) {
+            Logger::error(
+                'Failed to execute query.',
+                array_replace(['class' => static::class, 'sql' => $resolvedSql], ['error' => $e->getMessage()])
+            );
+        }
+
+        return false;
+    }
+
+    /**
+     * @param  string|\MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface $sql
+     *
+     * @return string
+     */
+    private function resolveSql($sql): string
     {
         if ($sql instanceof SqlBuilderInterface) {
             $sql = $sql->build();
         }
 
-        Db::getInstance(_PS_USE_SQL_SLAVE_)
-            ->execute($sql);
+        return $sql;
+    }
 
-        Logger::debug('Query executed', ['class' => static::class, 'sql' => $sql]);
+    /**
+     * @template T of \MyParcelNL\PrestaShop\Database\Sql\Contract\SqlBuilderInterface
+     * @param  string          $table
+     * @param  class-string<T> $builderClass
+     * @param  callable<T>     $callable
+     *
+     * @return T
+     */
+    private function wrapBuilder(string $table, string $builderClass, callable $callable): SqlBuilderInterface
+    {
+        $builder = new $builderClass($table);
+
+        $callable($builder);
+
+        return $builder;
     }
 }
diff --git a/src/Database/CreateAuditTableDatabaseMigration.php b/src/Database/CreateAuditTableDatabaseMigration.php
@@ -4,12 +4,9 @@
 
 namespace MyParcelNL\PrestaShop\Database;
 
-use MyParcelNL\Pdk\Facade\Logger;
 use MyParcelNL\PrestaShop\Database\Sql\CreateIndexSqlBuilder;
 use MyParcelNL\PrestaShop\Database\Sql\CreateTableSqlBuilder;
-use MyParcelNL\PrestaShop\Database\Sql\DropTableSqlBuilder;
 use MyParcelNL\PrestaShop\Entity\MyparcelnlAudit;
-use Throwable;
 
 /**
  * @see \MyParcelNL\PrestaShop\Entity\MyparcelnlAudit
@@ -18,30 +15,25 @@ final class CreateAuditTableDatabaseMigration extends AbstractDatabaseMigration
 {
     public function down(): void
     {
-        $this->execute(new DropTableSqlBuilder($this->getTable()));
+        $this->dropTable($this->getTable());
     }
 
     public function up(): void
     {
-        $sql = (new CreateTableSqlBuilder($this->getTable()))
-            ->column('id', 'VARCHAR(36) NOT NULL')
-            ->column('action', 'VARCHAR(255) NOT NULL')
-            ->column('data')
-            ->column('model', 'VARCHAR(255) NOT NULL')
-            ->column('model_identifier', 'VARCHAR(255) NOT NULL')
-            ->primary(['id'])
-            ->createdTimestamps();
-
-        $this->execute($sql);
-
-        try {
-            $indexSql = (new CreateIndexSqlBuilder($this->getTable()))
-                ->index('model_identifier_idx', ['model', 'model_identifier']);
-
-            $this->execute($indexSql);
-        } catch (Throwable $e) {
-            Logger::error('Could not create index.', ['error' => $e->getMessage(), 'class' => self::class]);
-        }
+        $this->createTable($this->getTable(), function (CreateTableSqlBuilder $builder) {
+            $builder->id('id');
+            $builder->column('action', 'VARCHAR(255) NOT NULL');
+            $builder->column('data');
+            $builder->column('model', 'VARCHAR(255) NOT NULL');
+            $builder->column('model_identifier', 'VARCHAR(255) NOT NULL');
+            $builder->timestamps();
+            $builder->primary(['id']);
+            $builder->createdTimestamps();
+        });
+
+        $this->createIndex($this->getTable(), function (CreateIndexSqlBuilder $builder) {
+            $builder->index('model_identifier_idx', ['model', 'model_identifier']);
+        });
     }
 
     /**

diff --git a/src/Database/CreateCarrierMappingTableDatabaseMigration.php b/src/Database/CreateCarrierMappingTableDatabaseMigration.php
@@ -5,7 +5,6 @@
 namespace MyParcelNL\PrestaShop\Database;
 
 use MyParcelNL\PrestaShop\Database\Sql\CreateTableSqlBuilder;
-use MyParcelNL\PrestaShop\Database\Sql\DropTableSqlBuilder;
 use MyParcelNL\PrestaShop\Entity\MyparcelnlCarrierMapping;
 
 /**
@@ -15,19 +14,18 @@ final class CreateCarrierMappingTableDatabaseMigration extends AbstractDatabaseM
 {
     public function down(): void
     {
-        $this->execute(new DropTableSqlBuilder($this->getTable()));
+        $this->dropTable($this->getTable());
     }
 
     public function up(): void
     {
-        $sql = (new CreateTableSqlBuilder($this->getTable()))
-            ->id('carrier_id')
-            ->column('myparcel_carrier', 'VARCHAR(32)')
-            ->timestamps()
-            ->primary(['carrier_id'])
-            ->unique(['myparcel_carrier']);
-
-        $this->execute($sql);
+        $this->createTable($this->getTable(), function (CreateTableSqlBuilder $builder) {
+            $builder->id('carrier_id');
+            $builder->column('myparcel_carrier', 'VARCHAR(32)');
+            $builder->timestamps();
+            $builder->primary(['carrier_id']);
+            $builder->unique(['myparcel_carrier']);
+        });
     }
 
     /**