Allow the use of callbacks for element value callbacks and batch API redirect callbacks

Fix worked perfectly. Keep up the good work.

Reviewed, I agree with this. It would be great if we had a test for this, but I don't think it is strictly necessary.

Adding modern PHP tag, should also have a meta to track all those issues related to that.

I'm not sure about this - it introduces a potential incompatibility between PHP<5.4 and PHP>5.4 (see note at http://php.net/manual/en/function.call-user-func-array.php).

For example it's pretty common to define a submit handler like this:

function my_form_submit($form, &$form_state) {
...
}

(Since your form submit handler usually does not intend to modify $form by reference.)

But if your code happens to modify or overwrite $form in the course of doing something else in the function (which is OK since it's a local variable), now suddenly with this patch on PHP < 5.4 your modifications take effect outside the function too.

Maybe it's OK to live with this - it's essentially a PHP bug that's fixed in newer versions. But it is a behavior change, and it could potentially break sites in extremely hard-to-debug ways.

Yeah, I agree - the original patch is better than that solution.

In theory we could do a version check everywhere and only use call_user_func_array() >= 5.4 and < 7.

Because ironically in 7, the $function() does work again for all callables ...

To check the version would be the most correct way to do it.

See:

https://3v4l.org/uYgQV

for the problem David describes.

A version check would work although it would mean that any module using this feature would essentially have to declare a dependency on PHP 5.4.

Another way to do it might be with a helper function, something like:

function drupal_call_user_func_by_reference($callback, &p1 = NULL, &p2 = NULL, &p3 = NULL, &p4 = NULL, &p5 = NULL, &p6 = NULL, &p7 = NULL) {
  // Detect if this is a procedural callback function for which backwards compatibility
  // is needed. See http://php.net/manual/language.types.callable.php.
  if (is_string($callback) && strpos($callback, '::') === FALSE) {
    return $callback($p1, $p2, $p3, $p4, $p5, $p6, $p7);
  }
  else {
    return call_user_func_array($callback, array(&p1, &p2, &p3, &p4, &p5, &p6, &p7));
  }
}

The idea being that we only need to worry about backwards compatibility in the standard case where the callback is a procedural function (since that's the only type that was previously allowed); for other types the PHP < 5.4 weird behavior can still occur, of course, but that would only be for new code so it won't suddenly break anyone's existing site - and therefore it's fine.

#12: DX wise I agree, but performance wise I scream.

Even the call_user_func_array() in itself is already a performance regression (in a way), but 3 additional function calls is a lot.

The call_user_func_array() I could still live with - especially because I have a plan to fix that in PHP 7.2 or such, but adding is_string() and strpos() would add a lot more.

I think it is okay to support the newer call_user_func_array() only for >= PHP 5.4 - even if the code gets a little more ugly.

PHP 5.5 will go end-of-life next week and we are finally supporting PHP 5.4/5.5/5.6 and PHP 7.

I cannot imagine someone using the new OOP syntax, but then not using [] or traits or a 3rd party lib or any other feature that is incompatible with PHP 5.2 / 5.3.

And an if statement is neglectable performance wise.

So I am in favor of a version check - if we want to protect PHP 5.2 / 5.3 users.

And in a way it makes sense:

- If you are still on PHP 5.2 / 5.3 you most probably feature stability more than new features. (and in that case probably also care about more "unnecessary" function calls)
- If you are on PHP 5.5 / 5.6, you probably want the new features as some Drupal module needs them.

Thoughts?

OK, I think I'm convinced :) We could add the version check and then just document that this new feature is only available on PHP 5.4 or higher.

Code needs work for that.

#17:

Is really PHP so poor that you should be concerned to add 4 to 8 additional function calls per request? I'm sure the impact of this must be in microsecond range.

Yes! It is so poor ... Function calls are ridiculously expensive in PHP (it needs to setup a full new stack frame, but that is not optimized to use the stack frame already existing for function calls, but needs a lot more). Even FAPI is in the critical path, e.g. on the permissions page. And microsecond can add up to seconds pretty easily as we call things a "million times" in parts. (And yes that is bad in itself, but we can't solve that in this issue)

And while FAPI here might not be the total thing, we are discussing a general issue for the conversion to "Modern PHP" and the theme process / preprocess functions definitely are in the critical path.

I know that good DB structure, indexes, caches, etc. are in the grand scheme of things much more important, but I also know that we had much success in D8 micro-optimizing certain things in the critical path and contrary to common knowledge, if you turn off XHProf, function calls get a little bit faster, but there is still a noticeable difference to code with less function calls.

So long story short:

Version check: Yes, please!
Automagic and string comparison: No, thanks!

And yes, I am okay with #12 (e.g. a helper function) which does the version check internally as long as we don't do string comparisons.

Hm, is a helper function + version_compare() really better performance-wise than a helper function + is_string()/strpos()? I was always under the impression that user space functions are the biggest performance hit in PHP, compared to PHP's built-in functions.

Also worth noting that with a version check in place, the is_string()/strpos() would only have to run on PHP 5.2/5.3.

Decided to try it out quickly; my results were interesting. Basically call_user_func_array() over $function() seems like the biggest performance hit by far. version_compare() vs is_string()/strpos() showed no meaningful difference. Adding a user space helper function slowed things down somewhat for either.

Code:

function my_module_callback_function($p1) {
}

function drupal_call_user_func_by_reference_with_string_check($callback, &$p1 = NULL, &$p2 = NULL, &$p3 = NULL, &$p4 = NULL, &$p5 = NULL, &$p6 = NULL, &$p7 = NULL) {
  if (is_string($callback) && strpos($callback, '::') === FALSE) {
    return $callback($p1, $p2, $p3, $p4, $p5, $p6, $p7);
  }
  else {
    return call_user_func_array($callback, array(&$p1, &$p2, &$p3, &$p4, &$p5, &$p6, &$p7));
  }
}

function drupal_call_user_func_by_reference_with_version_check($callback, &$p1 = NULL, &$p2 = NULL, &$p3 = NULL, &$p4 = NULL, &$p5 = NULL, &$p6 = NULL, &$p7 = NULL) {
  if (version_compare(PHP_VERSION, '5.4.0', '<')) {
    return $callback($p1, $p2, $p3, $p4, $p5, $p6, $p7);
  }
  else {
    return call_user_func_array($callback, array(&$p1, &$p2, &$p3, &$p4, &$p5, &$p6, &$p7));
  }
}

function drupal_call_user_func_by_reference_with_string_check_no_function_calls($callback) {
  if (is_string($callback) && strpos($callback, '::') === FALSE) {
  }
}

function drupal_call_user_func_by_reference_with_version_check_no_function_calls($callback) {
  if (version_compare(PHP_VERSION, '5.4.0', '<')) {
  }
}

$callback = 'my_module_callback_function';
$p1 = 'test';

$functions = array(
  'drupal_call_user_func_by_reference_with_string_check',
  'drupal_call_user_func_by_reference_with_version_check',
  'drupal_call_user_func_by_reference_with_string_check_no_function_calls',
  'drupal_call_user_func_by_reference_with_version_check_no_function_calls',
);
foreach ($functions as $function) {
  $start = microtime(TRUE);
  for ($i = 0; $i < 1000000; $i++) {
    $function($callback, $p1);
  }
  $end = microtime(TRUE);
  $diff = $end - $start;
  print "Function $function took $diff seconds.\n";
}

$start = microtime(TRUE);
for ($i = 0; $i < 1000000; $i++) {
  if (version_compare(PHP_VERSION, '5.4.0', '<')) {
  }
}
$end = microtime(TRUE);
$diff = $end - $start;
print "Plain version comparison took $diff seconds.\n";

$start = microtime(TRUE);
for ($i = 0; $i < 1000000; $i++) {
  if (is_string($callback) && strpos($callback, '::') === FALSE) {
  }
}
$end = microtime(TRUE);
$diff = $end - $start;
print "Plain string comparison took $diff seconds.\n";

And my results on PHP 5.5 (note that "drupal_call_user_func_by_reference_with_version_check" is the only one that does a call_user_func_array()):

$ php --version
PHP 5.5.37-1+deprecated+dontuse+deb.sury.org~precise+1 (cli) (built: Jun 24 2016 08:28:29) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies
    with Xdebug v2.3.2, Copyright (c) 2002-2015, by Derick Rethans
$ 
$ php index.php 
Function drupal_call_user_func_by_reference_with_string_check took 2.3834331035614 seconds.
Function drupal_call_user_func_by_reference_with_version_check took 4.5452010631561 seconds.
Function drupal_call_user_func_by_reference_with_string_check_no_function_calls took 1.3886950016022 seconds.
Function drupal_call_user_func_by_reference_with_version_check_no_function_calls took 1.3805930614471 seconds.
Plain version comparison took 0.81417012214661 seconds.
Plain string comparison took 0.832200050354 seconds.

When I changed the code to use $callback() always (to elimiate the call_user_func_array() effect entirely), strangely version_compare() does noticeably better against string comparison when it is done inside the functions compared to when it's done natively:

$ php index.php 
Function drupal_call_user_func_by_reference_with_string_check took 2.3991439342499 seconds.
Function drupal_call_user_func_by_reference_with_version_check took 2.1317200660706 seconds.
Function drupal_call_user_func_by_reference_with_string_check_no_function_calls took 1.5904839038849 seconds.
Function drupal_call_user_func_by_reference_with_version_check_no_function_calls took 1.4686839580536 seconds.
Plain version comparison took 0.82325983047485 seconds.
Plain string comparison took 0.84052991867065 seconds.

This is odd - maybe it has something to do with the fact that the string comparsion accesses the parameter passed in to the function and version_compare() doesn't. PHP is weird :)

Either way though the effect is still a lot smaller than the call_user_func_array() call itself.

Hm, and one more thing. The above was all with xdebug turned on. With xdebug turned off, the main conclusions above don't change (although all the numbers get faster across the board of course) but the raw string comparisons now actually perform significantly better than the version_compare().

#23: Interesting results.

But we can actually statically cache the version_compare, while we can't cache the string comparisons and yes, call_user_func_array() is slow :(.

Perhaps we can combine:

  static $use_cufa;

  if (!isset($use_cufa) {
    $use_cufa = !version_compare(PHP_VERSION, '5.4.0', '<'));
  }

  if ($use_cufa && is_string($function) && strpos($callback, '::') !== FALSE) {
    call_user_func_array($function, array(&$a1,&$a2,...));
  }
  else {
    $function($a1,$a2,...);
  }

However I just tested:

https://3v4l.org/NnX5h

And it seems the only syntax that is not compatible with $function is indeed the '::' syntax. But it can be changed to array('class_name', 'method') instead and then it works from PHP 5.4 on.

The above code would still ensure we support it, I am just not sure it is worth the trouble if there is a work-around ...

e.g. could do:

  static $use_cufa;

  if (!isset($use_cufa) {
    $use_cufa = !version_compare(PHP_VERSION, '5.4.0', '<'));
  }

  if ($use_cufa && is_string($function) && strpos($callback, '::') !== FALSE) {
    $function = explode('::', $function);
  }

   $function($a1,$a2,...);

avoiding the call_user_func_array completely.

Oh, good point about caching version_compare() in a static.

And it seems the only syntax that is not compatible with $function is indeed the '::' syntax. But it can be changed to array('class_name', 'method') instead and then it works from PHP 5.4 on.

Huh, I didn't know that, but according to http://php.net/manual/en/functions.variable-functions.php you are right.

And I just tested it with anonymous functions (which I think are one of the nicer use cases of this issue) and $function() worked for that too.

Another way of saying that, though, is that if we're limiting ourselves to PHP 5.4 and higher anyway, Drupal 7 already supports the goal of this issue almost completely :) As you alluded to, there's no real reason the caller has to do $element['#after_build'][] = 'SomeClass::someMethod' when they could do $element['#after_build'][] = array('SomeClass', 'someMethod') instead.

So does this issue even have a point, except for potentially making it work for PHP < 5.4 also?

#25: I am now strongly leading to won't fix or rather: Lets document the alternatives (closure or array syntax), because < PHP 5.4 we can't make reliably work anyway.

Also with full PHP 7 support on the horizon and "class::method" being supported there, there is even less incentive.

Also:

With static methods working a magic __call even the controller notion of Drupal 8 can be represented:

['ControllerResolver', 'my__service_myFunction'] would call ['my.service', 'myFunction']

I think if we find cases where the data cannot be stored in array form, we could add the string conversion on a case by case basis, but in general $function() is not only fine, but also as shown quite a bit more performant.

CNW =>

Lets just fix the function_exists => is_callable() and remove all other changes from the patch.

I couldn't be happier that it is finally decided to not use a string comparison. Thank you :). It is not worth hurting performance for those who care to upgrade their PHP version.

I am affected by this a lot, and my solution was (for ajax elements) to pass extra array key in the #ajax property and a generic function (so it passes function_exists() and forwarding the call in the new function).

While trying to create a fix, I had some question that I would be grateful if you could shed a light in.

- The form builder itself (drupal_get_form($var)) accepts a function name, but it is treated as a string in many cases, including when the CSRF tokens are build, etc. Am I correct to assume that we do not support array syntax here?

- The first patch changes the batch operations redirect_callback to the new is_callback() check. This needs complement fixes in batch.inc file as it is implements a form as well, and has some function_exists() calls.

Attaching a patch that replaces function_exists() calls that can be provided by either the form element, or from form array.

- redirect_callback (of a batch).
- #value_callback callbacks.

Ignored these situations:
- drupal_get_form() style callback functions.
- wrapper_callback.
Both above cases because they are used as strings.

- Batch operations function_exists calls because I'm basically changing what changed from above patch.

First, a very important and strong disclaimer: I neither agree or disagree with the patch or the direction if this issue. If this is the direction you want to take then my comment might be useful but nothing else. Please free to disregard. No offense meant by commenting here.

The form builder itself (drupal_get_form($var)) accepts a function name, but it is treated as a string in many cases, including when the CSRF tokens are build, etc.

What the form builder accepts is a form identifier and it indeed can be a function but if that function does not exist then drupal_retrieve_form will invoke hook_forms and then use the callback provided in there without checking whether it is an existing function -- search https://api.drupal.org/api/drupal/includes%21form.inc/function/drupal_re... for $callback to see. So you can just provide a form_id, define it in hook_forms and set the callback to whatever sort of callable you want and that'll already work.

But there's something that might need fixing:

 if (isset($form_state['wrapper_callback']) && function_exists($form_state['wrapper_callback']))

change this to is_callable and appropriately update the hook_forms documentation about wrapper_callback.

Thanks #29 for chiming in. Your expertise and experience on the subject matter of FAPI is very much appreciated.

That is a very good point that hook_forms() makes OOP style callback for forms possible - and that can be transparently implemented by a module.

I agree to change wrapper_callback as well (and update the documentation about it).

Yay, committer approval! Well, then let's continue: there's one problem here: the callback in hook_forms is used as the base_form_id. We should allow for specifying base_form_id explicitly and then amend drupal_get_form::$form_id doxygen which already mentions hook_forms to encourage using it for classes and of course amend hook_forms as well. As we are adding features here, no BC problems. If you are using them, your module will need a versioned dependency on system but that's OK.

I approve of the plan in the IS (adding tag). I think being able to define base_form_id is a nice way to get out of the dilemma with the base_form_id.

I think me might want to split the patch in #28 to another issue though as I am not sure this issue will make it before the 7.50 release and it would be nice to get the minimal parts in at least.

I cloned this issue to #2760609: Allow the use of callbacks instead of global functions in parts of the Form API and make this one purely about value and element callbacks.

The RTBC'ed patch is #28.

Added follow-up to add some tests for #28:

#2760643: Add tests for is_callable() throughout the system

I also made sure callbacks for value callbacks are callables in Drupal 8.

RTBC

Here is some manual testing by hacking color module to use a class callback. It works.

The patch is _not_ for commit.

Updating docs as well.

We'll also need to update the Form API reference (https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...).

+++ b/includes/form.inc
@@ -1718,9 +1718,10 @@ function form_error(&$element, $message = '') {
+ *    property. This defaults to a function named 'form_type_TYPE_value' where
+ *    TYPE is $element['#type'].

indented by a space too many

Moving to next feature release. This definitely needs tests and work first - as the test passed locally, but failed on PHP 5.3 and we have not even tested PHP 5.2.

I was clearly confused when testing this.

Obviously it would fail on PHP 5.3 as the used syntax is just for PHP 5.4+.

So the manual test passing on PHP 5.4 (and 7) should be enough to verify this.

Leaving for someone else to commit though.

Committed and pushed to 7.x, thanks!

I had to roll this back unfortunately. The batch 'redirect_callback' was not fully converted; only one place in core that invokes it was converted but the other still uses function_exists().

Oddly, the same is true for Drupal 8 - but the exact opposite one was converted vs. left at function_exists().... So this basically needs some work for both Drupal 7 and 8, and perhaps a test.

Also, the documentation has some issues:

- * - $element['#value_callback']: A function that implements how user input is
- *   mapped to an element's #value property. This defaults to a function named
- *   'form_type_TYPE_value' where TYPE is $element['#type'].
+ * - $element['#value_callback']: A function (or as of Drupal 7.50, any kind of
+ *   callback) that implements how user input is mapped to an element's #value
+ *    property. This defaults to a function named 'form_type_TYPE_value' where
+ *    TYPE is $element['#type'].

"callback" => "callable", and the indentation is off...

But more generally, it doesn't mention the PHP 5.4 restriction, etc. Actually since it's somewhat complicated to explain the restrictions on this and we didn't document many of the others as allowing PHP callables either, I think the best fix here would probably just be to not change the documentation at all? (It will be documented in a change notice but otherwise we could leave these as "semi-hidden" features, perhaps to be documented later in some kind of centralized, comprehensive way within the codebase.)

Thanks David you are right the batch redirect callbacks did not had a proper fix.
Attaching a new patch with a complement batch redirect_callback fix, sans the documentation update.

To summarize,

Places is_callable and/or call_user_func_array is being used already.
- Batch worker callbacks.
- Custom #validate and #submit

Patch adds to list callable support for:
- Batch redirect_callback.
- #value_callback.

Marking the issue as Needs review to run a test, we lack the batch tests element value callback tests so they need work.