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.

diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php
index 53560c22cd15cfdfafd232a3e6ae0843a7952075..c540c93fbf94ab71220f488d00b3bf14ef2c2d0f 100644 (file)
--- 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 f6cb6f30e1dba19f7ce9ec4a18ae712569b18ceb..efdd8ccb2b5cc40040ee6ce770dae91c4d8c3776 100644 (file)
--- 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']);
+  }
+
 }