From 78b7cb8930864ab3965a7eb7a701daf8d2e4135e Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Tue, 21 Sep 2021 12:52:26 +1200
Subject: [PATCH] Fix bug where log hook is ignored on custom field create

It is possible to specify that various tables should not
result in a log table via the `alterLogTables` hook.

This is useful for excluding tables of low information
value or tables like summary fields that are calculated
values. However, when altering custom fields this
hook's output was being ignored as it was going through
the specific path of
which did not have awareness of this hook.

This code path is the primary use for this function.

It's also called via createMissingLogTables - but in
that case the calculation is already done. It is not
elsewhere in git universe and the return value
is never used, so I removed it
---
 CRM/Core/BAO/CustomField.php              |  3 +--
 CRM/Core/DAO.php                          |  2 +-
 CRM/Logging/Schema.php                    | 16 ++++++++--------
 tests/phpunit/CRM/Logging/LoggingTest.php | 22 ++++++++++++++++++++++
 4 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php
index 73944a1e72..c5421a604b 100644
--- a/CRM/Core/BAO/CustomField.php
+++ b/CRM/Core/BAO/CustomField.php
@@ -134,9 +134,8 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField {
    * @param array $records
    * @return CRM_Core_DAO_CustomField[]
    * @throws CRM_Core_Exception
-   * @throws CiviCRM_API3_Exception
    */
-  public static function writeRecords(array $records) {
+  public static function writeRecords(array $records): array {
     $addedColumns = $sql = $customFields = $pre = $post = [];
     foreach ($records as $index => $params) {
       CRM_Utils_Hook::pre(empty($params['id']) ? 'create' : 'edit', 'CustomField', $params['id'] ?? NULL, $params);
diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index 774b3548b6..2a375fbca1 100644
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -932,7 +932,7 @@ class CRM_Core_DAO extends DB_DataObject {
    * @return static[]
    * @throws CRM_Core_Exception
    */
-  public static function writeRecords(array $records) {
+  public static function writeRecords(array $records): array {
     $results = [];
     foreach ($records as $record) {
       $results[] = static::writeRecord($record);
diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php
index ad09e3a32c..9d09e62d20 100644
--- a/CRM/Logging/Schema.php
+++ b/CRM/Logging/Schema.php
@@ -429,16 +429,18 @@ AND    (TABLE_NAME LIKE 'log_civicrm_%' $nonStandardTableNameString )
    *   name of the relevant table.
    * @param array $cols
    *   Mixed array of columns to add or null (to check for the missing columns).
-   *
-   * @return bool
    */
-  public function fixSchemaDifferencesFor($table, $cols = []) {
-    if (empty($table)) {
-      return FALSE;
+  public function fixSchemaDifferencesFor(string $table, array $cols = []): void {
+    if (!in_array($table, $this->tables, TRUE)) {
+      // Create the table if the log table does not exist and
+      // the table is in 'this->tables'. This latter array
+      // could have been altered by a hook if the site does not
+      // want to log a specific table.
+      return;
     }
     if (empty($this->logs[$table])) {
       $this->createLogTableFor($table);
-      return TRUE;
+      return;
     }
 
     if (empty($cols)) {
@@ -480,8 +482,6 @@ AND    (TABLE_NAME LIKE 'log_civicrm_%' $nonStandardTableNameString )
     }
 
     $this->resetSchemaCacheForTable("log_$table");
-
-    return TRUE;
   }
 
   /**
diff --git a/tests/phpunit/CRM/Logging/LoggingTest.php b/tests/phpunit/CRM/Logging/LoggingTest.php
index 050fce542a..58ba85a002 100644
--- a/tests/phpunit/CRM/Logging/LoggingTest.php
+++ b/tests/phpunit/CRM/Logging/LoggingTest.php
@@ -38,6 +38,28 @@ class CRM_Logging_LoggingTest extends CiviUnitTestCase {
     $this->assertNotEmpty(CRM_Core_DAO::singleValueQuery("SHOW tables LIKE 'log_abcd'"));
   }
 
+  /**
+   * Test that hooks removing tables from logging are respected during custom field add.
+   *
+   * During custom field save logging is only handled for the affected table.
+   * We need to make sure this respects hooks to remove from the logging set.
+   */
+  public function testLoggingHookIgnore(): void {
+    $this->hookClass->setHook('civicrm_alterLogTables', [$this, 'ignoreSillyName']);
+    Civi::settings()->set('logging', TRUE);
+    $this->createCustomGroupWithFieldOfType(['table_name' => 'silly_name']);
+    $this->assertEmpty(CRM_Core_DAO::singleValueQuery("SHOW tables LIKE 'log_silly_name'"));
+  }
+
+  /**
+   * Implement hook to cause our log table to be ignored.
+   *
+   * @param array $logTableSpec
+   */
+  public function ignoreSillyName(array &$logTableSpec): void {
+    unset($logTableSpec['silly_name']);
+  }
+
   /**
    * Test creating logging schema when database is in multilingual mode.
    */
-- 
2.25.1