From 759356288b1fbea262ed197ea91489c90e9f32c3 Mon Sep 17 00:00:00 2001 From: Joe Williams <121203672+JustASideQuestNPC@users.noreply.github.com> Date: Wed, 15 Oct 2025 22:53:14 -0700 Subject: [PATCH] Add transactions to missing config keys migration. (#4318) * `execute_script()` now returns a boolean for error handling. * Added transaction to `Migration_MissingConfigKeys.up()`. * Added `executeScriptWithTransaction()` to migration helpers. * Many changes for testing; also minor formatting fixes. * Removed test code and pointed the `NullableTaxCategoryId` migration at the right SQL file. * Fixed header.php * Code cleanup from code review: - Added IGNORE to SQL scripts. - Added try-catch to executeScriptWithTransaction(). - Various comment changes. * Fixed naming issue Nullable tax category ID migration now runs the correct script. * Updated SQL Replaced INSERT WHERE NOT EXISTS in missing config keys sql script to use a single INSERT IGNORE. * Updated migration helper Updated executeScriptWithTransaction to use transRollback --------- Co-authored-by: Joe Williams --- .../20250716170000_MissingConfigKeys.php | 10 +-- .../20250729170000_NullableTaxCategoryId.php | 6 +- .../sqlscripts/3.4.2_missing_config_keys.sql | 46 +++--------- app/Helpers/migration_helper.php | 71 +++++++++++++++++-- 4 files changed, 84 insertions(+), 49 deletions(-) diff --git a/app/Database/Migrations/20250716170000_MissingConfigKeys.php b/app/Database/Migrations/20250716170000_MissingConfigKeys.php index 4ae39c9cc..a519c3e7e 100644 --- a/app/Database/Migrations/20250716170000_MissingConfigKeys.php +++ b/app/Database/Migrations/20250716170000_MissingConfigKeys.php @@ -11,12 +11,14 @@ class Migration_MissingConfigKeys extends Migration */ public function up(): void { - error_log('Migrating config table'); - + error_log('Migrating config keys...'); helper('migration'); - execute_script(APPPATH . 'Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql'); - error_log('Migrating config table'); + if (executeScriptWithTransaction(APPPATH . 'Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql')) { + error_log('Migrated config keys.'); + } else { + error_log('Failed to migrate config keys.'); + } } /** diff --git a/app/Database/Migrations/20250729170000_NullableTaxCategoryId.php b/app/Database/Migrations/20250729170000_NullableTaxCategoryId.php index 97daa4df2..ba4378981 100644 --- a/app/Database/Migrations/20250729170000_NullableTaxCategoryId.php +++ b/app/Database/Migrations/20250729170000_NullableTaxCategoryId.php @@ -11,12 +11,12 @@ class Migration_NullableTaxCategoryId extends Migration */ public function up(): void { - error_log('Migrating config table'); + error_log('Migrating nullable tax category ID'); helper('migration'); - execute_script(APPPATH . 'Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql'); + execute_script(APPPATH . 'Database/Migrations/sqlscripts/3.4.2_nullable_tax_category_id.sql'); - error_log('Migrating config table'); + error_log('Migrated nullable tax category ID'); } /** diff --git a/app/Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql b/app/Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql index 8bbb1a1cd..79ecfa312 100644 --- a/app/Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql +++ b/app/Database/Migrations/sqlscripts/3.4.2_missing_config_keys.sql @@ -1,35 +1,11 @@ -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'msg_msg', '' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'msg_msg'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'msg_pwd', '' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'msg_pwd'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'msg_uid', '' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'msg_uid'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'msg_src', '' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'msg_src'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'smtp_timeout', 5000 - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'smtp_timeout'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'smtp_crypto', 'tls' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'smtp_crypto'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'smtp_port', 587 - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'smtp_port'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'mailpath', '/usr/bin/sendmail' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'mailpath'); - -INSERT INTO ospos_app_config (`key`, `value`) -SELECT 'protocol', 'sendmail' - WHERE NOT EXISTS (SELECT 1 FROM ospos_app_config WHERE `key` = 'protocol'); +INSERT IGNORE INTO ospos_app_config (`key`, `value`) +VALUES + ('msg_msg', ''), + ('msg_pwd', ''), + ('msg_uid', ''), + ('msg_src', ''), + ('smtp_timeout', 5000), + ('smtp_crypto', 'tls'), + ('smtp_port', 587), + ('mailpath', '/usr/bin/sendmail'), + ('protocol', 'sendmail'); diff --git a/app/Helpers/migration_helper.php b/app/Helpers/migration_helper.php index 9138b95e8..8a1221047 100644 --- a/app/Helpers/migration_helper.php +++ b/app/Helpers/migration_helper.php @@ -3,9 +3,11 @@ use Config\Database; /** - * Migration helper + * Migration helper. + * @param string $path Path to migration script. + * @return bool Whether the migration executed successfully. */ -function execute_script(string $path): void +function execute_script(string $path): bool { $version = preg_replace("/(.*_)?(.*).sql/", "$2", $path); error_log("Migrating to $version (file: $path)"); @@ -16,17 +18,72 @@ function execute_script(string $path): void $db = Database::connect(); + $success = true; foreach ($sqls as $statement) { $statement = "$statement;"; + $hadError = !$db->simpleQuery($statement); - if (!$db->simpleQuery($statement)) { + if ($hadError) { + $success = false; foreach ($db->error() as $error) { error_log("error: $error"); } } } - error_log("Migrated to $version"); + if ($success) { + error_log("Successfully migrated to $version"); + } else { + error_log("Could not migrate to $version."); + } + + return $success; +} + +/** + * Migration helper that uses a transaction. + * @param string $path Path to migration script. + * @return bool Whether the migration executed successfully. + */ +function executeScriptWithTransaction(string $path): bool +{ + $version = preg_replace("/(.*_)?(.*).sql/", "$2", $path); + error_log("Migrating to $version (file: $path) with transaction"); + + $sql = file_get_contents($path); + $sqls = explode(';', $sql); + array_pop($sqls); + + $db = Database::connect(); + $db->transStart(); + + $success = true; + try { + foreach ($sqls as $statement) { + $statement = "$statement;"; + $hadError = !$db->query($statement); + + if ($hadError) { + $success = false; + foreach ($db->error() as $error) { + error_log("error: $error"); + } + } + } + } catch (Exception $e) { + error_log("Could not migrate to $version: " . $e->getMessage()); + $db->transRollback(); + return false; + } + + if ($success) { + error_log("Successfully migrated to $version"); + } else { + error_log("Could not migrate to $version."); + } + + $db->transComplete(); + return $success; } /** @@ -228,9 +285,9 @@ function dropColumnIfExists(string $table, string $column): void // Check if the column exists in the table $builder->select('COLUMN_NAME') - ->where('TABLE_SCHEMA', $db->database) - ->where('TABLE_NAME', $prefix . $table) - ->where('COLUMN_NAME', $column); + ->where('TABLE_SCHEMA', $db->database) + ->where('TABLE_NAME', $prefix . $table) + ->where('COLUMN_NAME', $column); $query = $builder->get();