Merge pull request #21615 from totten/master-event-test-mail

tests/events/*.php - Enforce general compliance with hook/event signatures 

CRM/Utils/System/UnitTests.php

@@ -34,6 +34,7 @@ class CRM_Utils_System_UnitTests extends CRM_Utils_System_Base {
       $listenerMap = \Civi\Core\Event\EventScanner::findListeners($test);
       \Civi::dispatcher()->addListenerMap($test, $listenerMap);
     }
+    \Civi\Test::eventChecker()->addListeners();
   }
 
   /**

Civi/Test.php

@@ -212,6 +212,16 @@ class Test {
     return $result['data'];
   }
 
+  /**
+   * @return \Civi\Test\EventChecker
+   */
+  public static function eventChecker() {
+    if (!isset(self::$singletons['eventChecker'])) {
+      self::$singletons['eventChecker'] = new \Civi\Test\EventChecker();
+    }
+    return self::$singletons['eventChecker'];
+  }
+
   /**
    * Prepare and execute a batch of SQL statements.
    *

Civi/Test/CiviTestListener.php

@@ -65,9 +65,24 @@ else {
       else {
         $this->tx = NULL;
       }
+
+      if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+        \Civi\Test::eventChecker()->start($test);
+      }
     }
 
     public function endTest(\PHPUnit\Framework\Test $test, $time) {
+      $exception = NULL;
+
+      if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+        try {
+          \Civi\Test::eventChecker()->stop($test);
+        }
+        catch (\Exception $e) {
+          $exception = $e;
+        }
+      }
+
       if ($test instanceof TransactionalInterface) {
         $this->tx->rollback()->commit();
         $this->tx = NULL;
@@ -81,6 +96,10 @@ else {
         error_reporting(E_ALL & ~E_NOTICE);
         $this->errorScope = NULL;
       }
+
+      if ($exception) {
+        throw $exception;
+      }
     }
 
     /**

Civi/Test/CiviTestListenerPHPUnit7.php

@@ -57,9 +57,24 @@ class CiviTestListenerPHPUnit7 implements \PHPUnit\Framework\TestListener {
     else {
       $this->tx = NULL;
     }
+
+    if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+      \Civi\Test::eventChecker()->start($test);
+    }
   }
 
   public function endTest(\PHPUnit\Framework\Test $test, float $time): void {
+    $exception = NULL;
+
+    if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+      try {
+        \Civi\Test::eventChecker()->stop($test);
+      }
+      catch (\Exception $e) {
+        $exception = $e;
+      }
+    }
+
     if ($test instanceof TransactionalInterface) {
       $this->tx->rollback()->commit();
       $this->tx = NULL;
@@ -73,6 +88,10 @@ class CiviTestListenerPHPUnit7 implements \PHPUnit\Framework\TestListener {
       error_reporting(E_ALL & ~E_NOTICE);
       $this->errorScope = NULL;
     }
+
+    if ($exception) {
+      throw $exception;
+    }
   }
 
   /**

Civi/Test/EventCheck.php

+<?php
+/*
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC. All rights reserved.                        |
+ |                                                                    |
+ | This work is published under the GNU AGPLv3 license with some      |
+ | permitted exceptions and without any warranty. For full license    |
+ | and copyright information, see https://civicrm.org/licensing       |
+ +--------------------------------------------------------------------+
+ */
+
+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;
+
+  /**
+   * Determine whether this check should be used during the current test.
+   *
+   * @param \PHPUnit\Framework\Test|NULL $test
+   *
+   * @return bool|string
+   *   FALSE: The check will be completely skipped.
+   *   TRUE: The check will be enabled. However, if the events never
+   *         execute, that is OK. Useful for general compliance-testing.
+   */
+  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.
+   *
+   * @param string|string[] $types
+   *   List of types, per `gettype()` or `get_class()`
+   *   Ex: 'int|string|NULL'
+   *   Ex: [`array`, `NULL`, `CRM_Core_DAO`]
+   * @param mixed $value
+   *   The variable to check
+   * @param string|NULL $msg
+   * @see \CRM_Utils_Type::validatePhpType
+   */
+  public function assertType($types, $value, ?string $msg = NULL) {
+    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));
+    }
+  }
+
+  public function setUp() {
+  }
+
+  public function tearDown() {
+  }
+
+}

