APIv4 - When running validateValues(), fire an event

[totten]

Overview

This patch introduces an internal event, civi.api4.validate, which can be used to introduce more fine-tuned/per-entity validation rules.

Before

To validate an entity in APIv4, one would need to override each method (MyEntity::create(), MyEntity::save()), then override the relevant class and override the validateValues() method.

After

One may listen to civi.api4.validate and add extra validation rules. Here's an example adapted from the docblock:

Civi::dispatcher()->addListener('civi.api4.validate', function(ValidateValuesEvent $e) {
  if ($e->getEntity() !== 'Foozball') return;
  foreach ($e->getRecords() as $r => $record) {
    if (strtotime($record['start_time']) < CRM_Utils_Time::time()) {
      $e->addError($r, 'start_time', 'past', ts('Foozball can only be scheduled in the future. Foozball revisionism is not permitted.'));
    }
    if ($record['length'] * $record['width'] * $record['height'] > VOLUME_LIMIT) {
      $e->addError($r, ['length', 'width', 'height'], 'excessive_volume', ts('The record is too big.'));
    }
  }
});

Technical Details

As with hook_civicrm_pre and hook_civicrm_post or Drupal form hooks, this actually dispatches synonymous events (civi.api4.validate and civi.api4.validate::ENTITY_NAME). For typical/simple cases, this allows to avoid the problem of systemic over-subscription (wherein every listener as to receive every event and then do some conditionals).

There is fairly light test coverage directly in this patch. There is more extensive (but implicit) coverage in a follow-up branch (master-strings). Specifically:

• The Strings entity has a validation listener.
• The StringsTest has several cases where it attempts to send bad data - and the data is rejected based on a validation hook.

[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).

[totten]

ping @colemanw

[colemanw]

Interesting. There's some overlap with #20170 or maybe it's synergy.

[colemanw]

One thing I noticed right away is that this doesn't pass the name of the action to the event, it's assumed to be "create or maybe update". But those two aren't the same. In the Save action you have to tease them apart like this: 20af3ca#diff-ccb444858b7cfcd34c34786bd76003cdccad4c53fbadef2b89df5ba7a94e82f2R107

I think we're going to want to fire this event from the delete action too.

[totten]

@colemanw A few updates here:

• Provide the name of the $action as part of the event (per suggestion)
• Change the code-style to resemble PreEvent/PostEvent more (ie with respect to how event fields are arranged and how the sub-events are dispatched).
• Rebase for good measure

I didn't expand the scope of validateValues to include the delete action... I'm not quite seeing how that would make sense. A delete doesn't have any $values to validate. (I suppose there are some old values in the DB... but if those were invalid to begin with, then it would be silly to require them to become valid before permitting deletion...)

Perhaps that suggestion was spurred by the idea of combining validateValues and checkAccess into one event? I think we could aim for similar style in the events - but now I'm leaning strongly towards "keep them separate". (1) delete is a good example where an action needs one event (checkAccess) but not the other event (validateValues). (2) The payload is different. checkAccess builds a bool $granted, whereas validateValues builds an array $errors.

[colemanw]

I think this whole block ought to be inside a foreach loop as the event is supposed to fire once per record not for an array of records. Also I think the second param (action name) shouldn't be 'save' but should be 'create' or 'update' depending on the presence of $idField like 20af3ca#diff-ccb444858b7cfcd34c34786bd76003cdccad4c53fbadef2b89df5ba7a94e82f2R107

[colemanw]

@totten this looks great. I think it's almost merge-ready. See my one comment on the save action. Also, the event appears to be missing from the update action?

[totten]

@colemanw Revised it to support the update operation. Additionally, the new ValidateValuesTest does a bunch of assertions to ensure that $e->records (etal) are well-defined across different scenarios (eg create vs save vs update).

[totten]

Also, I took a stab at refactoring checkRequiredFields() to be a service that participates in civi.api4.validate - but several implementation bits were hard to get hold because they're baked into the action-class-hierarchy ($this->idField, $this->evaluateCondition(), $this->entityFields()) and I got a bit put-off by the scope-creep. There is a partial refactoring in 61aa344 (which at least puts the 'check-required-fields' stuff into a distinct file), but I'm really tired of having this PR hanging out there, so that's just living in a side-branch.

(EDIT) Which is a long-winded way of saying... I think there's some opportunity for making it easier to use things like idField and entityFields() from a listener... just not right now...

[colemanw]

@totten I think the problem is

PHP Fatal error:  Declaration of api\v4\Entity\ValidateValuesTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /home/jenkins/bknix-dfl/build/core-20184-2ttjf/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Entity/ValidateValuesTest.php on line 31

[totten]

Good eye @colemanw. Fixed/squashed.

[colemanw]

@totten we could trade reviews for #20170

[totten]

Fixed another quirky test issue (data-leak - needed to tweak tearDown()).

@colemanw Sure, I'll take a look when I get up.

[totten]

Replaced by #20533.