Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce "civi.dao.preUpdate" and "civi.dao.preInsert" events #16714

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

christianwach
Copy link
Member

@christianwach christianwach commented Mar 9, 2020

Overview

This PR is for this issue on GitLab and mirrors the pattern of hook_civicrm_pre + hook_civicrm_post and hook_civicrm_preProcess + hook_civicrm_postProcess and the existing pattern of civi.dao.preDelete + civi.dao.postDelete to complete the set with:

  • civi.dao.preInsert + civi.dao.postInsert
  • civi.dao.preUpdate + civi.dao.postUpdate

Before

There isn't a hook available to inspect (for example) the data of an Option Value prior to it being created or updated programatically.

After

It is possible to inspect the data of an Option Value prior to it being created or updated programatically.

Technical Details

With this PR, the following comparison between Option Value data before and after it is updated becomes possible:

class CiviCRM_Pre_Update_Demo {

  /**
   * Register hooks.
   */
  public function __construct() {
    add_action( 'civicrm_config', [ $this, 'config_callback' ], 10 );
  }

  /**
   * Callback for "hook_civicrm_config".
   *
   * @param object $config The CiviCRM config object.
   */
  public function config_callback( &$config ) {

    // Add callback for CiviCRM "preUpdate" hook.
    Civi::service('dispatcher')->addListener(
      'civi.dao.preUpdate',
      [ $this, 'event_type_pre_update' ],
      -100 // Default priority.
    );

    // Add callback for CiviCRM "postUpdate" hook.
    Civi::service('dispatcher')->addListener(
      'civi.dao.postUpdate',
      [ $this, 'event_type_updated' ],
      -100 // Default priority.
    );

  }

  /**
   * Callback for the CiviCRM 'civi.dao.preUpdate' hook.
   *
   * @param object $event The event object.
   * @param string $hook The hook name.
   */
  public function event_type_pre_update( $event, $hook ) {

    // Extract Event Type for this hook.
    $event_type =& $event->object;

    // Bail if this isn't the type of object we're after.
    if ( ! ( $event_type instanceof CRM_Core_DAO_OptionValue ) ) {
      return;
    }
    
    // Get the full data for the Event Type about to be updated.
    $this->event_type_pre = $this->get_event_type_by_id( $event_type->id );
  
    // Maybe do something with the existing data.

  }

  /**
   * Callback for the CiviCRM 'civi.dao.postUpdate' hook.
   *
   * @param object $event The event object.
   * @param string $hook The hook name.
   */
  public function event_type_updated( $event, $hook ) {

    // Extract Event Type for this hook.
    $event_type =& $event->object;

    // Bail if this isn't the type of object we're after.
    if ( ! ( $event_type instanceof CRM_Core_DAO_OptionValue ) ) {
      return;
    }

    // Get the actual Event Type being updated.
    $event_type_full = $this->get_event_type_by_id( $event_type->id );
  
    // Maybe compare new data with the existing data.
    if ( $this->event_type_pre['is_default'] != $event_type_full['is_default'] ) {
      // Do something.
    }

  }

  /**
   * Grab the full data for the Event Type via the CiviCRM API.
   *
   * @param int $event_type_id The ID of the Event Type.
   * @return array $event_type The array of data from the API.
   */
  public function get_event_type_by_id( $event_type_id ) {
    
    // Code to get the data for the Event Type.
  
  }

}

new CiviCRM_Pre_Update_Demo();

@civibot
Copy link

civibot bot commented Mar 9, 2020

(Standard links)

@civibot civibot bot added the master label Mar 9, 2020
@mattwire
Copy link
Contributor

This looks good - would be nice to get some feedback from @totten if possible?

@jaapjansma
Copy link
Contributor

@christianwach could you also update documentation?

@christianwach
Copy link
Member Author

@jaapjansma How do I do that?

@jaapjansma
Copy link
Contributor

@christianwach
Copy link
Member Author

@jaapjansma Oh jeez, yet another complicated CiviCRM workflow to get my head round! FWIW, there's practically no documentation to be found on any individual Symfony hooks as yet (I assume this is because they are considered "internal") so I'd appreciate feedback on whether it's a good idea to start with these ones.

@jaapjansma
Copy link
Contributor

@totten and @eileenmcnaughton what do you think? Is there any reason why the symfony hooks don't have any documentation?

@mattwire
Copy link
Contributor

There is some general symfony docs here: https://docs.civicrm.org/dev/en/latest/hooks/usage/symfony/ In this case I would suggest adding a note about these new ones to the existing hook_civicrm_pre/post pages.
It would be good to add a list of symfony events too, though in theory I guess there could be a lot so difficult to record them all - but more commonly used ones like this would help. We can start small, and the docs can grow from there. I'm sure @MikeyMJCO will have some ideas of how they could grow.

@christianwach
Copy link
Member Author

Given that all the hook signatures are always identical (i.e. two params $event and $hook) I'm not clear what documentation is required. Also, it seems to me that these hooks are only ever going to be used by developers who have found them in the code - how hard is it for us to inspect what's being passed in to the callback? ;-)

@mattwire
Copy link
Contributor

Given that all the hook signatures are always identical (i.e. two params $event and $hook) I'm not clear what documentation is required. Also, it seems to me that these hooks are only ever going to be used by developers who have found them in the code - how hard is it for us to inspect what's being passed in to the callback? ;-)

It's nice to know what to put in addEventListener without having to search through lot's of code - so maybe just a list of those strings.

@homotechsual
Copy link
Contributor

If stuff isn't CiviCRM specific I'd rather we link to symfony's docs where appropriate rather the re-writing stuff or maintaining our own documentation for symfony code - their docs are pretty good.

@christianwach
Copy link
Member Author

It's nice to know what to put in addEventListener

@mattwire Isn't that already covered by this section of the docs?

/**
* @var mixed
*/
public $result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really no consistent data type for $result

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Thanks for highlighting the docblocks as being in need of some TLC. I think that your observations apply more generally to this class - and most likely to other classes too. I'd be happy to tackle those in a separate PR.

/**
* @param $object
* @param $result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add data types to the code blocks (I know it's not there for '$object' :-(

public $object;

/**
* @param $object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto this should have a type

@eileenmcnaughton
Copy link
Contributor

@colemanw thoughts on this? Seems consistent with the delete one we recently discovered....

@totten
Copy link
Member

totten commented Apr 17, 2020

I agree this looks sensible - making the set of events more consistent (with the examples of civi.dao.preDelete, civi.dao.postDelete).

@mattwire
Copy link
Contributor

@christianwach I'm going to merge this as it seems to have wide approval. The docblocks and documentation needs some work and you've said you'll follow up on that :-)

@mattwire mattwire merged commit b5b2761 into civicrm:master Apr 17, 2020
@christianwach christianwach deleted the lab-1638 branch May 18, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants