Angular - Noisily deprecate 'settings' in favor of 'settingsFactory'

civicrm · Oct 13, 2023 · 41ce741 · 41ce741
1 parent 7a915c4
commit 41ce741
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 28 deletions.
diff --git a/CRM/Utils/Check/Component/Env.php b/CRM/Utils/Check/Component/Env.php
@@ -1106,4 +1106,25 @@ public function checkPHPIntlExists() {
     return $messages;
   }
 
+  public function checkAngularModuleSettings() {
+    $messages = [];
+    $modules = Civi::container()->get('angular')->getModules();
+    foreach ($modules as $name => $module) {
+      if (!empty($module['settings'])) {
+        $messages[] = new CRM_Utils_Check_Message(
+          __FUNCTION__ . $name,
+          ts('The Angular file "%1" from extension "%2" must be updated to use "settingsFactory" instead of "settings". <a %3>Developer info...</a>', [
+            1 => $name,
+            2 => $module['ext'],
+            3 => 'target="_blank" href="https://github.com/civicrm/civicrm-core/pull/19052"',
+          ]),
+          ts('Unsupported Angular Setting'),
+          \Psr\Log\LogLevel::WARNING,
+          'fa-code'
+        );
+      }
+    }
+    return $messages;
+  }
+
 }
diff --git a/Civi/Angular/Manager.php b/Civi/Angular/Manager.php
@@ -135,6 +135,9 @@ public function getModules() {
       foreach ($angularModules as $module => $info) {
         // Merge in defaults
         $angularModules[$module] += ['basePages' => ['civicrm/a']];
+        if (!empty($info['settings'])) {
+          \CRM_Core_Error::deprecatedWarning('Angular "settings" is not supported. See https://github.com/civicrm/civicrm-core/pull/19052');
+        }
         // Validate settingsFactory callables
         if (isset($info['settingsFactory'])) {
           // To keep the cache small, we want `settingsFactory` to contain the string names of class & function, not an object

diff --git a/tests/phpunit/Civi/Angular/LoaderTest.php b/tests/phpunit/Civi/Angular/LoaderTest.php
@@ -16,22 +16,20 @@
  */
 class LoaderTest extends \CiviUnitTestCase {
 
-  public static $dummy_setting_count = 0;
   public static $dummy_callback_count = 0;
 
   public function setUp(): void {
     parent::setUp();
     $this->hookClass->setHook('civicrm_angularModules', [$this, 'hook_angularModules']);
-    self::$dummy_setting_count = 0;
     self::$dummy_callback_count = 0;
     $this->createLoggedInUser();
   }
 
   public function factoryScenarios() {
     return [
-      ['dummy1', 2, 1, ['access CiviCRM', 'administer CiviCRM']],
-      ['dummy2', 2, 0, []],
-      ['dummy3', 2, 2, ['access CiviCRM', 'administer CiviCRM', 'view debug output']],
+      ['dummy1', 1, ['access CiviCRM', 'administer CiviCRM']],
+      ['dummy2', 0, []],
+      ['dummy3', 2, ['access CiviCRM', 'administer CiviCRM', 'view debug output']],
     ];
   }
 
@@ -41,11 +39,10 @@ public function factoryScenarios() {
    *
    * @dataProvider factoryScenarios
    * @param $module
-   * @param $expectedSettingCount
    * @param $expectedCallbackCount
    * @param $expectedPermissions
    */
-  public function testSettingFactory($module, $expectedSettingCount, $expectedCallbackCount, $expectedPermissions) {
+  public function testSettingFactory($module, $expectedCallbackCount, $expectedPermissions) {
     $loader = \Civi::service('angularjs.loader');
     $loader->addModules([$module]);
     $loader->useApp();
@@ -65,29 +62,21 @@ public function testSettingFactory($module, $expectedSettingCount, $expectedCall
     // Dummy3 module's factory setting should be set if it is loaded directly
     $this->assertTrue(($expectedCallbackCount > 1) === isset($actual['dummy3']['dummy_setting_factory']));
 
-    // Dummy1 module's regular setting should be set if it is loaded directly or required by dummy3
-    $this->assertTrue(($module !== 'dummy2') === isset($actual['dummy1']['dummy_setting']));
-    // Dummy2 module's regular setting should be set if loaded
-    $this->assertTrue(($module === 'dummy2') === isset($actual['dummy2']['dummy_setting']));
-
     // Assert appropriate permissions have been added
     $this->assertEquals($expectedPermissions, array_keys($actual['permissions']));
 
-    // Assert the callback functions ran the expected number of times
-    $this->assertEquals($expectedSettingCount, self::$dummy_setting_count);
+    // Assert the callback function ran the expected number of times
     $this->assertEquals($expectedCallbackCount, self::$dummy_callback_count);
   }
 
   public function hook_angularModules(&$modules) {
     $modules['dummy1'] = [
       'ext' => 'civicrm',
-      'settings' => $this->getDummySetting(),
       'permissions' => ['access CiviCRM', 'administer CiviCRM'],
       'settingsFactory' => [self::class, 'getDummySettingFactory'],
     ];
     $modules['dummy2'] = [
       'ext' => 'civicrm',
-      'settings' => $this->getDummySetting(),
     ];
     $modules['dummy3'] = [
       'ext' => 'civicrm',
@@ -99,10 +88,6 @@ public function hook_angularModules(&$modules) {
     ];
   }
 
-  public function getDummySetting() {
-    return ['dummy_setting' => self::$dummy_setting_count++];
-  }
-
   public static function getDummySettingFactory() {
     return ['dummy_setting_factory' => self::$dummy_callback_count++];
   }

diff --git a/tests/phpunit/Civi/Angular/ManagerTest.php b/tests/phpunit/Civi/Angular/ManagerTest.php
@@ -47,7 +47,6 @@ public function testGetModules(): void {
       'js' => 0,
       'css' => 0,
       'partials' => 0,
-      'settings' => 0,
       'settingsFactory' => 0,
     ];
 
@@ -75,12 +74,7 @@ public function testGetModules(): void {
           $counts['partials']++;
         }
       }
-      if (isset($module['settings'])) {
-        $this->assertTrue(is_array($module['settings']));
-        foreach ($module['settings'] as $name => $value) {
-          $counts['settings']++;
-        }
-      }
+      $this->assertArrayNotHasKey('settings', $module);
       if (isset($module['settingsFactory'])) {
         $this->assertTrue(is_callable($module['settingsFactory']));
         $counts['settingsFactory']++;
@@ -91,7 +85,6 @@ public function testGetModules(): void {
     $this->assertTrue($counts['css'] > 0, 'Expect to find at least one CSS file');
     $this->assertTrue($counts['partials'] > 0, 'Expect to find at least one partial HTML file');
     $this->assertTrue($counts['settingsFactory'] > 0, 'Expect to find at least one settingsFactory');
-    $this->assertEquals(0, $counts['settings'], 'Angular settings are deprecated in favor of settingsFactory');
   }
 
   /**