Allow the use of callbacks instead of global functions in preprocess function callbacks

Don't think there's a reason for this one to be postponed (currently the Drupal 7 issue is marked postponed on it).

Patch attached.

Assumption confirmed :)

@Fabianx, I was trying to write tests but ran to a question. Should these callables support suggestions some how?

Example in core tests that I was looking to add too is test_theme_theme_test_function_suggestions__theme_override

Created some tests for this

Cool thanks @lauriii besides an extra linebreak that works to demonstrate the feature's functionality.

Removed the change for the theme functions since they are deprecated.

+++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestPreprocess.php
@@ -0,0 +1,19 @@
+    $variables['foo'] = 'Make Drupal full of kittens again!';

+1

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -283,8 +283,8 @@ public function render($hook, array $variables) {
-        if (function_exists($preprocessor_function)) {
-          $preprocessor_function($variables, $hook, $info);
+        if (is_callable($preprocessor_function)) {
+          call_user_func_array($preprocessor_function, array(&$variables, $hook, $info));

Do we care about the performance difference here?

I did some quick profiling with the script attached.

PHP 7.0.7 (cli) (built: May 26 2016 16:01:05) ( NTS )
CUFA: 0.30433106422424
Magic call: 0.30942106246948

PHP 5.6.23 (cli) (built: Jun 26 2016 13:17:47)
CUFA: 1.8177201747894
Magic call: 0.77650499343872

PHP 5.5.18 (cli) (built: Nov  3 2014 09:22:08)
CUFA: 1.9406440258026
Magic call: 0.77947521209717

The good thing is this change will have little difference in PHP7 afaics. I guess the question is how many times do we call a preprocess function on a page? Definitely not the 3,000,000 calls of the script. On perhaps a more realistic check on PHP5.5 where $iterations is 100 the script results in:

PHP 5.5.18 (cli) (built: Nov  3 2014 09:22:08)
CUFA: 0.00021886825561523
Magic call: 0.00011205673217773

So double not very much.

CUFA: 0.00021886825561523
Magic call: 0.00011205673217773

To me this doesn't look worth micro-optimization, but the proposed code flow in #20 also looks fine see below. I pinged catch for a second opinion and he said if it's negligible on PHP7 he doesn't mind (but also added that he's looking forward to deprecating preprocess functions as well as theme functions).

@cilefen pointed out that we support callables elsewhere as well, e.g. in Form API. I looked up how we do that. FormStateInterface::prepareCallback() accepts a callback specified as ::methodName(). The implementation:

  public function prepareCallback($callback) {
    if (is_string($callback) && substr($callback, 0, 2) == '::') {
      $callback = [$this->getFormObject(), substr($callback, 2)];
    }
    return $callback;
  }

So that is a bit more hardcoded than what we want to support here.

@cilefen and I also agreed that it would be good to add a change record for this, especially if we are not adding it for deprecated theme functions.

@catch also said:

I don't like the elseif idea because then if a site actually uses the feature there'll be a load of missed function_exists() on top.

Re: perf

Considering that PHP 5 support will be officially removed in March, perhaps this should wait for 8.7.x? That way we don't have to worry about it. I mean, we've already waited many years for this very annoying issue to get fixed, what's a few more months :D

I would also argue that that is actually now a "bug" of 8.x. Almost every callback in 8.x allows for various types of callables (including services) due to its heavy OO nature. This limitation/drupalism was simply inherited from the previous 7.x theme system and never properly fixed when migrated from procedural code to classes.

Oh and #2869859: [PP-1] Refactor theme hooks/registry into plugin managers will likely completely remove the need for any arbitrary functions like this anyway, so this particular issue probably won't be much of one in the future anyway (except in BC cases).

Rerolled

Still needs a change record - needs work for this. I'm +1 on the change - brings things into line.

I had a quick think about if this widens any security concerns because theme callbacks have been involved in recents security advisories but that's when they can be part of the render array and these cannot.

Added change record for the issue - https://www.drupal.org/node/3266641

Resolved CSpell errors & removed change record tag from this issue.

Updated patch will fix test failures.

Missed one test failure in previous patch so resolved in this patch.

Patch #43 is working for me on Core 9.3.

+1

As noted in #31 this was only waiting on a CR, which was added in #38. Moving back to RTBC.

Committed and pushed fb3dc749ac to 10.0.x and 9a265629be to 9.4.x. Thanks!

Does this really allow all PHP callables? Even anonymous functions?