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

[totten]

Overview

Introduce a suite of tests (tests/events/*.php) which enforce general compliance with hook-signatures. Improve predictability/reliability of hook-data. Add a couple examples.

(This is re-spin of #20438. The original PR attempted to identify quirks in hook_civicrm_pre as well as hook_civicrm_post, and the list of quirks was too long. So this targets a more modest hook, which is less pervasive - but still a bit quirky.)

Before

While some hooks have thoughtful test-coverage, many have no test-coverage or inadequate coverage. Techniques for testing hooks are adhoc and focused on individual use-cases. It is difficult to write a test that covers the range of ways in which a hook executes.

Consider hook_civicrm_alterMailParams. There is one test in CiviMail which asserts that the hook fires (CRM_Mailing_MailingSystemTest). But does the hook have sensible content? If an extension-author wants to use the hook, what fields will be available? Under what conditions? Sure, you could update MailingSystemTest to describe the flavor of the hook emitted by CiviMail -- but what about the flavors of the hook from ActionSchedule or MessageTemplates? They're different. For consumers of the hook, we should be presenting a coherent contract -- and we don't have a way to test conformance of those various callers.

After

One may write a general test which will be used any time a hook fires (at least, any time it fires within PHPUnit). For example, consider this draft of tests/events/hook_civicrm_alterMailParams.evch.php:

return new class() extends \Civi\Test\EventCheck implements \Civi\Test\HookInterface {

  /**
   * 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";
    $this->assertTrue(in_array($context, [NULL, 'messageTemplate', 'singleEmail']), "$msg: Unrecognized context ($context)");
    $this->assertTrue(is_array($params), "$msg: Parameters must be an array");
    $this->assertTrue(isset($params['subject']), "$msg: The subject-line is missing");
  }

This leverages all the existing scenarios in the core test-suites and ensures that the hook always presents data in the expected format (regardless of when or how it's fired). If alterMailParams ever fires with an undocumented $context, or if something neglects to pass in the email 'subject', then it will raise an error.

Comments

While working through the example of alterMailParams, I discovered some interesting things, like:

• When firing via sendTemplate(), this hook will actually fire twice:
  • First, it fires with $context==messageTemplate. Some key parameters (html, body, subject) are not populated at this time.
  • Second, it fires with $context==singleEmail. This is the more complete rendering. (But it fires too late to influence some content.)
• When processing scheduled-reminders, there are several extra params, ie:
  • groupName - This is a constant value 'Scheduled Reminder Sender'. This is very different from how message-templates traditionally used the valueName/groupName. (Note that valueName is not used, and groupName is not an option-group, and it doesn't following the naming-convention of the old option-groups.)
  • entity and entity_id: This is the target record which was matched by date-filter.

Of course, when you discover these quirks, you might want to go on safari and do something with them - eg document them in https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_alterMailParams/ or transition the behavior to something else. But... I'll take it as progress that (1) we're able to identify these existing quirks and (2) we'll get an automated headsup if something tries to introduce more quirks.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

So you know I hate this because it locks in us passing a legacy paramter (groupName) to hooks that has been meaningless for months & which we did the work to communicate about over a year ago (including logging issues about extensions that us it) AND have communicated about again recently.

I really think that we should be giving really clear release notes & messaging about it in the release blog & stopping the bleeding on groupName. We're gonna be here in another year :-(

However, it seems like it's sucking the air out of everything so I'll merge it.

[totten]

So you know I hate this because it locks in us passing a legacy paramter (groupName)

(1) I think you're misreading the intent. This doesn't lock us in. It's just about clarity. I want clarity that the changes we think we make are the changes that we actually make. That is not a given when working with alterMailParams because that hook is such a mess.

(2) It sounds like you interpreted the test as requiring that groupName be supplied. I believe that is inaccurate. Here's what it says:

    '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'],
    ],

These rules are only consulted if the field is actually supplied. So breaking out what that means...

• regex: If groupName is supplied, it must begin with msg_ or it must match a narrow list of grandfathered names. If some future PR starts to emit a new groupName, then this should complain. (After all, we're trying to phase-out groupName. We certainly don't want people adding new ones...)
• type: If groupName is supplied, it must be a string. If someone passes the groupName as an array or object, then it should complain. (To the extent that groupName is still emitted, and to the extent that people may be refactoring that code, we would want to know if there was boneheaded mix-up that put invalid data there.)
• for: groupName may only appear in the context of messageTemplate or singleEmail. It must not appear in other contexts (eg civimail, flexmailer, or $unknownFutureContext).

I get confused by your assertion that this is somehow locking us into supporting groupName... if anything, it pins-down groupName to stop it from spreading.

Re-reading, there is one small patch we may want:

-      'type' => 'string',
+      'type' => 'string|NULL',

i.e. Right now, it might complain if the field be both present and null (ie ['groupName'=>NULL]). I dunno - when removing it, do we have a strong preference between omitting-vs-nulling? If nulling is an OK way to remove, then maybe we do the 1-line patch for that.

[eileenmcnaughton]

@totten ok - so we CAN rip out groupName & the tests will let us - phew. Our api contract has always been defined as 'we support what is tested' so I'm glad we are not testing that!