From: colemanw Date: Tue, 17 Oct 2023 01:24:05 +0000 (-0400) Subject: ManagedEntities - Don't recreate deleted records unless update policy is 'always' X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=40d44ec03d56b5dbd6a92de3861f4ed1521e2e64;p=civicrm-core.git ManagedEntities - Don't recreate deleted records unless update policy is 'always' --- diff --git a/CRM/Core/BAO/Managed.php b/CRM/Core/BAO/Managed.php index 4204350105..38fc061a7c 100644 --- a/CRM/Core/BAO/Managed.php +++ b/CRM/Core/BAO/Managed.php @@ -50,11 +50,14 @@ class CRM_Core_BAO_Managed extends CRM_Core_DAO_Managed implements Civi\Core\Hoo * @param \Civi\Core\Event\PostEvent $event */ public static function on_hook_civicrm_post(\Civi\Core\Event\PostEvent $event) { - // When an entity is deleted, delete the corresponding Managed record + // When an entity is deleted by the user, nullify 'entity_id' from corresponding Managed record + // This tells the ManagedEntity system that the entity was manually deleted, + // and should be recreated at the discretion of the `update` policy. if ($event->action === 'delete' && $event->id && self::isApi4ManagedType($event->entity)) { - \Civi\Api4\Managed::delete(FALSE) + \Civi\Api4\Managed::update(FALSE) ->addWhere('entity_type', '=', $event->entity) ->addWhere('entity_id', '=', $event->id) + ->addValue('entity_id', NULL) ->execute(); } // When an entity is updated, update the timestamp in corresponding Managed record diff --git a/CRM/Core/DAO/Managed.php b/CRM/Core/DAO/Managed.php index cad889fc02..d23d84643e 100644 --- a/CRM/Core/DAO/Managed.php +++ b/CRM/Core/DAO/Managed.php @@ -6,7 +6,7 @@ * * Generated from xml/schema/CRM/Core/Managed.xml * DO NOT EDIT. Generated by CRM_Core_CodeGen - * (GenCodeChecksum:43846305220f9d67d37b9a1f910ad085) + * (GenCodeChecksum:4ff1b19106d4bc7810d4aa9eb2a73f65) */ /** @@ -67,9 +67,9 @@ class CRM_Core_DAO_Managed extends CRM_Core_DAO { public $entity_type; /** - * Foreign key to the referenced item. + * Soft foreign key to the referenced item. * - * @var int|string + * @var int|string|null * (SQL type: int unsigned) * Note that values will be retrieved from the database as a string. */ @@ -212,8 +212,7 @@ class CRM_Core_DAO_Managed extends CRM_Core_DAO { 'name' => 'entity_id', 'type' => CRM_Utils_Type::T_INT, 'title' => ts('Entity ID'), - 'description' => ts('Foreign key to the referenced item.'), - 'required' => TRUE, + 'description' => ts('Soft foreign key to the referenced item.'), 'usage' => [ 'import' => FALSE, 'export' => FALSE, diff --git a/CRM/Core/ManagedEntities.php b/CRM/Core/ManagedEntities.php index 27353fd21b..4832c9a0d0 100644 --- a/CRM/Core/ManagedEntities.php +++ b/CRM/Core/ManagedEntities.php @@ -200,12 +200,18 @@ class CRM_Core_ManagedEntities { } /** - * Create a new entity. + * Create a new entity (if policy allows). * * @param array $item * Entity specification (per hook_civicrm_managedEntities). */ protected function insertNewEntity(array $item) { + // If entity has previously been created, only re-insert if 'update' policy is 'always' + // NOTE: $item[id] is the id of the `civicrm_managed` row not the entity itself + // If that id exists, then we know the entity was inserted previously and subsequently deleted. + if (!empty($item['id']) && $item['update'] !== 'always') { + return; + } $params = $item['params']; // APIv4 if ($params['version'] == 4) { @@ -233,6 +239,8 @@ class CRM_Core_ManagedEntities { } $dao = new CRM_Core_DAO_Managed(); + // If re-inserting the entity, we'll update instead of create the managed record. + $dao->id = $item['id'] ?? NULL; $dao->module = $item['module']; $dao->name = $item['name']; $dao->entity_type = $item['entity_type']; @@ -591,29 +599,20 @@ class CRM_Core_ManagedEntities { $plan = []; foreach ($managedEntities as $managedEntity) { $key = "{$managedEntity['module']}_{$managedEntity['name']}_{$managedEntity['entity_type']}"; - // Set to disable or delete if module is disabled or missing - it will be overwritten below module is active. + // Set to disable or delete if module is disabled or missing - it will be overwritten below if module is active. $action = $this->isModuleDisabled($managedEntity['module']) ? 'disable' : 'delete'; $plan[$key] = array_merge($managedEntity, ['managed_action' => $action]); } foreach ($declarations as $declaration) { $key = "{$declaration['module']}_{$declaration['name']}_{$declaration['entity']}"; - if (isset($plan[$key])) { - $plan[$key]['params'] = $declaration['params']; - $plan[$key]['managed_action'] = 'update'; - $plan[$key]['cleanup'] = $declaration['cleanup'] ?? NULL; - $plan[$key]['update'] = $declaration['update'] ?? 'always'; - } - else { - $plan[$key] = [ - 'module' => $declaration['module'], - 'name' => $declaration['name'], - 'entity_type' => $declaration['entity'], - 'managed_action' => 'create', - 'params' => $declaration['params'], - 'cleanup' => $declaration['cleanup'] ?? NULL, - 'update' => $declaration['update'] ?? 'always', - ]; - } + // Set action to update if already managed + $plan[$key]['managed_action'] = empty($plan[$key]['entity_id']) ? 'create' : 'update'; + $plan[$key]['module'] = $declaration['module']; + $plan[$key]['name'] = $declaration['name']; + $plan[$key]['entity_type'] = $declaration['entity']; + $plan[$key]['params'] = $declaration['params']; + $plan[$key]['cleanup'] = $declaration['cleanup'] ?? NULL; + $plan[$key]['update'] = $declaration['update'] ?? 'always'; } return $plan; } diff --git a/CRM/Upgrade/Incremental/php/FiveSixtyEight.php b/CRM/Upgrade/Incremental/php/FiveSixtyEight.php index 9a3f16a4d5..93ca90b2cf 100644 --- a/CRM/Upgrade/Incremental/php/FiveSixtyEight.php +++ b/CRM/Upgrade/Incremental/php/FiveSixtyEight.php @@ -33,6 +33,7 @@ class CRM_Upgrade_Incremental_php_FiveSixtyEight extends CRM_Upgrade_Incremental $this->addTask('Update Tag.name field', 'alterColumn', 'civicrm_tag', 'name', "varchar(64) NOT NULL COMMENT 'Unique machine name'"); $this->addTask('Update Tag.created_date field', 'alterColumn', 'civicrm_tag', 'created_date', "datetime DEFAULT CURRENT_TIMESTAMP COMMENT 'Date and time that tag was created.'"); $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev); + $this->addTask('Update civicrm_managed.entity_id', 'alterColumn', 'civicrm_managed', 'entity_id', "int unsigned COMMENT 'Soft foreign key to the referenced item.'"); $this->addTask('Update civicrm_managed.module', 'alterColumn', 'civicrm_managed', 'module', "varchar(255) NOT NULL COMMENT 'Name of the module which declared this object (soft FK to civicrm_extension.full_name)'"); $this->addTask('Update civicrm_managed.name', 'alterColumn', 'civicrm_managed', 'name', "varchar(255) NOT NULL COMMENT 'Symbolic name used by the module to identify the object'"); $this->addTask('Update civicrm_managed.cleanup', 'alterColumn', 'civicrm_managed', 'cleanup', "varchar(16) NOT NULL DEFAULT 'always' COMMENT 'Policy on when to cleanup entity (always, never, unused)'"); diff --git a/tests/phpunit/api/v4/Entity/ManagedEntityTest.php b/tests/phpunit/api/v4/Entity/ManagedEntityTest.php index 85e6b26241..9bc8946b14 100644 --- a/tests/phpunit/api/v4/Entity/ManagedEntityTest.php +++ b/tests/phpunit/api/v4/Entity/ManagedEntityTest.php @@ -97,8 +97,6 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti ], ]; $this->_managedEntities[] = [ - // Setting module to 'civicrm' works for the test but not sure we should actually support that - // as it's probably better to package stuff in a core extension instead of core itself. 'module' => 'civicrm', 'name' => 'testSavedSearch', 'entity' => 'SavedSearch', @@ -226,8 +224,7 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti $this->assertCount(0, $search); // Restore managed entity - $this->_managedEntities = []; - $this->_managedEntities[] = $autoUpdateSearch; + $this->_managedEntities = [$autoUpdateSearch]; CRM_Core_ManagedEntities::singleton(TRUE)->reconcile(); // Entity should be restored @@ -251,8 +248,7 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti // Update packaged version $autoUpdateSearch['params']['values']['description'] = 'New packaged state'; - $this->_managedEntities = []; - $this->_managedEntities[] = $autoUpdateSearch; + $this->_managedEntities = [$autoUpdateSearch]; CRM_Core_ManagedEntities::singleton(TRUE)->reconcile(); // Because the entity was not modified, it will be updated to match the new packaged version @@ -271,8 +267,7 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti // Update packaged version $autoUpdateSearch['params']['values']['description'] = 'Newer packaged state'; - $this->_managedEntities = []; - $this->_managedEntities[] = $autoUpdateSearch; + $this->_managedEntities = [$autoUpdateSearch]; CRM_Core_ManagedEntities::singleton(TRUE)->reconcile(); // Because the entity was modified, it will not be updated @@ -299,6 +294,47 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti $this->assertEquals('civicrm', $search['base_module']); // local_modified_date should be reset by the revert action $this->assertNull($search['local_modified_date']); + + // Delete by user + SavedSearch::delete(FALSE) + ->addWhere('name', '=', 'TestAutoUpdateSavedSearch') + ->execute(); + CRM_Core_ManagedEntities::singleton(TRUE)->reconcile(); + + // Because update policy is 'unmodified' the search won't be re-created + $search = SavedSearch::get(FALSE) + ->addWhere('name', '=', 'TestAutoUpdateSavedSearch') + ->execute(); + $this->assertCount(0, $search); + + // But the managed record is still hanging around with a null entity_id + $managed = Managed::get(FALSE) + ->addWhere('name', '=', 'testAutoUpdate') + ->addWhere('module', '=', 'civicrm') + ->execute(); + $this->assertCount(1, $managed); + $this->assertNull($managed[0]['entity_id']); + $managedId = $managed[0]['id']; + + // Change update policy to 'always' + $autoUpdateSearch['update'] = 'always'; + $this->_managedEntities = [$autoUpdateSearch]; + CRM_Core_ManagedEntities::singleton(TRUE)->reconcile(); + + // Because update policy is 'always' the search will be re-created + $search = SavedSearch::get(FALSE) + ->addWhere('name', '=', 'TestAutoUpdateSavedSearch') + ->execute(); + $this->assertCount(1, $search); + + // But the managed record is still hanging around with a null id + $managed = Managed::get(FALSE) + ->addWhere('name', '=', 'testAutoUpdate') + ->addWhere('module', '=', 'civicrm') + ->execute(); + $this->assertCount(1, $managed); + $this->assertEquals($search[0]['id'], $managed[0]['entity_id']); + $this->assertEquals($managedId, $managed[0]['id']); } /** @@ -656,7 +692,7 @@ class ManagedEntityTest extends TestCase implements HeadlessInterface, Transacti } /** - * Tests a scenario where a record may already exist and we want to make it a managed entity.@dataProvider + * Tests a scenario where a record may already exist and we want to make it a managed entity. * * @throws \CRM_Core_Exception */ diff --git a/xml/schema/Core/Managed.xml b/xml/schema/Core/Managed.xml index f5af8a98ba..9897042cf8 100644 --- a/xml/schema/Core/Managed.xml +++ b/xml/schema/Core/Managed.xml @@ -63,8 +63,7 @@ entity_id Entity ID int unsigned - true - Foreign key to the referenced item. + Soft foreign key to the referenced item. 4.2