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

This would be useful for 📌 Allow validators passed to file_validate to use callable syntax Closed: outdated

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.

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.

Pre PHP 8 is_callable would return true for public methods that required instantiation, but this changed: https://3v4l.org/d93fd

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

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 last submitted patch, 87: 2982949-87-test-only.patch, failed testing. View results →

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

#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:

• 📌 Use CallableResolver for \Drupal\Core\Render\Renderer::doCallback() Needs work
• 📌 Use CallableResolver for \Drupal\Core\Menu\MenuLinkTree::transform() Fixed
• 📌 Use CallableResolver for \Drupal\Core\Routing\RouteBuilder::rebuild Needs work
• 📌 Use CallableResolver for \Drupal\Core\Access\CustomAccessCheck::access() Fixed
• 📌 Use CallableResolver for \Drupal\user\PermissionHandler::buildPermissionsYaml() instead of the controller resolver Fixed

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 📌 Limit what can be called by a callback in form arrays Needs work onto 11.x-dev and update it in light of this work.

Automatically closed - issue fixed for 2 weeks with no activity.