From 235592e09103ed7d6dff6e1eb91d9bc3cae0ee4e Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 13 Oct 2020 16:05:38 +1300
Subject: [PATCH] dev/core#2120 Do not attempt to obsolete primary key on log
 tables

---
 CRM/Logging/Schema.php                   | 12 +++++++-----
 tests/phpunit/CRM/Logging/SchemaTest.php | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php
index 2d1d0e3037..e323c82114 100644
--- a/CRM/Logging/Schema.php
+++ b/CRM/Logging/Schema.php
@@ -715,7 +715,7 @@ WHERE  table_schema IN ('{$this->db}', '{$civiDB}')";
         $logTableSpecs[$col] = [];
       }
       $specDiff = array_diff($civiTableSpecs[$col], $logTableSpecs[$col]);
-      if (!empty($specDiff) && $col != 'id' && !in_array($col, $diff['ADD'])) {
+      if (!empty($specDiff) && $col !== 'id' && !in_array($col, $diff['ADD'])) {
         if (empty($colSpecs['EXTRA']) || (!empty($colSpecs['EXTRA']) && $colSpecs['EXTRA'] !== 'auto_increment')) {
           // ignore 'id' column for any spec changes, to avoid any auto-increment mysql errors
           if ($civiTableSpecs[$col]['DATA_TYPE'] != CRM_Utils_Array::value('DATA_TYPE', $logTableSpecs[$col])
@@ -725,14 +725,14 @@ 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' &&
+          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'
+            $logTableSpecs[$col]['IS_NULLABLE'] === 'NO'
           ) {
             // if is-null property is different, and log table's column is NOT-NULL, surely consider the column
             $diff['MODIFY'][] = $col;
@@ -751,7 +751,9 @@ WHERE  table_schema IN ('{$this->db}', '{$civiDB}')";
     $oldCols = array_diff(array_keys($logTableSpecs), array_keys($civiTableSpecs));
     foreach ($oldCols as $col) {
       if (!in_array($col, ['log_date', 'log_conn_id', 'log_user_id', 'log_action']) &&
-        $logTableSpecs[$col]['IS_NULLABLE'] == 'NO'
+        $logTableSpecs[$col]['IS_NULLABLE'] === 'NO'
+        // This could be to support replication - https://lab.civicrm.org/dev/core/-/issues/2120
+        && $logTableSpecs[$col]['EXTRA'] !== 'auto_increment'
       ) {
         // if its a column present only in log table, not among those used by log tables for special purpose, and not-null
         $diff['OBSOLETE'][] = $col;
@@ -806,7 +808,7 @@ COLS;
     $mysqlEngines = [];
     $engines = CRM_Core_DAO::executeQuery("SHOW ENGINES");
     while ($engines->fetch()) {
-      if ($engines->Support == 'YES' || $engines->Support == 'DEFAULT') {
+      if ($engines->Support === 'YES' || $engines->Support === 'DEFAULT') {
         $mysqlEngines[] = $engines->Engine;
       }
     }
diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php
index 19631715e2..02bf5598ec 100644
--- a/tests/phpunit/CRM/Logging/SchemaTest.php
+++ b/tests/phpunit/CRM/Logging/SchemaTest.php
@@ -159,6 +159,9 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     }
   }
 
+  /**
+   * Test that autoincrement keys are handled sensibly in logging table reconciliation.
+   */
   public function testAutoIncrementNonIdColumn() {
     CRM_Core_DAO::executeQuery("CREATE TABLE `civicrm_test_table` (
       test_id  int(10) unsigned NOT NULL AUTO_INCREMENT,
@@ -167,10 +170,20 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     $schema = new CRM_Logging_Schema();
     $schema->enableLogging();
     $diffs = $schema->columnsWithDiffSpecs("civicrm_test_table", "log_civicrm_test_table");
-    // Test that just havving a non id nanmed column with Auto Increment doesn't create diffs
+    // Test that just having a non id named column with Auto Increment doesn't create diffs
     $this->assertTrue(empty($diffs['MODIFY']));
     $this->assertTrue(empty($diffs['ADD']));
     $this->assertTrue(empty($diffs['OBSOLETE']));
+
+    // Check we can add a primary key to the log table and it will not be treated as obsolete.
+    CRM_Core_DAO::executeQuery("
+      ALTER TABLE log_civicrm_test_table ADD COLUMN `log_id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
+      ADD PRIMARY KEY (`log_id`)
+   ");
+    \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = [];
+    $diffs = $schema->columnsWithDiffSpecs('civicrm_test_table', "log_civicrm_test_table");
+    $this->assertTrue(empty($diffs['OBSOLETE']));
+
     CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_test_table ADD COLUMN test_varchar varchar(255) DEFAULT NULL");
     \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = [];
     // Check that it still picks up new columns added.
-- 
2.25.1