Civi/Test/EventChecker.php

+<?php
+/*
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC. All rights reserved.                        |
+ |                                                                    |
+ | This work is published under the GNU AGPLv3 license with some      |
+ | permitted exceptions and without any warranty. For full license    |
+ | and copyright information, see https://civicrm.org/licensing       |
+ +--------------------------------------------------------------------+
+ */
+
+namespace Civi\Test;
+
+use Civi\Core\Event\EventScanner;
+
+class EventChecker {
+
+  /**
+   * @var \Civi\Test\EventCheck[]|null
+   */
+  private $allChecks = NULL;
+
+  /**
+   * @var \Civi\Test\EventCheck[]|null
+   */
+  private $activeChecks = NULL;
+
+  /**
+   * @param \PHPUnit\Framework\Test $test
+   *
+   * @return $this
+   */
+  public function start(\PHPUnit\Framework\Test $test) {
+    if ($this->activeChecks === NULL) {
+      $this->activeChecks = [];
+      foreach ($this->findAll() as $template) {
+        /** @var EventCheck $template */
+        if ($template->isSupported($test)) {
+          $checker = clone $template;
+          $checker->setTest($test);
+          $this->activeChecks[] = $checker;
+          $checker->setUp();
+        }
+      }
+    }
+    return $this;
+  }
+
+  /**
+   * @return $this
+   */
+  public function addListeners() {
+    $d = \Civi::dispatcher();
+    foreach ($this->activeChecks ?: [] as $checker) {
+      /** @var EventCheck $checker */
+      $d->addListenerMap($checker, EventScanner::findListeners($checker));
+      // For the moment, KISS. But we may want a counter at some point - to ensure things actually run.
+      //foreach (EventScanner::findListeners($checker) as $event => $listeners) {
+      //  foreach ($listeners as $listener) {
+      //    $d->addListener($event,
+      //      function($args...) use ($listener) {
+      //        $count++;
+      //        $m = $listener[1];
+      //        $checker->$m(...$args);
+      //      },
+      //      $listener[1] ?? 0
+      //    );
+      //  }
+      //}
+    }
+    return $this;
+  }
+
+  /**
+   * @return $this
+   */
+  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;
+  }
+
+  /**
+   * @return EventCheck[]
+   */
+  protected function findAll() {
+    if ($this->allChecks === NULL) {
+      $all = [];
+      $testDir = \Civi::paths()->getPath('[civicrm.root]/tests/events');
+      $files = \CRM_Utils_File::findFiles($testDir, '*.evch.php', TRUE);
+      sort($files);
+      foreach ($files as $file) {
+        $all[$file] = require $testDir . '/' . $file;
+      }
+      $this->allChecks = $all;
+    }
+
+    return $this->allChecks;
+  }
+
+}

Civi/Test/Legacy/CiviTestListener.php

@@ -55,9 +55,24 @@ class CiviTestListener extends \PHPUnit_Framework_BaseTestListener {
     else {
       $this->tx = NULL;
     }
+
+    if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+      \Civi\Test::eventChecker()->start($test);
+    }
   }
 
   public function endTest(\PHPUnit_Framework_Test $test, $time) {
+    $exception = NULL;
+
+    if ($this->isCiviTest($test) || $test instanceof \CiviUnitTestCase) {
+      try {
+        \Civi\Test::eventChecker()->stop($test);
+      }
+      catch (\Exception $e) {
+        $exception = $e;
+      }
+    }
+
     if ($test instanceof \Civi\Test\TransactionalInterface) {
       $this->tx->rollback()->commit();
       $this->tx = NULL;
@@ -71,6 +86,10 @@ class CiviTestListener extends \PHPUnit_Framework_BaseTestListener {
       error_reporting(E_ALL & ~E_NOTICE);
       $this->errorScope = NULL;
     }
+
+    if ($exception) {
+      throw $exception;
+    }
   }
 
   /**

tests/events/civi_region_render.evch.php

+<?php
+return new class() extends \Civi\Test\EventCheck implements \Civi\Test\HookInterface {
+
+  private $validSnippetTypes = [
+    'callback',
+    'jquery',
+    'markup',
+    'script',
+    'scriptFile',
+    'scriptUrl',
+    'settings',
+    'style',
+    'styleFile',
+    'styleUrl',
+    'template',
+  ];
+
+  private $validRegion = '/^[A-Za-z0-9\\-]+$/';
+
+  /**
+   * Ensure that the hook data is always well-formed.
+   */
+  public function on_civi_region_render(\Civi\Core\Event\GenericHookEvent $e) {
+    $this->assertTrue($e->region instanceof \CRM_Core_Region);
+    /** @var \CRM_Core_Region $region */
+    $region = $e->region;
+    $this->assertRegexp($this->validRegion, $region->_name);
+    foreach ($region->getAll() as $snippet) {
+      $this->assertContains($snippet['type'], $this->validSnippetTypes);
+    }
+  }
+
+};

