CRM-21798 updates CRM_Event_BAO_Event::checkPermission so it can be called repeatedly to check permissions on a list of even

[spalmstr]

Removed setting the ID as a parameter before getting the list of events as this meant we only got the first event for which to check permissions.

Overview

This pull request updates CRM_Event_BAO_Event::checkPermission so it can be called repeatedly to check permissions on a list of event.

Before

When CRM_Event_BAO_Event::checkPermission() is called repeatedly, it only checks permissions for the first event. All other events are set to have no permissions.
After

When CRM_Event_BAO_Event::checkPermission() is called repeatedly, it checks the permissions for each event and returns them.

Technical Details

if ($eventId) {
$params['id'] = $eventId;
}
in lines 2078ff before getting the result meant that the function only remembered the permissions of the first event checked. Deleting this statement means that all relevant events are checked. The permission on a specific event is checked later on.

Comments

Anything else you would like the reviewer to note

---

• CRM-21798: CRM_Event_BAO_Event::checkPermission($dao->id) only works with the first ID when called repeatedly.

[eileenmcnaughton]

I think the issue is the caching in the function is a bit of a mess. It needs to cache it in a way that allows for the possibility of some events being populated but not others. In general we are moving to Civi::$statics so my suggestion would be to separate the getById to it's own function - the below is NOT a tested functional version - just a quick-write. We could then deprecate passing $eventId into the other function & set this static from the other function as well so we don't lose performance

  /**
   * Make sure that the user has permission to access this event.
   *
   * @param int $eventId
   * @param int $type
   *
   * @return string
   *   the permission that the user has (or null)
   */
  public static function checkPermissionById($eventId = NULL, $type = CRM_Core_Permission::VIEW) {
    if ($eventId && !isset(Civi::$statics[__CLASS__]['permissions'][$eventId])) {
      $permissions[CRM_Core_Permission::VIEW] = civicrm_api3('Event', 'getcount', [
        'check_permissions' => 1,
        'return' => 'id',
      ]);
      if (CRM_Core_Permission::check('edit all events')) {
        $permissions[CRM_Core_Permission::EDIT] = TRUE;
      }
      else {
        $permittedEvents = CRM_ACL_API::group(CRM_Core_Permission::EDIT, NULL, 'civicrm_event', [$eventId => 1], NULL);      
        if ($permittedEvents[$eventId]) {
          $permissions[CRM_Core_Permission::EDIT] = TRUE;
        }
        else {
          $permissions[CRM_Core_Permission::EDIT] = FALSE;
        }
      }
      Civi::$statics[__CLASS__]['permissions'][$eventId] = $permissions;
    }
    return Civi::$statics[__CLASS__]['permissions'][$eventId];
  }

[spalmstr]

Do you mean checkPermissionById as a separate function? Is anyone else working on caching in general?

I agree my workaround was a quick fix to cure the error. There may well be a better approach than the one I took.

[eileenmcnaughton]

yes - I'm proposing it as a different function - in general the CiviCRM codebase does an awful lot of overloading functions to do completely different things in the same function & identifying those cases & splitting the functions out is a good approach.

Regarding Caching in general - the current approach is to use Civi::$statics which is very similar to a drupal pattern. The main advantage of that over an in-function static variable is that it is flushed between test runs, saving much painful debugging. I think in this sort of case php caching is fine

[eileenmcnaughton]

@mlutfy this is actually arising from a patch you did for CRM-21393

[mlutfy]

hmm indeed, so it would revert the fix, re-introducing the performance problem?

Just to get a better overview, in which scenarios does this issue occur? i.e. as a user/admin, how do I reproduce the bug?

[eileenmcnaughton]

@mlutfy it's probably reproducable in a unit test - although the static cache would need to be converted to a Civi::statics cache.

[eileenmcnaughton]

@mlutfy were you going to look at this? I think it's should be replicable in a unit test by calling the function more than once in a row - otherwise we maybe should revert the performance fix for now?

[mlutfy]

@eileenmcnaughton We don't have (user) steps to reproduce this issue, so I find this a bit annoying.

[eileenmcnaughton]

@mlutfy you should be able to reproduce in a test - ie. call that function twice with different event ids & make sure the result is correct

[eileenmcnaughton]

#12424 may relate

[mattwire]

So this PR has been superseded by #12424 and #12769 and should be closed. @spalmstr Could you review/test #12769 for me please?

[eileenmcnaughton]

Closing as superceded per @mattwire