From 9cf68fccef6d367417998eed366c8650317aea79 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Wed, 8 Sep 2021 14:32:43 +1200
Subject: [PATCH 1/2] dev/core#2823 Move validation into validation function

As the code comments suggest the handling of a module being unrecognised sould
be handled in the validate not the enable function
---
 CRM/Core/ManagedEntities.php | 49 ++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/CRM/Core/ManagedEntities.php b/CRM/Core/ManagedEntities.php
index eff92beeb252..0d77755664fd 100644
--- a/CRM/Core/ManagedEntities.php
+++ b/CRM/Core/ManagedEntities.php
@@ -142,15 +142,9 @@ public function reconcileEnabledModules() {
     // index by moduleName,name
     $decls = $this->createDeclarationIndex($this->moduleIndex, $this->getDeclarations());
     foreach ($decls as $moduleName => $todos) {
-      if (isset($this->moduleIndex[TRUE][$moduleName])) {
+      if ($this->isModuleEnabled($moduleName)) {
         $this->reconcileEnabledModule($this->moduleIndex[TRUE][$moduleName], $todos);
       }
-      elseif (isset($this->moduleIndex[FALSE][$moduleName])) {
-        // do nothing -- module should get swept up later
-      }
-      else {
-        throw new Exception("Entity declaration references invalid or inactive module name [$moduleName]");
-      }
     }
   }
 
@@ -443,18 +437,53 @@ protected function createDeclarationIndex($moduleIndex, $declarations) {
    *   string on error, or FALSE
    */
   protected function validate($declarations) {
-    foreach ($declarations as $declare) {
+    foreach ($declarations as $module => $declare) {
       foreach (['name', 'module', 'entity', 'params'] as $key) {
         if (empty($declare[$key])) {
           $str = print_r($declare, TRUE);
-          return ("Managed Entity is missing field \"$key\": $str");
+          return ts('Managed Entity (%1) is missing field "%2": %3', [$module, $key, $str]);
         }
       }
-      // FIXME: validate that each 'module' is known
+      if (!$this->isModuleRecognised($declare['module'])) {
+        return ts('Entity declaration references invalid or inactive module name [%1]', [$declare['module']]);
+      }
     }
     return FALSE;
   }
 
+  /**
+   * Is the module recognised (as an enabled or disabled extension in the system).
+   *
+   * @param string $module
+   *
+   * @return bool
+   */
+  protected function isModuleRecognised(string $module): bool {
+    return $this->isModuleDisabled($module) || $this->isModuleEnabled($module);
+  }
+
+  /**
+   * Is the module enabled.
+   *
+   * @param string $module
+   *
+   * @return bool
+   */
+  protected function isModuleEnabled(string $module): bool {
+    return isset($this->moduleIndex[TRUE][$module]);
+  }
+
+  /**
+   * Is the module disabled.
+   *
+   * @param string $module
+   *
+   * @return bool
+   */
+  protected function isModuleDisabled(string $module): bool {
+    return isset($this->moduleIndex[FALSE][$module]);
+  }
+
   /**
    * @param array $declarations
    *

From 78ce6ebbbf1410d55f70b2603a7c853b527403d0 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Wed, 8 Sep 2021 14:10:28 +1200
Subject: [PATCH 2/2] dev/core#2823 Extract code to load the declarations and
 call from the constructor

The declarations are only used in object context so it makes sense to load
them in the constructor. In addition they are ALWAYS loaded except in test usage
(it does seem a bit silly having the option to pass them in only for tests
but we can ignore that for now - I commented it)
---
 CRM/Core/ManagedEntities.php | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/CRM/Core/ManagedEntities.php b/CRM/Core/ManagedEntities.php
index 0d77755664fd..b7d44b35a635 100644
--- a/CRM/Core/ManagedEntities.php
+++ b/CRM/Core/ManagedEntities.php
@@ -64,7 +64,8 @@ function () {
    * @param array $modules
    *   CRM_Core_Module.
    * @param array $declarations
-   *   Per hook_civicrm_managed.
+   *   Per hook_civicrm_managed. Only ever passed in in unit tests - otherwise
+   *   calculated.
    */
   public function __construct($modules, $declarations) {
     $this->moduleIndex = $this->createModuleIndex($modules);
@@ -73,7 +74,7 @@ public function __construct($modules, $declarations) {
       $this->declarations = $this->cleanDeclarations($declarations);
     }
     else {
-      $this->declarations = NULL;
+      $this->loadDeclarations();
     }
   }
 
@@ -379,15 +380,6 @@ protected function removeStaleEntity($dao) {
    * @return array|null
    */
   protected function getDeclarations() {
-    if ($this->declarations === NULL) {
-      $this->declarations = [];
-      foreach (CRM_Core_Component::getEnabledComponents() as $component) {
-        /** @var CRM_Core_Component_Info $component */
-        $this->declarations = array_merge($this->declarations, $component->getManagedEntities());
-      }
-      CRM_Utils_Hook::managed($this->declarations);
-      $this->declarations = $this->cleanDeclarations($this->declarations);
-    }
     return $this->declarations;
   }
 
@@ -537,4 +529,18 @@ private function isActivationSupported(string $entity_type): bool {
     return Civi::$statics[__CLASS__][__FUNCTION__][$entity_type];
   }
 
+  /**
+   * Load declarations into the class property.
+   *
+   * This picks it up from hooks and enabled components.
+   */
+  protected function loadDeclarations(): void {
+    $this->declarations = [];
+    foreach (CRM_Core_Component::getEnabledComponents() as $component) {
+      $this->declarations = array_merge($this->declarations, $component->getManagedEntities());
+    }
+    CRM_Utils_Hook::managed($this->declarations);
+    $this->declarations = $this->cleanDeclarations($this->declarations);
+  }
+
 }