tests/events/hook_civicrm_alterMailParams.evch.php

+<?php
+return new class() extends \Civi\Test\EventCheck implements \Civi\Test\HookInterface {
+
+  private $paramSpecs = [
+
+    // ## Envelope: Common
+
+    'toName' => ['type' => 'string|NULL'],
+    'toEmail' => ['type' => 'string|NULL'],
+    'cc' => ['type' => 'string|NULL'],
+    'bcc' => ['type' => 'string|NULL'],
+    'headers' => ['type' => 'array'],
+    'attachments' => ['type' => 'array|NULL'],
+    'isTest' => ['type' => 'bool|int'],
+
+    // ## Envelope: singleEmail/messageTemplate
+
+    'from' => ['type' => 'string|NULL', 'for' => ['messageTemplate', 'singleEmail']],
+    'replyTo' => ['type' => 'string|NULL', 'for' => ['messageTemplate', 'singleEmail']],
+    'returnPath' => ['type' => 'string|NULL', 'for' => ['messageTemplate', 'singleEmail']],
+    'isEmailPdf' => ['type' => 'bool', 'for' => 'messageTemplate'],
+    'PDFFilename' => ['type' => 'string|NULL', 'for' => 'messageTemplate'],
+    'autoSubmitted' => ['type' => 'bool', 'for' => 'messageTemplate'],
+    'Message-ID' => ['type' => 'string', 'for' => ['messageTemplate', 'singleEmail']],
+    'messageId' => ['type' => 'string', 'for' => ['messageTemplate', 'singleEmail']],
+
+    // ## Envelope: CiviMail/Flexmailer
+
+    'Reply-To' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'Return-Path' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'From' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'Subject' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'List-Unsubscribe' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'X-CiviMail-Bounce' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer']],
+    'Precedence' => ['type' => 'string|NULL', 'for' => ['civimail', 'flexmailer'], 'regex' => '/(bulk|first-class|list)/'],
+    'job_id' => ['type' => 'int|NULL', 'for' => ['civimail', 'flexmailer']],
+
+    // ## Content
+
+    'subject' => ['for' => ['messageTemplate', 'singleEmail'], 'type' => 'string'],
+    'text' => ['type' => 'string|NULL'],
+    'html' => ['type' => 'string|NULL'],
+
+    // ## Model: messageTemplate
+
+    'tokenContext' => ['type' => 'array', 'for' => 'messageTemplate'],
+    'tplParams' => ['type' => 'array', 'for' => 'messageTemplate'],
+    'contactId' => ['type' => 'int|NULL', 'for' => 'messageTemplate' /* deprecated in favor of tokenContext[contactId] */],
+    'valueName' => [
+      'regex' => '/^([a-zA-Z_]+)$/',
+      'type' => 'string',
+      'for' => 'messageTemplate',
+    ],
+    'groupName' => [
+      // This field is generally deprecated. Historically, this was tied to various option-groups (`msg_*`),
+      // but it also seems to have been used with a few long-form English names.
+      'regex' => '/^(msg_[a-zA-Z_]+|Scheduled Reminder Sender|Activity Email Sender|Report Email Sender|Mailing Event Welcome|CRM_Core_Config_MailerTest)$/',
+      'type' => 'string',
+      'for' => ['messageTemplate', 'singleEmail'],
+    ],
+
+    // The model is not passed into this hook because it would create ambiguity when you alter properties.
+    // If you want to expose it via hook, add another hook.
+    'model' => ['for' => 'messageTemplate', 'type' => 'NULL'],
+    'modelProps' => ['for' => 'messageTemplate', 'type' => 'NULL'],
+
+    // ## Model: Adhoc/incomplete/needs attention
+
+    'contributionId' => ['type' => 'int', 'for' => 'messageTemplate'],
+    'petitionId' => ['type' => 'int', 'for' => 'messageTemplate'],
+    'petitionTitle' => ['type' => 'string', 'for' => 'messageTemplate'],
+    'table' => ['type' => 'string', 'for' => 'messageTemplate', 'regex' => '/civicrm_msg_template/'],
+    'entity' => ['type' => 'string|NULL', 'for' => 'singleEmail'],
+    'entity_id' => ['type' => 'int|NULL', 'for' => 'singleEmail'],
+
+    // ## View: messageTemplate
+
+    'messageTemplateID' => ['type' => 'int|NULL', 'for' => 'messageTemplate'],
+    'messageTemplate' => ['type' => 'array|NULL', 'for' => 'messageTemplate'],
+    'disableSmarty' => ['type' => 'bool|int', 'for' => 'messageTemplate'],
+  ];
+
+  public function isSupported($test) {
+    // MailTest does intentionally breaky things to provoke+ensure decent error-handling.
+    //So we will not enforce generic rules on it.
+    return !($test instanceof CRM_Utils_MailTest);
+  }
+
+  /**
+   * Ensure that the hook data is always well-formed.
+   *
+   * @see \CRM_Utils_Hook::alterMailParams()
+   */
+  public function hook_civicrm_alterMailParams(&$params, $context = NULL) {
+    $msg = "Non-conformant hook_civicrm_alterMailParams(..., $context)";
+    $dump = print_r($params, 1);
+
+    $this->assertRegExp('/^(messageTemplate|civimail|singleEmail|flexmailer)$/',
+      $context, "$msg: Unrecognized context ($context)\n$dump");
+
+    $contexts = [$context];
+    if ($context === 'singleEmail' && array_key_exists('tokenContext', $params)) {
+      // Don't look now, but `sendTemplate()` fires this hook twice for the message! Once with $context=messageTemplate; again with $context=singleEmail.
+      $contexts[] = 'messageTemplate';
+    }
+
+    $paramSpecs = array_filter($this->paramSpecs, function ($f) use ($contexts) {
+      return !isset($f['for']) || array_intersect((array) $f['for'], $contexts);
+    });
+
+    $unknownKeys = array_diff(array_keys($params), array_keys($paramSpecs));
+    if ($unknownKeys !== []) {
+      echo '';
+    }
+    $this->assertEquals([], $unknownKeys, "$msg: Unrecognized keys: " . implode(', ', $unknownKeys) . "\n$dump");
+
+    foreach ($params as $key => $value) {
+      $this->assertType($paramSpecs[$key]['type'], $value, "$msg: Bad data-type found in param ($key)\n$dump");
+      if (isset($paramSpecs[$key]['regex'])) {
+        $this->assertRegExp($paramSpecs[$key]['regex'], $value, "Parameter [$key => $value] should match regex ({$paramSpecs[$key]['regex']})");
+      }
+    }
+
+    if ($context === 'messageTemplate') {
+      $this->assertTrue(!empty($params['valueName']), "$msg: Message templates must always specify the name of the workflow step\n$dump");
+      $this->assertEquals($params['contactId'] ?? NULL, $params['tokenContext']['contactId'] ?? NULL, "$msg: contactId moved to tokenContext, but legacy value should be equivalent\n$dump");
+
+      // This assertion is surprising -- yet true. We should perhaps check if it was true in past releases...
+      $this->assertTrue(empty($params['text']) && empty($params['html']) && empty($params['subject']), "$msg: Content is not given if context==messageTemplate\n$dump");
+    }
+
+    if ($context !== 'messageTemplate') {
+      $this->assertTrue(!empty($params['text']) || !empty($params['html']) || !empty($params['subject']), "$msg: Must provide at least one of: text, html, subject\n$dump");
+    }
+
+    if (isset($params['groupName']) && $params['groupName'] === 'Scheduled Reminder Sender') {
+      $this->assertTrue(!empty($params['entity']), "$msg: Scheduled reminders should have entity\n$dump");
+      $this->assertTrue(!empty($params['entity_id']), "$msg: Scheduled reminders should have entity_id\n$dump");
+    }
+  }
+
+};