From f34f504a22996038f5f128eea49e52eb9ecdb775 Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Fri, 25 Jan 2019 19:57:49 +0100 Subject: [PATCH] CRM/Logging - Improve enum handling in schema diff Instead of storing permitted enum values in the LENGTH array key when extracting column information, this adds a separate ENUM_VALUES key. When schema differences are calculated for enum columns, this value triggers a change when new permitted values are added. --- CRM/Logging/Schema.php | 19 ++++++- tests/phpunit/CRM/Logging/SchemaTest.php | 68 ++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php index 53560c22cd..c540c93fbf 100644 --- a/CRM/Logging/Schema.php +++ b/CRM/Logging/Schema.php @@ -607,9 +607,20 @@ WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; 'EXTRA' => $dao->EXTRA, ); if (($first = strpos($dao->COLUMN_TYPE, '(')) != 0) { - \Civi::$statics[__CLASS__]['columnSpecs'][$dao->TABLE_NAME][$dao->COLUMN_NAME]['LENGTH'] = substr( + // this extracts the value between parentheses after the column type. + // it could be the column length, i.e. "int(8)", "decimal(20,2)") + // or the permitted values of an enum (e.g. "enum('A','B')") + $parValue = substr( $dao->COLUMN_TYPE, $first + 1, strpos($dao->COLUMN_TYPE, ')') - $first - 1 ); + if (strpos($parValue, "'") === FALSE) { + // no quote in value means column length + \Civi::$statics[__CLASS__]['columnSpecs'][$dao->TABLE_NAME][$dao->COLUMN_NAME]['LENGTH'] = $parValue; + } + else { + // single quote means enum permitted values + \Civi::$statics[__CLASS__]['columnSpecs'][$dao->TABLE_NAME][$dao->COLUMN_NAME]['ENUM_VALUES'] = $parValue; + } } } } @@ -649,6 +660,12 @@ WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; // if data-type is different, surely consider the column $diff['MODIFY'][] = $col; } + elseif ($civiTableSpecs[$col]['DATA_TYPE'] == 'enum' && + CRM_Utils_Array::value('ENUM_VALUES', $civiTableSpecs[$col]) != CRM_Utils_Array::value('ENUM_VALUES', $logTableSpecs[$col]) + ) { + // column is enum and the permitted values have changed + $diff['MODIFY'][] = $col; + } elseif ($civiTableSpecs[$col]['IS_NULLABLE'] != CRM_Utils_Array::value('IS_NULLABLE', $logTableSpecs[$col]) && $logTableSpecs[$col]['IS_NULLABLE'] == 'NO' ) { diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php index f6cb6f30e1..efdd8ccb2b 100644 --- a/tests/phpunit/CRM/Logging/SchemaTest.php +++ b/tests/phpunit/CRM/Logging/SchemaTest.php @@ -17,6 +17,8 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { $schema->dropAllLogTables(); CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_table"); CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_column_info"); + CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_length_change"); + CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_enum_change"); } public function queryExamples() { @@ -129,6 +131,8 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { test_id int(10) unsigned NOT NULL AUTO_INCREMENT, test_varchar varchar(42) NOT NULL, test_integer int(8) NULL, + test_decimal decimal(20,2), + test_enum enum('A','B','C'), test_integer_default int(8) DEFAULT 42, test_date date DEFAULT NULL, PRIMARY KEY (`test_id`) @@ -151,6 +155,13 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { $this->assertEquals('8', $ci['test_integer']['LENGTH']); $this->assertEquals('YES', $ci['test_integer']['IS_NULLABLE']); + $this->assertEquals('decimal', $ci['test_decimal']['DATA_TYPE']); + $this->assertEquals('20,2', $ci['test_decimal']['LENGTH']); + + $this->assertEquals('enum', $ci['test_enum']['DATA_TYPE']); + $this->assertEquals("'A','B','C'", $ci['test_enum']['ENUM_VALUES']); + $this->assertArrayNotHasKey('LENGTH', $ci['test_enum']); + $this->assertEquals('42', $ci['test_integer_default']['COLUMN_DEFAULT']); $this->assertEquals('date', $ci['test_date']['DATA_TYPE']); @@ -165,4 +176,61 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { $this->assertContains('index_sort_name', $indexes); } + public function testLengthChange() { + CRM_Core_DAO::executeQuery("CREATE TABLE `civicrm_test_length_change` ( + test_id int(10) unsigned NOT NULL AUTO_INCREMENT, + test_integer int(4) NULL, + test_decimal decimal(20,2) NULL, + PRIMARY KEY (`test_id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci"); + $schema = new CRM_Logging_Schema(); + $schema->enableLogging(); + CRM_Core_DAO::executeQuery( + "ALTER TABLE civicrm_test_length_change + CHANGE COLUMN test_integer test_integer int(6) NULL, + CHANGE COLUMN test_decimal test_decimal decimal(22,2) NULL" + ); + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + // need to do it twice so the columnSpecs static is refreshed + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + $ci = \Civi::$statics['CRM_Logging_Schema']['columnSpecs']; + // length should increase + $this->assertEquals(6, $ci['log_civicrm_test_length_change']['test_integer']['LENGTH']); + $this->assertEquals('22,2', $ci['log_civicrm_test_length_change']['test_decimal']['LENGTH']); + CRM_Core_DAO::executeQuery( + "ALTER TABLE civicrm_test_length_change + CHANGE COLUMN test_integer test_integer int(4) NULL, + CHANGE COLUMN test_decimal test_decimal decimal(20,2) NULL" + ); + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + $ci = \Civi::$statics['CRM_Logging_Schema']['columnSpecs']; + // length should not decrease + $this->assertEquals(6, $ci['log_civicrm_test_length_change']['test_integer']['LENGTH']); + $this->assertEquals('22,2', $ci['log_civicrm_test_length_change']['test_decimal']['LENGTH']); + } + + public function testEnumChange() { + CRM_Core_DAO::executeQuery("CREATE TABLE `civicrm_test_enum_change` ( + test_id int(10) unsigned NOT NULL AUTO_INCREMENT, + test_enum enum('A','B','C') NULL, + PRIMARY KEY (`test_id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci"); + $schema = new CRM_Logging_Schema(); + $schema->enableLogging(); + CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_test_enum_change CHANGE COLUMN test_enum test_enum enum('A','B','C','D') NULL"); + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + // need to do it twice so the columnSpecs static is refreshed + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = []; + $schema->fixSchemaDifferences(); + $ci = \Civi::$statics['CRM_Logging_Schema']['columnSpecs']; + // new enum value should be included + $this->assertEquals("'A','B','C','D'", $ci['civicrm_test_enum_change']['test_enum']['ENUM_VALUES']); + } + } -- 2.25.1