Callbacks

[totten]

I ran into an issue where I wanted to declare a callback. Now, normally we have callbacks of the form "CRM_Some_Class::someMethod", but I needed a callback which hit "CRM_Core_Resources::singleton()->resetCacheCode()". You can't sensibly declare the callback as either "CRM_Core_Resources::singleton" or "CRM_Core_Resources::resetCacheCode". I suppose you could write some dummy static function ("CRM_Core_Resources::getSingletonAndResetCacheCode"), but... no. I'd prefer to have notation which means "look up the object and then run a function".

Civi does have a system which can lookup an object based on a name (Civi\Core\Container). I wrote a quick patch to support listeners in Civi\Core\Container (e.g. "@crmResources::resetCacheCode") -- then realized I might be missing a requirement or two. So it seemed sensible to step back, skim requirements, and see what others think.

We don't really have a standard callback notation. Here's a quick survey of the callbacks I could find or think of:

 * Menu XML -- page_callback  -- "CRM_Foo_Bar" or "CRM_Foo_Bar::method"
 * Menu XML -- access_callback -- "CRM_Foo_Bar::method" or "1"
 * Settings -- on_change -- array('CRM_Foo_Bar','method')
 * Settings -- validate_callback -- 'CRM_Foo_Bar::method'
 * Pseudoconstant -- get options -- "CRM_Foo_Bar::method"
 * CRM_Core_BAO_RecurringEntity -- array('helper_class' => 'CRM_Foo_Bar', 'pre_delete_func' => 'myfunc')
 * CRM_Core_BAO_RecurringEntity -- array('helper_class' => 'CRM_Foo_Bar', 'delete_func' => 'myfunc') (but it seems to ignore helper_class and instead use an API)
 * Transactions -- addCallback (with any PHP callable)
 * Scheduled Jobs -- API entity + action
 * Managed Entities -- API entity + action

There's a lot of similarity -- most are influenced either by PHP's standard callback for global/static functions or by Civi's API. However, there's also a chunk of redundancy and similar-but-different bits.

My proposal is to introduce a parsing function which would be used several of these contexts. The parser takes any of the following expressions and returns a PHP callable:

 * Global function: 'my_func'
 * Static function: 'CRM_Foo_bar::method'
 * Static function: array('CRM_Foo_Bar', 'method')
 * OOP function: 'obj://myObject/myFunc' (lookup via Civi\Core\Container)
 * APIv3: 'api3://Entity/action?first=@1&second=@2' (API calls use named args instead of positional args, so need to map)
 * Constant: '1' or '0'

URL notation felt a little awkward at first, but this has some upsides:

 * URL is well-known.
 * PHP has built-in, C-based parsing functions for URLs.
 * It bridges the divide between Drupal-style global functions, Civi-style superstatics, APIs, and OOP.
 * It's extensible - e.g. we could add "api4", "http", or the object containers in D8 and Joomla.
 * It's easy to embed in most data-formats (XML/PHP/YAML/JSON).
 * It should be nearly a drop-in update for most of the callbacks (menu, settings, pseudoconstant, recurring-entity).

This is not to say that it's always a good design to use an API or an HTTP service (or even to use callbacks generally) -- all of these introduce overhead, so it would be bad idea (for example) for one API to kick off callbacks to 20 other APIs; it would be a bad idea for a transaction listener to kick off an HTTP callback. However, looking at actual/likely use-cases and notations, I don't feel like it's going to add overhead unless you purposefully do something expensive.

[Coleman Watts]

This looks like a good convention. Really any convention would be better than what we have. A couple questions:

• I don't quite see how the Class::singleton()->function scenario gets covered by these.
• Would the api call also parse params in json format? Not sure how to pass complex params like arrays or nested api calls.
• How would the parser tell the difference between a global function and a constant?

[totten]

1. The singletons can be registered in the container, e.g.

    $singletons = array(
      'resources' => 'CRM_Core_Resources',
      'httpClient' => 'CRM_Utils_HttpClient',
      'smarty' => 'CRM_Core_Smarty',
    );
    foreach ($singletons as $name => $class) {
      $container->setDefinition($name, new Definition(
        $class
      ))
        ->setFactoryClass($class)->setFactoryMethod('singleton');
    }

(EDIT: In the future, I'd like to deprecate standalone singletons and convert them to use the container, but this the smallest change for the moment.)

2. Hadn't thought about that. Probably not too hard to accept JSON. parse_str() supports []'s for free ("foo[alpha]=1&foo[beta]=2"). Any complex positional parameters ('@1', '@2') would be passed straight-through.

3. I think that only the constants 0 and 1 would be needed (for compat with access_callback), although if there were demand we could go further with tested quotes ("'foo'" would be the constant string 'foo').

4. Aside: In a few places that use callbacks (which I omitted from my list), we accept callbacks from untrusted sources but implement an adhoc checkAuthz() helper that checks the callback against a string pattern. checkAuthz() could (at some point) be generalized a little.

[Peter Haight]

What's the argument for not just using the same rules as what call_user_func allows?

[totten]

call_user_func() has limited support for calling objects. It does work in one situation -- ie when you can write imperative code to lookup the object:

Code: [Select]

$cb = array(Civi\Core\Container::singleton()->get('myobject'), 'myfunction');
call_user_func($cb);

The problem is that you need to execute the PHP code and produce a live copy of the object. This doesn't work in the following situations:

 * Menu XML: You can't call Container::get() from XML. You'd have to make up a notation and write some code to interpret it. (The same would apply to any XML, YAML, or JSON file.)
 * Settings PHP: You could theoretically call Container::get() in here, but this has a few problems. Firstly, the settings metadata will be aggregated and cached. Caching requires serialization, but we don't want to serialize 'myobject'. (In the example of CRM_Core_Resources, there's already a fair amount of work that's gone into managing its lifecycle, and serializing will just lead to extra copies and coherency issues.) Secondly, the settings PHP files are not intended to be imperative code -- they're more like JSON (a static file which happens to be encoded in a programming language for technical simplicity). Thirdly, the mere act of looking up metadata will cause you to spin up all callback objects -- even if the callbacks never fire.

In practical terms, what I'm thinking is to change:

Code: [Select]

// Old idiom. The various examples of this differ slightly, but the gist is generally the same.
if (strpos($callback, '::')) {
  $callback = explode('::', $callback);
}
call_user_func($callback, $arg1, $arg2);

// New idiom
call_user_func(Civi\Core\Callback::create($callback), $arg1, $arg2);


[totten]

Two variants of the PR:

https://github.com/civicrm/civicrm-core/pull/5045 - The lookup function is called Callback::create(). It returns only callables ("function_name", "Class::func", "api3://Entity/action", and "obj://objectName/method").

https://github.com/civicrm/civicrm-core/pull/5046 - The lookup function is called Resolver::get(). It returns callables ("function_name", "Class::func", "api3://Entity/action", "call://objectName/method") and objects ("obj://objectName" and "Class_Name").