Merge pull request #28723 from totten/master-strict-hook

tests/events/*.php - Allow optional runtime enforcement

Civi/Test/EventCheck.php

@@ -11,18 +11,11 @@
 
 namespace Civi\Test;
 
-use PHPUnit\Framework\Assert;
-
 /**
  * An EventCheck is a fragment of a unit-test -- it is mixed into
  * various test-scenarios and applies extra assertions.
  */
-class EventCheck extends Assert {
-
-  /**
-   * @var \PHPUnit\Framework\Test
-   */
-  private $test;
+class EventCheck {
 
   /**
    * Determine whether this check should be used during the current test.
@@ -38,20 +31,6 @@ public function isSupported($test) {
     return TRUE;
   }
 
-  /**
-   * @return \PHPUnit\Framework\Test|NULL
-   */
-  public function getTest() {
-    return $this->test;
-  }
-
-  /**
-   * @param \PHPUnit\Framework\Test|null $test
-   */
-  public function setTest($test): void {
-    $this->test = $test;
-  }
-
   /**
    * Assert that a variable has a given type.
    *
@@ -61,21 +40,56 @@ public function setTest($test): void {
    *   Ex: [`array`, `NULL`, `CRM_Core_DAO`]
    * @param mixed $value
    *   The variable to check
-   * @param string|null $msg
+   * @param string|null $message
    * @see \CRM_Utils_Type::validatePhpType
    */
-  public function assertType($types, $value, ?string $msg = NULL) {
+  public function assertType($types, $value, string $message = '') {
     if (!\CRM_Utils_Type::validatePhpType($value, $types, FALSE)) {
       $defactoType = is_object($value) ? get_class($value) : gettype($value);
       $types = is_array($types) ? implode('|', $types) : $types;
-      $this->fail(sprintf("Expected one of (%s) but found %s\n%s", $types, $defactoType, $msg));
+      $comment = sprintf('Expected one of (%s) but found %s', $types, $defactoType);
+      static::fail(trim("$message\n$comment"));
     }
   }
 
-  public function setUp() {
+  public static function assertEquals($expected, $actual, string $message = ''): void {
+    if ($expected !== $actual) {
+      $comment = sprintf('Expected value %s and actual value %s should match.', json_encode($expected), json_encode($actual));
+      static::fail(trim("$message\n$comment"));
+    }
+  }
+
+  public static function assertContains($needle, iterable $haystack, string $message = ''): void {
+    $haystack = ($haystack instanceof \Traversable) ? iterator_to_array($haystack) : (array) $haystack;
+    if (!in_array($needle, $haystack)) {
+      $comment = sprintf('Item %s not found in list: %s', json_encode($needle), json_encode($haystack));
+      static::fail(trim("$message\n$comment"));
+    }
+  }
+
+  public static function assertMatchesRegularExpression(string $pattern, string $string, string $message = ''): void {
+    if (!preg_match($pattern, $string)) {
+      $comment = sprintf('Value %s does not match pattern %s.', json_encode($string), json_encode($pattern));
+      static::fail(trim("$message\n$comment"));
+    }
+  }
+
+  public static function assertNotEmpty($actual, string $message = ''): void {
+    if (empty($actual)) {
+      $comment = 'Value should not be empty.';
+      static::fail(trim("$message\n$comment"));
+    }
+  }
+
+  public static function assertTrue($condition, string $message = ''): void {
+    if ($condition !== TRUE) {
+      $comment = 'Value should be TRUE.';
+      static::fail(trim("$message\n$comment"));
+    }
   }
 
-  public function tearDown() {
+  public static function fail(string $message = ''): void {
+    throw new EventCheckException($message);
   }
 
 }

Civi/Test/EventCheckException.php

@@ -0,0 +1,24 @@
+<?php
+
+namespace Civi\Test;
+
+/**
+ * The "event checks" ensure that hooks/events satisfy some expected contracts.
+ * This exception is emitted if there is a non-conformant hook/event.
+ * It should only be emitted on development environments.
+ */
+
+if (class_exists('PHPUnit\Framework\AssertionFailedError')) {
+
+  class EventCheckException extends \PHPUnit\Framework\AssertionFailedError {
+
+  }
+
+}
+else {
+
+  class EventCheckException extends \RuntimeException {
+
+  }
+
+}

Civi/Test/EventChecker.php

@@ -26,7 +26,7 @@ class EventChecker {
   private $activeChecks = NULL;
 
   /**
-   * @param \PHPUnit\Framework\Test $test
+   * @param \PHPUnit\Framework\Test|NULL $test
    *
    * @return $this
    */
@@ -37,9 +37,7 @@ public function start($test) {
         /** @var EventCheck $template */
         if ($template->isSupported($test)) {
           $checker = clone $template;
-          $checker->setTest($test);
           $this->activeChecks[] = $checker;
-          $checker->setUp();
         }
       }
     }
@@ -76,11 +74,6 @@ public function addListeners() {
    */
   public function stop() {
     // NOTE: In test environment, dispatcher will be removed regardless.
-    foreach ($this->activeChecks ?? [] as $checker) {
-      /** @var \Civi\Test\EventCheck $checker */
-      Invasive::call([$checker, 'tearDown']);
-      $checker->setTest(NULL);
-    }
     $this->activeChecks = NULL;
     return $this;
   }

tests/events/hook_civicrm_links.evch.php

@@ -5,7 +5,7 @@
 
 return new class() extends EventCheck implements HookInterface {
 
-  // There are several properties named "$grandfatherdXyz". These mark itmes which
+  // There are several properties named "$grandfatherdXyz". These mark items which
   // pass through hook_civicrm_links but deviate from the plain interpretation of the docs.
   // Auditing or cleaning each would be its own separate project. Please feel free to
   // do that audit and figure how to normalize it. But for now, the goal of this file is
@@ -69,6 +69,16 @@
     'pcp.user.actions::Pcp',
   ];
 
+  /**
+   * List of events with multiple problems. These are completely ignored.
+   *
+   * @var string[]
+   */
+  protected $unrepentantMiscreants = [
+    'create.new.shortcuts', /* FIXME */
+    'create.new.shorcuts', /* Deprecated */
+  ];
+
   /**
    * Ensure that the hook data is always well-formed.
    *
@@ -78,6 +88,10 @@ public function hook_civicrm_links($op, $objectName, &$objectId, &$links, &$mask
     // fprintf(STDERR, "CHECK hook_civicrm_links($op)\n");
     $msg = sprintf('Non-conforming hook_civicrm_links(%s, %s)', json_encode($op), json_encode($objectName));
 
+    if (in_array($op, $this->unrepentantMiscreants)) {
+      return;
+    }
+
     $this->assertTrue((bool) preg_match(';^\w+(\.\w+)+$;', $op), "$msg: Operation ($op) should be dotted expression");
     $this->assertTrue((bool) preg_match(';^[A-Z][a-zA-Z0-9]+$;', $objectName) || in_array($objectName, $this->grandfatheredObjectNames),
       "$msg: Object name ($objectName) should be a CamelCase name or a grandfathered name");