Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems

1. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,89 @@
   + * as service notation, defined in the form of service_name:method.
   

   I would put bullet points here to explain exactly how a string is resolved into a callback, and in what order things are tried.

   But doing that should probably wait until we've actually got the code stable.

2. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,89 @@
   +  public function invokeFromDefinition($definition, $arguments = []) {
   

   Do we need this?

   call_user_func_array() is notoriously slow, and this method would be stuck using it until we can use variadics, which is when we drop PHP 5 support, which is something like at least a year away.

Also, I wrote the following by mistake on the parent issue:

One of the problems we face in unifying this is that different places use 'ThisClass::thisMethod' to mean different things.

In FieldConfigInterface::setDefaultValueCallback(), a value of 'ThisClass::thisMethod' simply is a PHP callable, representing a static call to thisMethod() on the class ThisClass.

However, in the routing system, putting 'ThisClass::thisMethod' as the controller for a route will cause ControllerResolver to use the DI ClassResolver to instantiate ThisClass, and call thisMethod() on the instance:

  protected function createController($controller) {
    // Controller in the service:method notation.
    $count = substr_count($controller, ':');
    if ($count == 1) {
      list($class_or_service, $method) = explode(':', $controller, 2);
    }
    // Controller in the class::method notation.
    elseif (strpos($controller, '::') !== FALSE) {
      list($class_or_service, $method) = explode('::', $controller, 2);
    }
    else {
      throw new \LogicException(sprintf('Unable to parse the controller name "%s".', $controller));
    }

    $controller = $this->classResolver->getInstanceFromDefinition($class_or_service);

So given that, I think our new CallbackResolver will need two public methods, one for each behaviour: resolveCallbackWithStatic(), and resolveCallbackWithInstantiation().

Then I'd have a single protected method resolveCallback(bool $instantiate_static_notation), where the parameter controls the behaviour.

The two public methods are then just thin wrappers that pass the parameter value, which means we get code re-use, but also we don't require developers to know what a boolean parameter means.

> Hm, it would be a handy convenience method based on the usages I can see. Callables are often invoked as soon as they are resolved.

True, but without variadics, I don't think it's that nice DX, as you have to wrap up your parameters into an array:

$result = $service->invokeCallable($callable, [$param1, $param2]);

With variadics, it becomes:

$result = $service->invokeCallable($callable, $param1, $param2);

There's the performance issue too, so really think we should leave that to a follow-up to be postponed until we can use variadics.

> 3. I think we're already supporting both of these:

I can't see that in the code.

And those two cases in the test look the same to me:

> + static::class . '::staticMethod',

The code will receive a string that looks like \MyNamespace\MyClass::staticMethod

> + '\Drupal\Tests\Core\Utility\MockContainerInjection::getResult',

Same here.

And the service will instatiate the class with the class resolver service and call the method on the instantiated object.

But for baseFieldDefinitions() we actually want a static call. The callable string '\MyNamespace\MyClass::staticMethod' should be called as \MyNamespace\MyClass::staticMethod().

Just a thought that occurs to me as I write tests for #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation -- tests here should check that the service method default callback is not called statically. Stack overflow says the way to do this is:

      $static = !(isset($this) && get_class($this) == __CLASS__);

+++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
@@ -0,0 +1,107 @@
+ * - Static methods: @code '\FooClass::staticMethod' @endcode
+ * - Non-static methods, instantiated with the class resolver:
+ *   @code '\DependencyInjectedClass::method' @endcode

      try {
        if ((new \ReflectionClass($class))->getMethod($method)->isStatic()) {
          return $definition;
        }

I don't think we should do it like this. Reflection is expensive, and we want the callable resolver to be something fast, that other components can use without worrying about performance.

I still think we should handle this the way I suggested, with two methods, as I suggested previously:

> So given that, I think our new CallbackResolver will need two public methods, one for each behaviour: resolveCallbackWithStatic(), and resolveCallbackWithInstantiation().

Users of this service will know whether they want 'MyClass::myMethod' to mean an instantiation, or a static call. It'll be up to them to document what they mean in their own APIs.

    // Support using classes as callables if the __invoke method exists on the
    // class.
    if (method_exists($definition, '__invoke')) {
      return new $definition();
    }

Is there anywhere in core that does this? If not, I would say this is YAGNI, and we should remove it until we do find we need it.

  public function invokeFromDefinition($definition) {

I still think this is surplus to what we need here, and until we can use variadics, it has a performance cost.

      'Static function' => [
        static::class .  '::staticMethod',
      ],

I think for legibility, it would be better for all the callables in the test to be explicit, so I would add another class for these methods to be on.

> in that scenario if we're invoking a callable, are you saying fields can only accept either static or instantiated callbacks but not both?

Yup. I'm saying exactly that :)

The Field system currently treats a callable for FieldConfigInterface::setDefaultValueCallback() of the form 'MyClass::myMethod' as a static call. There is no way to get an object instantiated.

Meanwhile, the routing system treats a controller declared in mymodule.routing.yml as 'MyClass::myMethod' as an object to be instantiated with \Drupal\Core\DependencyInjection\ClassResolver.

In the field case, I imagine it was designed like this because you typically want the method for the default value for a base field to be on entity class, but when the Field system calls that method, it does not have an entity to hand. So it must be a static.

For the routing case, my guess is that it's following the convention from Symfony.

So we have two systems that treat the same syntax differently, and I think the simplest thing is to let them just say to the CallbackResolver what they want to be done with it.

> I'm not really a fan of that from a DX perspective and it goes against the goal of standardisation.

I agree that having the string 'MyClass::myMethod' not result in the same kind of call in different systems is not very good DX, but that situation currently exists. This issue is not adding to or changing this situation.

This proposed CallableResolver is a very low-level API. Most developers shouldn't encounter it. It's something for other systems to use, that then have to declare and document their own APIs.

And the Field system and routing systems *already* document (or should!) what it means when the string 'MyClass::myMethod' is given to their APIs.

And in fact, because we can't break BC, we can't go changing what the string 'MyClass::myMethod' means to either of those systems. For Field system, it must remain that it means a static call. For routing, it must remain that it means an instantiation. So we have to deal with that difference.

This issue is about sliding a new piece of common code at the bottom of existing systems, in such a way that the existing systems change to use our new code, but consumers of the existing systems don't notice or have to care about the change.

Because the existing systems have different ideas about what things mean, we need to account for that.

I think the root cause of why we are disagreeing on how to write this service is that we're approaching it from very different angles.

I think this service should be a consolidation and refactoring of code that's currently duplicated in different parts of core.

It seems to me that you're trying to write a utility class that covers all cases.

I think that approach is over-stretching, and as we can see from the use cases we have to satisfy, it's over-complicating.

Yup, good idea!

FWIW I have been tracking this issue for a while and would like to see something like this implemented, and I tend to agree with @Sam152 on the point that this is supposed to be a DX improvement, so we shouldn't make the DX overcomplicated by adding multiple methods or flags if at all possible. I understand @joachim has performance concerns but I think if we are to take that into account we need a concrete example where this will be a problem, #32 is quite a contrived example but seems to imply there won't be a real world performance hit anywhere? If it turns out there is a critical path that is affected, then we are also free just not to use this new method.

+++ b/core/tests/Drupal/Tests/Core/Utility/CallableResolverTest.php
@@ -0,0 +1,236 @@
+    if (!isset($this)) {
+      throw new \Exception('Non-static method called statically.');

doesn't php enforce this?

I think this makes sense +1 to the idea.

1. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,107 @@
   +  public function invokeFromDefinition($definition) {
   +    $callable = $this->getCallableFromDefinition($definition);
   +    $arguments = func_get_args();
   +    array_shift($arguments);
   +    return call_user_func_array($callable, $arguments);
   +  }
   

   As 8.8.x is going to PHP7 (not sure of the minimum version yet but they all support this) let's use a variadic argument here...

     public function invokeFromDefinition($definition, ...$arguments) {
       $callable = $this->getCallableFromDefinition($definition);
       return call_user_func_array($callable, $arguments);
     }
   

   Also the interface documentation can be updated to use variadic documentation.

2. Sometimes I ponder about whether this sort of service should have an interface. I mean swapping this out or decorating this feels like an amazingly small edge case that we should not make easy because in all likelihood supporting will introduce all sorts of bugs. I.e. imagine if two contrib modules have competing versions.
3. The reflection kinda bothers me - can we test early for callability and avoid reflection?

> The reflection kinda bothers me - can we test early for callability and avoid reflection?

The reason the reflection is in the patch is that in Drupal there is an inconsistency in what we mean by some kinds of callable strings.

Specifically, the string 'MySpace\MyClass::myMethod' does not always mean the same thing.

- In FieldConfigInterface::setDefaultValueCallback(), a value of 'MySpace\MyClass::myMethod' simply is a PHP callable, representing a static call.
- In the routing system, putting 'MySpace\MyClass::myMethod' as the controller for a route will cause ControllerResolver to use the DI ClassResolver to instantiate MySpace\MyClass, and call thisMethod() on the instance.

So the problem is how the new service can handle both techniques.

The options are basically:

1. Provide two methods on the service.

2. Provide a 2nd optional parameter that can specify exactly which kinds of callable techniques are allowed

3. Use reflection.

In light of the more recent issues about callable security, option 2 might be the best, as it could also allow users of the service to enforce their security rules.

  +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
  @@ -0,0 +1,127 @@
  +  public function getCallableFromDefinition($definition) {

Does the order of resolution have an impact on performance? Ie. is_callable() could come first? It all looks pretty simple, but thought I'd ask the question.

Can we have an interface? I know this has been discussed ad nauseam, but if we are creating a new service definition, then it's a public interface right?

Can we justify its usefulness by including real usage of this by replacing it in core somewhere?

I've not read #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE in detail, but it sounds like the requirement there is to fine-control which types of callable are allowed.

Given that, should we revisit the plan outlined in #28, where @Sam152 proposed we allow the users of this service to specify precisely which type of callable are ok?

#2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE is specifically about limiting the callbacks used in render arrays to functions that were written to be render array callbacks. This is important in that context because there are places where a user can enter Twig templates in the UI, where it's possible to build and render render arrays, ie. we're dealing with callbacks from user input.

This issue is way more low-level and seems like it's mainly about situations where only a developer who can edit code files can setup callbacks. So, I'm not sure this issue needs to dig into limiting callbacks that were setup in user input - the validation of the callbacks can be done before calling the APIs built in this issue, which probably makes sense, given that getting callbacks from user input is an edge case that hopefully won't come up very often. :-)

I've just noticed in passing that Drupal\Core\Render\Renderer uses ControllerResolver::getControllerFromDefinition() to resolve callbacks in render arrays, which doesn't look right!

Coming back to this issue as I'm working with options fields, and finding that the 'allowed_values_function' can't use service notation...

Just some nitpicks:

1. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,127 @@
   +      } catch(\ReflectionException $e) {
   

   The catch shouldn't be coddled.

2. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,127 @@
   +        throw new \InvalidArgumentException(sprintf('The callable definition provided was invalid. No method "%s" was not found on the class "%s".', $method, $class));
   

   Is this exception message accurate? Are there other cases in which is_callable() would be false here?

3. +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php
   @@ -0,0 +1,127 @@
   +    // If the definition is natively a callable, we can return it immediately.
   +    if (is_callable($definition)) {
   +      return $definition;
   +    }
   +
   +    // Support using classes as callables if the __invoke method exists on the
   +    // class.
   +    if (method_exists($definition, '__invoke')) {
   +      return new $definition();
   +    }
   +
   +    // Support the service notation syntax.
   

   I think we should check for service notation before we treat the $definition as either a callable or a class name.

Here's a patch with the changes from my review in #51 and a few other tweaks too.

I've changed the first line of the class docs, and also made it clearer in the class docs code example that you can use a service name with a '.' in it because I think that's useful to see.

Created an issue fork with #52 applied to investigate the test failure

Updated test class for deprecations per https://www.drupal.org/node/3056869 and https://www.drupal.org/node/3114724

Updated the exception message to match the message now returned

Tests pass for me locally using:

• PHP 7.4.12
• phpunit 8.5.13

Investigating the failures

Tests pass.

Patch looks good!

Let's try to add the return type hint.

Also wondering if the order of resolution is the most efficient we can do - i.e. could we move is_callable() checks in front of things checking exception and possibly hitting exceptions.

Also can't see immediately what the next step for this is, do we need to open up follow-ups under #2982950: [meta] Standardize the approach for capturing and invoking callables across various subsystems to start using this?

And should we move the work from \Drupal\Core\Security\DoTrustedCallbackTrait into this class? In the course of working on #2966711: Limit what can be called by a callback in form arrays I've needed to add that Trait to FormBuilder and FormValidator, so we are now using the trait in 3 places.

> Also wondering if the order of resolution is the most efficient we can do - i.e. could we move is_callable() checks in front of things checking exception and possibly hitting exceptions.

Yes, definitely! I could have sworn we have all the string manipulations and is_callable() being done before wading into reflection.

FWIW I'm still not keen on the use of reflection here.

> And should we move the work from \Drupal\Core\Security\DoTrustedCallbackTrait into this class?

Yes, that's maybe a good idea.

Though I am torn between saying it should be done in a follow-up, and saying we should do it here to make sure the API is consistent.

Something like add a matching pair of methods getTrustedCallableFromDefinition() and invokeTrustedCallableFromDefinition()?

I agree we should update DoTrustedCallbackTrait within this issue. Updating other "end-user" calls within core is one thing, but that's pretty low-level on it's own.

Also I'm not 100% clear on what the future interaction between this and ControllerResolver will be. It'd be good to make that change here too, IMO.

Hiding the patches since this issue is using MRs now.

Btw PHP 8.2 will need it as voting mostly ++ https://wiki.php.net/rfc/partially-supported-callables-expand-deprecatio...

This would be useful for #3353583: Allow validators passed to file_validate to use callable syntax

I made quite a few changes to address some of the feedback.

• Move the is_callable() check to the top for quick exit.
• Removed the reflection to just rely on the class resolver and is_callable()
• Removed the invokeFromDefinition() method
• Removed the dependency on the container

One side effect is that the error messages are a bit more generic without the extra service container lookups. Not sure whether that is a worthwhile trade off?

I think invoking the callbacks should be out of scope. Having a look at how we use them, they are pretty simple wrappers around DoTrustedCallbackTrait that are specific to Renderer etc. I think we want to keep this more generic.

Updating IS with remaining tasks.

It's been ages since I looked at this.

Was there a resolution to the problem of how to deal with the 'Class::method' syntax, where different systems currently handle that in different ways?

Pretty sure its just :: is for static or instance method. The class resolver takes care of creating new instances for us.

> Pretty sure its just :: is for static or instance method.

That's the thing though -- in some systems, Class::method means a static call, and in others, it means instantiate the class and call $object->method.

Added more callables to test and rebased

Re: #78 Not sure it matters? Calling is_callable() on the string '\Drupal\Tests\Core\Utility\NoInstantiationMockStaticCallable::staticMethod' returns true if it's a static method. If not, it drops down to where we instantiate an object.

Changed ControllerResolver to use CallableResolver and added a CR.

Hence the reflection is no longer required and false from is_callable can accurately signal the need for instantiation or the container.

Whew! 😅

So this looks like a genuine fail for Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest.

When embedding a media item into a ckeditor5 field, I get the following error in logs:

NOTICE: PHP message: Uncaught PHP Exception ReflectionException: "Function \Drupal\media\Controller\MediaFilterController::formatUsesMediaEmbedFilter() does not exist" at /data/app/core/lib/Drupal/Component/Utility/ArgumentsResolver.php line 122

\Drupal\media\Controller\MediaFilterController::formatUsesMediaEmbedFilter() does exist

Is there something we are missing to handle static methods in:

  protected function getReflector(callable $callable) {
    return is_array($callable) ? new \ReflectionMethod($callable[0], $callable[1]) : new \ReflectionFunction($callable);
  }

?

Confirmed! Added a check for a string static method callable with "::" syntax.

Here's a test-only patch for the ArgumentsResolver to show that callables that are static methods in string "::" format aren't currently supported.

> Pre PHP 8 is_callable would return true for public methods that required instantiation, but this changed: https://3v4l.org/d93fd - created this snippet 24h ago because I was similarly confused 😅

Oh that's NICE!

It works for this too:

$callable = Foo::class . '::bar';

The change in PHP 8 is documented here BTW: https://www.php.net/manual/en/migration80.incompatible.php

Tests passed so needs review.

Left some comments on the MR - looking great - some solid test coverage too

Re #64 I don't think moving the trait into this service is needed. Traits should be reused, so I don't see the harm in there being N classes that use it.

However I do think we should be using this in \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback

I think to enable that we'd need a $callableResolver property in that trait and ::getCallableResolver method that first checked the property, and if it wasn't set reached out to the \Drupal singleton. That way classes using the trait can use DI to the property eventually, but if they don't, the class will be fetched from the container. We'd have to update the usages of the trait in core to set that property.

Addressed mostly all feedback in MR, leaving NW for #92 and

- https://git.drupalcode.org/project/drupal/-/merge_requests/338#note_166660
- https://git.drupalcode.org/project/drupal/-/merge_requests/338#note_166667

All feedback has been addressed or at least commented on.

Should we split static methods in strings '::' into arrays first? That we we can do the ArgumentResolver change in a follow up.

Ah, the change to ArgumentResolver is so that it can handle getting the method reflector for a method given in 'Class::method' form? That makes sense now :)

I'd keep it in.

Re #95 there's interesting numbers - #3274867-21: Add TrustedCallback attribute

TL'DR callable as array faster ~50% vs string

PS only first-class callable faster, see #3259716: Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...)

#97 are you saying we should split by "::" first up?

@andypost I'm not sure how much of a micro-optimisation that is? Currently static methods in "::" string format will be returned by the first line:

    if (is_callable($definition)) {
      return $definition;
    }

@kim.pepper no need to change it as PHP internals will do that on call, I see no reason in changing callable as the most performant case is first-class callable but it's just preemptive optimization ATM

Looks ready to go

I noticed service notation without specifying a method name (with an __invoke method) is missing from the documentation/tests. Is this supported yet? If not, do you think it should be supported? It seems potentially useful to me.

Yes I think we should support #101

Added test and docs for service with __invoke()

PS: rebased and merge commits are gone, /cc @kim.pepper

Seems this was already previously reviewed.

Point #101 appears to have been addressed so remarking this for ya.

There's an unresolved comment on the MR about whether we should be using this in a few places

\Drupal\Core\Render\Renderer::doCallback
\Drupal\Core\Menu\MenuLinkTree::transform (and \Drupal\toolbar\Menu\ToolbarMenuLinkTree which subclasses it)
\Drupal\Core\Routing\RouteBuilder::rebuild
\Drupal\Core\Access\CustomAccessCheck::access
\Drupal\user\PermissionHandler::buildPermissionsYaml instead of the controller resolver?

Can we get follow up issues created for that?

Created follow up issues:

• #3363420: Use CallableResolver for \Drupal\Core\Render\Renderer::doCallback()
• #3363421: Use CallableResolver for \Drupal\Core\Menu\MenuLinkTree::transform()
• #3363422: Use CallableResolver for \Drupal\Core\Routing\RouteBuilder::rebuild
• #3363423: Use CallableResolver for \Drupal\Core\Access\CustomAccessCheck::access()
• #3363424: Use CallableResolver for \Drupal\user\PermissionHandler::buildPermissionsYaml() instead of the controller resolver

Needs reroll

Rebased on 11.x

Hiding patch as it makes it look like the MR is failing when its not.

Back to RTBC, thanks

There's some measurements of different types of callables https://gist.github.com/donquixote/85efcca90056111e967dd14cb1f9de9c

Which means we should try re-use a callable as possible

The last MR comment can be resolved.

Issue credits

Committed to 11.x and published the change record.

The child issues are now good to go.

I also added an extra CR announcing the new feature - https://www.drupal.org/node/3368504 please review and amend if appropriate

Grateful and excited to see this committed. I'll rebase the work I started on #2966711: Limit what can be called by a callback in form arrays onto 11.x-dev and update it in light of this work.