From 89d7d62ee836696f38c4289a9417adf44b576d16 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Thu, 4 Jul 2019 14:36:30 +1200
Subject: [PATCH] e-notice fix & unit test

This addresses a fairly obscure enotice. The notice occurs when a table has been excluded from
triggers by a hook (as makes sense if the table includes only calculated fields) and then a new
custom field is added to the table (or presumably edited or removed). In this case we should
not go through the whole trigger handling loop.

Note I see it as a feature rather than a bug that this would also apply to triggers that
update the contact.modified_date as it's not helpful to update this when updating a
calculation - if someone decides to intervene by hook we should trust them
---
 CRM/Logging/Schema.php                   |  7 ++++
 tests/phpunit/CRM/Logging/SchemaTest.php | 45 ++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php
index 8b5f53d6b3..72c8322a56 100644
--- a/CRM/Logging/Schema.php
+++ b/CRM/Logging/Schema.php
@@ -913,6 +913,13 @@ COLS;
 
     // logging is enabled, so now lets create the trigger info tables
     foreach ($tableNames as $table) {
+      if (!isset($this->logTableSpec[$table])) {
+        // Per testIgnoreCustomTableByHook this would be unset if a hook had
+        // intervened to prevent logging / triggers on this table.
+        // This could go to the extent of blocking the updates to 'modified_date'
+        // which makes sense, in particular, for calculated fields.
+        continue;
+      }
       $columns = $this->columnsOf($table, $force);
 
       // only do the change if any data has changed
diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php
index f7fcdb8e51..7b1a06ff0f 100644
--- a/tests/phpunit/CRM/Logging/SchemaTest.php
+++ b/tests/phpunit/CRM/Logging/SchemaTest.php
@@ -10,10 +10,16 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     parent::setUp();
   }
 
+  /**
+   * Clean up after test.
+   *
+   * @throws \CRM_Core_Exception
+   */
   public function tearDown() {
-    parent::tearDown();
     $schema = new CRM_Logging_Schema();
     $schema->disableLogging();
+    parent::tearDown();
+    $this->quickCleanup(['civicrm_contact'], TRUE);
     $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");
@@ -21,6 +27,11 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_enum_change");
   }
 
+  /**
+   * Data provider for testing query re-writing.
+   *
+   * @return array
+   */
   public function queryExamples() {
     $examples = [];
     $examples[] = ["`modified_date` timestamp NULL DEFAULT current_timestamp() ON UPDATE current_timestamp() COMMENT 'When the mailing (or closely related entity) was created or modified or deleted.'", "`modified_date` timestamp NULL  COMMENT 'When the mailing (or closely related entity) was created or modified or deleted.'"];
@@ -62,6 +73,29 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     }
   }
 
+  /**
+   * Tests that choosing to ignore a custom table does not result in e-notices.
+   */
+  public function testIgnoreCustomTableByHook() {
+    $group = $this->customGroupCreate();
+    Civi::settings()->set('logging', TRUE);
+    $this->hookClass->setHook('civicrm_alterLogTables', [$this, 'noCustomTables']);
+    $this->customFieldCreate(['custom_group_id' => $group['id']]);
+  }
+
+  /**
+   * Remove all custom tables from tables to be logged.
+   *
+   * @param array $logTableSpec
+   */
+  public function noCustomTables(&$logTableSpec) {
+    foreach (array_keys($logTableSpec) as $index) {
+      if (substr($index, 0, 14) === 'civicrm_value_') {
+        unset($logTableSpec[$index]);
+      }
+    }
+  }
+
   /**
    * Test that existing log tables with ARCHIVE engine are converted to InnoDB
    *
@@ -80,16 +114,21 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     $schema->updateLogTableSchema(['updateChangedEngineConfig' => FALSE, 'forceEngineMigration' => FALSE]);
     $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl");
     while ($log_table->fetch()) {
-      $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table);
+      $this->assertRegExp('/ENGINE=ARCHIVE/', $log_table->Create_Table);
     }
     // update with forceEngineMigration should convert to InnoDB
     $schema->updateLogTableSchema(['updateChangedEngineConfig' => FALSE, 'forceEngineMigration' => TRUE]);
     $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl");
     while ($log_table->fetch()) {
-      $this->assertRegexp('/ENGINE=InnoDB/', $log_table->Create_Table);
+      $this->assertRegExp('/ENGINE=InnoDB/', $log_table->Create_Table);
     }
   }
 
+  /**
+   * Alter the engine on the log tables.
+   *
+   * @param $logTableSpec
+   */
   public function alterLogTables(&$logTableSpec) {
     foreach (array_keys($logTableSpec) as $tableName) {
       $logTableSpec[$tableName]['engine'] = 'MyISAM';
-- 
2.25.1