Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ManagedEntities - Allow targeted reconciliation. Add hook parameter. Simplify logic. #22959

Merged
merged 3 commits into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
355 changes: 125 additions & 230 deletions CRM/Core/ManagedEntities.php

Large diffs are not rendered by default.

32 changes: 29 additions & 3 deletions CRM/Utils/Hook.php
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,21 @@ public static function activeTheme(&$theme, $context) {
/**
* This hook is called for declaring managed entities via API.
*
* @code
* // Example: Optimal skeleton for backward/forward compatibility
* function example_civicrm_managed(&$entities, ?array $modules = NULL) {
* if ($modules !== NULL && !in_array(E::LONG_NAME, $modules, TRUE)) {
* return;
* }
* $entities[] = [
* 'module' => E::LONG_NAME,
* 'name' => 'my_option_value',
* 'entity' => 'OptionValue',
* 'params' => [...],
* ];
* }
* @endCode
*
* @param array $entities
* List of pending entities. Each entity is an array with keys:
* + 'module': string; for module-extensions, this is the fully-qualifed name (e.g. "com.example.mymodule"); for CMS modules, the name is prefixed by the CMS (e.g. "drupal.mymodule")
Expand All @@ -715,15 +730,26 @@ public static function activeTheme(&$theme, $context) {
* - 'always' (default): always delete orphaned records
* - 'never': never delete orphaned records
* - 'unused': only delete orphaned records if there are no other references to it in the DB. (This is determined by calling the API's "getrefcount" action.)
* @param array|NULL $modules
* (Added circa v5.50) If given, only report entities related to $modules. NULL is a wildcard ("all modules").
*
* This parameter is _advisory_ and is not supplied on older versions.
* Listeners SHOULD self-censor (only report entities which match the filter).
* However, all pre-existing listeners were unaware of this option, and they WILL over-report.
* Over-reported data will be discarded.
* @return null
* the return value is ignored
*/
public static function managed(&$entities) {
return self::singleton()->invoke(['entities'], $entities,
self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject,
public static function managed(&$entities, ?array $modules = NULL) {
self::singleton()->invoke(['entities', 'modules'], $entities, $modules,
self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject,
'civicrm_managed'
);
if ($modules) {
$entities = array_filter($entities, function($entity) use ($modules) {
return in_array($entity['module'], $modules, TRUE);
});
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions ext/afform/core/Civi/Api4/Action/Afform/Revert.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Civi\Api4\Action\Afform;

use Civi\Api4\Generic\Result;
use CRM_Afform_ExtensionUtil as E;

/**
* @inheritDoc
Expand Down Expand Up @@ -32,8 +33,7 @@ protected function processBatch(Result $result, array $items) {
_afform_clear();

if ($this->flushManaged) {
// FIXME: more targeted reconciliation
\CRM_Core_ManagedEntities::singleton()->reconcile();
\CRM_Core_ManagedEntities::singleton()->reconcile(E::LONG_NAME);
}
if ($this->flushMenu) {
\CRM_Core_Menu::store();
Expand Down
5 changes: 3 additions & 2 deletions ext/afform/core/Civi/Api4/Utils/AfformSaveTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Civi\Api4\Utils;

use CRM_Afform_ExtensionUtil as E;

/**
* Class AfformSaveTrait
* @package Civi\Api4\Action\Afform
Expand Down Expand Up @@ -70,8 +72,7 @@ protected function writeRecord($item) {
$isChanged('is_dashlet') ||
(!empty($meta['is_dashlet']) && $isChanged('title'))
) {
// FIXME: more targeted reconciliation
\CRM_Core_ManagedEntities::singleton()->reconcile();
\CRM_Core_ManagedEntities::singleton()->reconcile(E::LONG_NAME);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten this FIXME has been staring me down for a long time. Finally fixed!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot! That should help performance when editing afforms!

}

// Right now, permission-checks are completely on-demand.
Expand Down
5 changes: 4 additions & 1 deletion ext/afform/core/afform.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ function afform_civicrm_upgrade($op, CRM_Queue_Queue $queue = NULL) {
*
* @link http://wiki.civicrm.org/confluence/display/CRMDOC/hook_civicrm_managed
*/
function afform_civicrm_managed(&$entities) {
function afform_civicrm_managed(&$entities, $modules) {
if ($modules && !in_array(E::LONG_NAME, $modules, TRUE)) {
return;
}
/** @var \CRM_Afform_AfformScanner $scanner */
if (\Civi::container()->has('afform_scanner')) {
$scanner = \Civi::service('afform_scanner');
Expand Down
4 changes: 4 additions & 0 deletions mixin/mgd-php@1/mixin.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@
return;
}

if (is_array($event->modules) && !in_array($mixInfo->longName, $event->modules, TRUE)) {
return;
}

$mgdFiles = CRM_Utils_File::findFiles($mixInfo->getPath(), '*.mgd.php');
sort($mgdFiles);
foreach ($mgdFiles as $file) {
Expand Down
55 changes: 55 additions & 0 deletions tests/phpunit/CRM/Core/ManagedEntitiesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,28 @@ public function setUp(): void {
'update' => 'never',
],
];
$this->fixtures['com.example.two-CustomGroup'] = [
'module' => 'com.example.two',
'name' => 'CustomGroup',
'entity' => 'CustomGroup',
'params' => [
'version' => 3,
'name' => 'test_custom_group_two',
'title' => 'Test custom group two',
'extends' => 'Individual',
],
];
$this->fixtures['com.example.three-CustomGroup'] = [
'module' => 'com.example.three',
'name' => 'CustomGroup',
'entity' => 'CustomGroup',
'params' => [
'version' => 3,
'name' => 'test_custom_group_three',
'title' => 'Test custom group three',
'extends' => 'Individual',
],
];

$this->apiKernel = \Civi::service('civi_api_kernel');
$this->adhocProvider = new \Civi\API\Provider\AdhocProvider(3, 'CustomSearch');
Expand Down Expand Up @@ -581,4 +603,37 @@ public function testDependentEntitiesUninstallCleanly(): void {

}

/**
* The hook_managed signature expanded slightly (adding the `$modules` filter).
* Pre-existing implementations may over-report (ie return entities despite the `$modules` filter).
* This test ensures that the framework respects the `$modules` filter (even if specific implementations don't).
*/
public function testHookManaged_FilterModule() {
$this->managedEntities = [
$this->fixtures['com.example.one-bar'],
$this->fixtures['com.example.two-CustomGroup'],
$this->fixtures['com.example.three-CustomGroup'],
];

$entitiesAll = [];
CRM_Utils_Hook::managed($entitiesAll);
$this->assertEquals($this->managedEntities, $entitiesAll);
$this->assertEquals(3, count($entitiesAll));

$entitiesTwoOnly = [];
CRM_Utils_Hook::managed($entitiesTwoOnly, ['com.example.two']);
$this->assertEquals([$this->fixtures['com.example.two-CustomGroup']], array_values($entitiesTwoOnly));
$this->assertEquals(1, count($entitiesTwoOnly));

$entitiesTwoExtra = [];
CRM_Utils_Hook::managed($entitiesTwoExtra, ['com.example.two', 'com.example.extra']);
$this->assertEquals([$this->fixtures['com.example.two-CustomGroup']], array_values($entitiesTwoExtra));
$this->assertEquals(1, count($entitiesTwoExtra));

$entitiesTwoThree = [];
CRM_Utils_Hook::managed($entitiesTwoThree, ['com.example.two', 'com.example.three']);
$this->assertEquals([$this->fixtures['com.example.two-CustomGroup'], $this->fixtures['com.example.three-CustomGroup']], array_values($entitiesTwoThree));
$this->assertEquals(2, count($entitiesTwoThree));
}

}
8 changes: 6 additions & 2 deletions tests/phpunit/api/v4/Entity/ManagedEntityTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ public function testExportOptionGroupWithDomain() {
}

public function testManagedNavigationWeights() {
$this->_managedEntities = [
$managedEntities = [
[
'module' => 'unit.test.fake.ext',
'name' => 'Navigation_Test_Parent',
Expand Down Expand Up @@ -474,13 +474,14 @@ public function testManagedNavigationWeights() {
],
],
];
$this->_managedEntities = $managedEntities;

// Throw a monkey wrench by placing duplicates in another domain
$d2 = Domain::create(FALSE)
->addValue('name', 'Decoy domain')
->addValue('version', \CRM_Utils_System::version())
->execute()->single();
foreach ($this->_managedEntities as $item) {
foreach ($managedEntities as $item) {
$decoys[] = civicrm_api4('Navigation', 'create', [
'checkPermissions' => FALSE,
'values' => ['domain_id' => $d2['id']] + $item['params']['values'],
Expand Down Expand Up @@ -535,6 +536,8 @@ public function testManagedNavigationWeights() {
$allModules = [
new \CRM_Core_Module('unit.test.fake.ext', FALSE),
];
// If module is disabled it will not run hook_civicrm_managed.
$this->_managedEntities = [];
(new \CRM_Core_ManagedEntities($allModules))->reconcile();

// Children's weight should have been unaffected, but they should be disabled
Expand All @@ -556,6 +559,7 @@ public function testManagedNavigationWeights() {
$allModules = [
new \CRM_Core_Module('unit.test.fake.ext', TRUE),
];
$this->_managedEntities = $managedEntities;
(new \CRM_Core_ManagedEntities($allModules))->reconcile();

// Children's weight should have been unaffected, but they should be enabled
Expand Down