Fix trigger generation for modified_date on custom data
authoreileen <emcnaughton@wikimedia.org>
Tue, 17 Apr 2018 00:03:00 +0000 (12:03 +1200)
committereileen <emcnaughton@wikimedia.org>
Tue, 17 Apr 2018 05:44:13 +0000 (17:44 +1200)
In 4.7.25 https://github.com/civicrm/civicrm-core/pull/10754/commits introduced
some modifications to the generation of triggers to update the modified date field.

It basically derived the entity being extended by a table and then if that entity had a
modified_date field then a trigger would be created to update that field.

However, a bug in the CRM_Core_BAO_CustomGroup::getAllCustomGroupsByBaseEntity function
meant that incorrect additional tables are also being updated for custom fields.

For entities extending contact the contact table AND the mailing table are updated. e.g
```
CREATE TRIGGER {mycustomtable}...
UPDATE civicrm_contact SET modified_date = CURRENT_TIMESTAMP WHERE id = NEW.entity_id;

UPDATE civicrm_mailing SET modified_date = CURRENT_TIMESTAMP WHERE id = NEW.entity_id;
```

For entities that extend tables that should not attract a trigger ONLY
the mailing table is updated.

The bug in CRM_Core_BAO_CustomGroup::getAllCustomGroupsByBaseEntity is that it adds the
WHERE clause 'AND extends IN ($entityType)' ONLY if $entityType is in a whitelist.

Invalid entities result in no filtering.

As a fix using a whitelist is no longer valid - we support any entity that might
be configured now so simply filtering on the entity makes sense.

Other paths to this function seem unlikely to pass in invalid entities & hence trigger this bug.

CRM/Core/BAO/CustomGroup.php
tests/phpunit/CRM/Logging/SchemaTest.php

index 1ec8b471441f22c4622659c58ea7599d7c6a0c7a..6653d6b4f7fb8770d7c294227dc5cc1382b37785 100644 (file)
@@ -1259,6 +1259,10 @@ ORDER BY civicrm_custom_group.weight,
    */
   private static function _addWhereAdd(&$customGroupDAO, $entityType, $entityID = NULL, $allSubtypes = FALSE) {
     $addSubtypeClause = FALSE;
+    // This function isn't really accessible with user data but since the string
+    // is not passed as a param to the query CRM_Core_DAO::escapeString seems like a harmless
+    // precaution.
+    $entityType = CRM_Core_DAO::escapeString($entityType);
 
     switch ($entityType) {
       case 'Contact':
@@ -1281,13 +1285,7 @@ ORDER BY civicrm_custom_group.weight,
         }
         break;
 
-      case 'Case':
-      case 'Location':
-      case 'Address':
-      case 'Activity':
-      case 'Contribution':
-      case 'Membership':
-      case 'Participant':
+      default:
         $customGroupDAO->whereAdd("extends IN ('$entityType')");
         break;
     }
index 9c8b7a1a7adb52f94abafbdbcd6deab87f69db3e..2b238077e13d2f364366e935f8425535ed554711 100644 (file)
@@ -43,6 +43,26 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase {
     }
   }
 
+  /**
+   * Test correct creation of modified date triggers.
+   *
+   * Specifically we are testing that the contact table modified date and
+   * ONLY the contact table modified date is updated when the custom field is updated.
+   *
+   * (At point of writing this the modification was leaking to the mailing table).
+   */
+  public function testTriggers() {
+    $customGroup = $this->entityCustomGroupWithSingleFieldCreate('Contact', 'ContactTest....');
+    Civi::service('sql_triggers')->rebuild();
+    $log_table = CRM_Core_DAO::executeQuery("SHOW TRIGGERS WHERE `Trigger` LIKE 'civicrm_value_contact_{$customGroup['custom_group_id']}_after_insert%'");
+
+    while ($log_table->fetch()) {
+      $this->assertContains('UPDATE civicrm_contact SET modified_date = CURRENT_TIMESTAMP WHERE id = NEW.entity_id;', $log_table->Statement, "Contact modification update should be in the trigger :\n" . $log_table->Statement);
+      $this->assertNotContains('civicrm_mailing', $log_table->Statement, 'Contact field should not update mailing table');
+      $this->assertEquals(1, substr_count($log_table->Statement, 'SET modified_date'), 'Modified date should only be updated on one table (here it is contact)');
+    }
+  }
+
   public function testAutoIncrementNonIdColumn() {
     CRM_Core_DAO::executeQuery("CREATE TABLE `civicrm_test_table` (
       test_id  int(10) unsigned NOT NULL AUTO_INCREMENT,