From c4cf5949ca0db4242ba68cdc591ee0d0150cd9ac Mon Sep 17 00:00:00 2001 From: Lee Rowlands <lee.rowlands@previousnext.com.au> Date: Thu, 22 Jun 2023 16:16:51 +1000 Subject: [PATCH] Issue #2982949 by kim.pepper, Sam152, andypost, FatherShawn, elber, joachim, larowlan, alexpott, catch, DieterHolvoet, tim.plunkett: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems --- core/core.services.yml | 6 +- .../Component/Utility/ArgumentsResolver.php | 8 +- .../Core/Controller/ControllerResolver.php | 85 ++---- .../Drupal/Core/Utility/CallableResolver.php | 107 +++++++ .../Utility/ArgumentsResolverTest.php | 16 ++ .../Core/Access/CustomAccessCheckTest.php | 11 +- .../Controller/ControllerResolverTest.php | 16 +- .../Core/Utility/CallableResolverTest.php | 263 ++++++++++++++++++ 8 files changed, 423 insertions(+), 89 deletions(-) create mode 100644 core/lib/Drupal/Core/Utility/CallableResolver.php create mode 100644 core/tests/Drupal/Tests/Core/Utility/CallableResolverTest.php diff --git a/core/core.services.yml b/core/core.services.yml index 43244f98dd0b..acd765e78f06 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -827,7 +827,7 @@ services: Symfony\Contracts\EventDispatcher\EventDispatcherInterface: '@event_dispatcher' controller_resolver: class: Drupal\Core\Controller\ControllerResolver - arguments: ['@psr7.http_message_factory', '@class_resolver'] + arguments: ['@callable_resolver'] Drupal\Core\Controller\ControllerResolverInterface: '@controller_resolver' Symfony\Component\HttpKernel\Controller\ControllerResolverInterface: '@controller_resolver' class_resolver: @@ -835,6 +835,10 @@ services: calls: - [setContainer, ['@service_container']] Drupal\Core\DependencyInjection\ClassResolverInterface: '@class_resolver' + callable_resolver: + class: Drupal\Core\Utility\CallableResolver + arguments: ['@class_resolver'] + Drupal\Core\Utility\CallableResolver: '@callable_resolver' title_resolver: class: Drupal\Core\Controller\TitleResolver arguments: ['@controller_resolver', '@string_translation', '@http_kernel.controller.argument_resolver'] diff --git a/core/lib/Drupal/Component/Utility/ArgumentsResolver.php b/core/lib/Drupal/Component/Utility/ArgumentsResolver.php index 0fc5d49925ad..f009a5266fc6 100644 --- a/core/lib/Drupal/Component/Utility/ArgumentsResolver.php +++ b/core/lib/Drupal/Component/Utility/ArgumentsResolver.php @@ -119,7 +119,13 @@ protected function getArgument(\ReflectionParameter $parameter) { * The ReflectionMethod or ReflectionFunction to introspect the callable. */ protected function getReflector(callable $callable) { - return is_array($callable) ? new \ReflectionMethod($callable[0], $callable[1]) : new \ReflectionFunction($callable); + if (is_array($callable)) { + return new \ReflectionMethod($callable[0], $callable[1]); + } + if (is_string($callable) && str_contains($callable, "::")) { + return new \ReflectionMethod($callable); + } + return new \ReflectionFunction($callable); } /** diff --git a/core/lib/Drupal/Core/Controller/ControllerResolver.php b/core/lib/Drupal/Core/Controller/ControllerResolver.php index a7f60b143646..0aaacbea8192 100644 --- a/core/lib/Drupal/Core/Controller/ControllerResolver.php +++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php @@ -3,6 +3,7 @@ namespace Drupal\Core\Controller; use Drupal\Core\DependencyInjection\ClassResolverInterface; +use Drupal\Core\Utility\CallableResolver; use Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface; use Symfony\Component\HttpFoundation\Request; @@ -23,54 +24,34 @@ */ class ControllerResolver implements ControllerResolverInterface { - /** - * The class resolver. - * - * @var \Drupal\Core\DependencyInjection\ClassResolverInterface - */ - protected $classResolver; - - /** - * The PSR-7 converter. - * - * @var \Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface - */ - protected $httpMessageFactory; - /** * Constructs a new ControllerResolver. * - * @param \Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface $http_message_factory - * The PSR-7 converter. - * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver + * @param \Drupal\Core\Utility\CallableResolver|\Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface $callableResolver + * The callable resolver. + * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver * The class resolver. */ - public function __construct(HttpMessageFactoryInterface $http_message_factory, ClassResolverInterface $class_resolver) { - $this->httpMessageFactory = $http_message_factory; - $this->classResolver = $class_resolver; + public function __construct(protected CallableResolver|HttpMessageFactoryInterface $callableResolver, ClassResolverInterface $class_resolver = NULL) { + if ($callableResolver instanceof HttpMessageFactoryInterface) { + @trigger_error('Calling ' . __METHOD__ . '() with the $http_message_factory argument is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3353869', E_USER_DEPRECATED); + $this->callableResolver = \Drupal::service("callable_resolver"); + } + if ($class_resolver !== NULL) { + @trigger_error('Calling ' . __METHOD__ . '() with the $class_resolver argument is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3353869', E_USER_DEPRECATED); + } } /** * {@inheritdoc} */ public function getControllerFromDefinition($controller, $path = '') { - if (is_array($controller) || (is_object($controller) && method_exists($controller, '__invoke'))) { - return $controller; + try { + $callable = $this->callableResolver->getCallableFromDefinition($controller); } - - if (!str_contains($controller, ':')) { - if (function_exists($controller)) { - return $controller; - } - return $this->classResolver->getInstanceFromDefinition($controller); - } - - $callable = $this->createController($controller); - - if (!is_callable($callable)) { - throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable.', $path)); + catch (\InvalidArgumentException $e) { + throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable.', $path), 0, $e); } - return $callable; } @@ -84,38 +65,4 @@ public function getController(Request $request): callable|FALSE { return $this->getControllerFromDefinition($controller, $request->getPathInfo()); } - /** - * Returns a callable for the given controller. - * - * @param string $controller - * A Controller string. - * - * @return mixed - * A PHP callable. - * - * @throws \LogicException - * If the controller cannot be parsed. - * - * @throws \InvalidArgumentException - * If the controller class does not exist. - */ - protected function createController($controller) { - // Controller in the service:method notation. - $count = substr_count($controller, ':'); - if ($count == 1) { - [$class_or_service, $method] = explode(':', $controller, 2); - } - // Controller in the class::method notation. - elseif (str_contains($controller, '::')) { - [$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); - - return [$controller, $method]; - } - } diff --git a/core/lib/Drupal/Core/Utility/CallableResolver.php b/core/lib/Drupal/Core/Utility/CallableResolver.php new file mode 100644 index 000000000000..b41fedf84ed2 --- /dev/null +++ b/core/lib/Drupal/Core/Utility/CallableResolver.php @@ -0,0 +1,107 @@ +<?php + +namespace Drupal\Core\Utility; + +use Drupal\Core\DependencyInjection\ClassResolverInterface; + +/** + * Resolves PHP callables. + * + * The callable resolver service aims to provide a standardized approach to how + * callables are resolved and invoked from various subsystems. The callable + * resolver will return or invoke a callable defined in any of the following + * definition formats: + * + * - Service notation: + * @code + * 'some.service:method' + * @endcode + * - Static methods: + * @code + * '\FooClass::staticMethod' + * @endcode + * - Non-static methods, instantiated with the class resolver: + * @code + * '\DependencyInjectedClass::method' + * @endcode + * - Object calls: + * @code + * [$object, 'method'] + * @endcode + * - Classes with an __invoke method: + * @code + * '\ClassWithInvoke' + * @endcode + * - Closures. + */ +class CallableResolver { + + /** + * Constructs a CallableResolver object. + * + * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $classResolver + * The class resolver. + */ + public function __construct( + protected readonly ClassResolverInterface $classResolver + ) { + } + + /** + * Gets a callable from a definition. + * + * @param callable|array|string $definition + * A callable definition. + * + * @return callable + * A callable. + * + * @throws \InvalidArgumentException + * Thrown when no valid callable could be resolved from the definition. + */ + public function getCallableFromDefinition(callable|array|string $definition): callable { + // If the definition is natively a callable, we can return it immediately. + if (is_callable($definition)) { + return $definition; + } + + if (is_array($definition)) { + throw new \InvalidArgumentException(sprintf('The callable definition provided "[%s]" is not a valid callable.', implode(",", $definition))); + } + + if (!is_string($definition)) { + throw new \InvalidArgumentException(sprintf('The callable definition provided was invalid. Illegal format of type: %s', gettype($definition))); + } + + // Callable with __invoke(). + if (!str_contains($definition, ':')) { + $instance = $this->classResolver->getInstanceFromDefinition($definition); + if (!is_callable($instance)) { + throw new \InvalidArgumentException(sprintf('The callable definition provided was invalid. Class "%s" does not have a method "__invoke" and is not callable.', $instance::class)); + } + return $instance; + } + + // Callable in the service:method notation. + $class_or_service = NULL; + $method = NULL; + $count = substr_count($definition, ':'); + if ($count == 1) { + [$class_or_service, $method] = explode(':', $definition, 2); + } + // Callable in the class::method notation. + if (str_contains($definition, '::')) { + [$class_or_service, $method] = explode('::', $definition, 2); + } + if (empty($class_or_service) || empty($method)) { + throw new \InvalidArgumentException(sprintf('The callable definition provided was invalid. Could not get class and method from definition "%s".', $definition)); + } + + $instance = $this->classResolver->getInstanceFromDefinition($class_or_service); + if (!is_callable([$instance, $method])) { + throw new \InvalidArgumentException(sprintf('The callable definition provided was invalid. Either class "%s" does not have a method "%s", or it is not callable.', $instance::class, $method)); + } + return [$instance, $method]; + } + +} diff --git a/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php index 11cc7b36fa30..99b8da2c516b 100644 --- a/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php +++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php @@ -67,6 +67,15 @@ function ($foo) {}, [], ['foo' => NULL], [], [NULL], function ($foo) {}, $scalars, $objects, [], ['baz'], ]; + // Test a static method string. + $data[] = [ + TestStaticMethodClass::class . "::access", + [], + ['foo' => NULL], + [], + [NULL], + ]; + return $data; } @@ -197,6 +206,13 @@ public function access($foo) { } +class TestStaticMethodClass { + + public static function access($foo) { + } + +} + /** * Provides a test interface. */ diff --git a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php index 1218df686cc6..fd2d53100a71 100644 --- a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php @@ -12,10 +12,9 @@ use Drupal\Core\Controller\ControllerResolver; use Drupal\Core\Routing\RouteMatchInterface; use Drupal\Core\Session\AccountInterface; +use Drupal\Core\Utility\CallableResolver; use Drupal\Tests\UnitTestCase; use Symfony\Component\Routing\Route; -use Drupal\Core\DependencyInjection\ClassResolverInterface; -use Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface; /** * @coversDefaultClass \Drupal\Core\Access\CustomAccessCheck @@ -105,13 +104,13 @@ public function testAccess() { * Tests the access method exception for invalid access callbacks. */ public function testAccessException() { - // Create two mocks for the ControllerResolver constructor. - $httpMessageFactory = $this->getMockBuilder(HttpMessageFactoryInterface::class)->getMock(); - $controllerResolver = $this->getMockBuilder(ClassResolverInterface::class)->getMock(); + $callableResolver = $this->createMock(CallableResolver::class); + $callableResolver->method('getCallableFromDefinition') + ->willThrowException(new \InvalidArgumentException()); // Re-create the controllerResolver mock with proxy to original methods. $this->controllerResolver = $this->getMockBuilder(ControllerResolver::class) - ->setConstructorArgs([$httpMessageFactory, $controllerResolver]) + ->setConstructorArgs([$callableResolver]) ->enableProxyingToOriginalMethods() ->getMock(); diff --git a/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php index 862d4aa89173..cd51d3c191f5 100644 --- a/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php +++ b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php @@ -11,15 +11,14 @@ use Drupal\Core\DependencyInjection\ClassResolver; use Drupal\Core\DependencyInjection\ContainerInjectionInterface; use Drupal\Core\Routing\RouteMatchInterface; +use Drupal\Core\Utility\CallableResolver; use Drupal\Tests\UnitTestCase; -use GuzzleHttp\Psr7\HttpFactory; -use Symfony\Bridge\PsrHttpMessage\Factory\PsrHttpFactory; +use Psr\Http\Message\ServerRequestInterface; use Symfony\Component\DependencyInjection\ContainerAwareInterface; use Symfony\Component\DependencyInjection\ContainerAwareTrait; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\Request; -use Psr\Http\Message\ServerRequestInterface; /** * @coversDefaultClass \Drupal\Core\Controller\ControllerResolver @@ -41,13 +40,6 @@ class ControllerResolverTest extends UnitTestCase { */ protected $container; - /** - * The PSR-7 converter. - * - * @var \Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface - */ - protected $httpMessageFactory; - /** * {@inheritdoc} */ @@ -57,8 +49,8 @@ protected function setUp(): void { $this->container = new ContainerBuilder(); $class_resolver = new ClassResolver(); $class_resolver->setContainer($this->container); - $this->httpMessageFactory = new PsrHttpFactory(new HttpFactory(), new HttpFactory(), new HttpFactory(), new HttpFactory()); - $this->controllerResolver = new ControllerResolver($this->httpMessageFactory, $class_resolver); + $callable_resolver = new CallableResolver($class_resolver); + $this->controllerResolver = new ControllerResolver($callable_resolver); } /** diff --git a/core/tests/Drupal/Tests/Core/Utility/CallableResolverTest.php b/core/tests/Drupal/Tests/Core/Utility/CallableResolverTest.php new file mode 100644 index 000000000000..d0836ec62623 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Utility/CallableResolverTest.php @@ -0,0 +1,263 @@ +<?php + +namespace Drupal\Tests\Core\Utility; + +use Drupal\Core\DependencyInjection\ClassResolver; +use Drupal\Core\DependencyInjection\ContainerBuilder; +use Drupal\Core\DependencyInjection\ContainerInjectionInterface; +use Drupal\Core\Utility\CallableResolver; +use Drupal\Tests\UnitTestCase; +use Symfony\Component\DependencyInjection\ContainerAwareInterface; +use Symfony\Component\DependencyInjection\ContainerAwareTrait; +use Symfony\Component\DependencyInjection\ContainerInterface; + +/** + * @coversDefaultClass \Drupal\Core\Utility\CallableResolver + * @group Utility + */ +class CallableResolverTest extends UnitTestCase { + + /** + * The callable resolver. + * + * @var \Drupal\Core\Utility\CallableResolver + */ + protected CallableResolver $resolver; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + + $container = new ContainerBuilder(); + $container->set('test_service', $this); + + $class_resolver = new ClassResolver(); + $class_resolver->setContainer($container); + + $this->resolver = new CallableResolver($class_resolver); + } + + /** + * @dataProvider callableResolverTestCases + * @covers ::getCallableFromDefinition + */ + public function testCallbackResolver($definition, $result) { + $argument = 'bar'; + $this->assertEquals($result . '+' . $argument, $this->resolver->getCallableFromDefinition($definition)($argument)); + } + + /** + * Test cases for ::testCallbackResolver. + */ + public function callableResolverTestCases() { + return [ + 'Inline function' => [ + function ($suffix) { + return __METHOD__ . '+' . $suffix; + }, + 'Drupal\Tests\Core\Utility\{closure}', + ], + 'First-class callable function' => [ + $this->method(...), + __CLASS__ . '::method', + ], + 'First-class callable static' => [ + static::staticMethod(...), + __CLASS__ . '::staticMethod', + ], + 'Arrow function' => [ + fn($suffix) => __METHOD__ . '+' . $suffix, + 'Drupal\Tests\Core\Utility\{closure}', + ], + 'Static function' => [ + '\Drupal\Tests\Core\Utility\NoInstantiationMockStaticCallable::staticMethod', + 'Drupal\Tests\Core\Utility\NoInstantiationMockStaticCallable::staticMethod', + ], + 'Static function, array notation' => [ + [NoInstantiationMockStaticCallable::class, 'staticMethod'], + 'Drupal\Tests\Core\Utility\NoInstantiationMockStaticCallable::staticMethod', + ], + 'Static function, array notation, with object' => [ + [$this, 'staticMethod'], + __CLASS__ . '::staticMethod', + ], + 'Non-static function, array notation, with object' => [ + [$this, 'method'], + __CLASS__ . '::method', + ], + 'Non-static function, instantiated by class resolver' => [ + static::class . '::method', + __CLASS__ . '::method', + ], + 'Non-static function, instantiated by class resolver, container injection' => [ + '\Drupal\Tests\Core\Utility\MockContainerInjection::getResult', + 'Drupal\Tests\Core\Utility\MockContainerInjection::getResult-foo', + ], + 'Non-static function, instantiated by class resolver, container aware' => [ + '\Drupal\Tests\Core\Utility\MockContainerAware::getResult', + 'Drupal\Tests\Core\Utility\MockContainerAware::getResult', + ], + 'Service notation' => [ + 'test_service:method', + __CLASS__ . '::method', + ], + 'Service notation, static method' => [ + 'test_service:staticMethod', + __CLASS__ . '::staticMethod', + ], + 'Class with invoke method' => [ + static::class, + __CLASS__ . '::__invoke', + ], + ]; + } + + /** + * @dataProvider callableResolverExceptionHandlingTestCases + * @covers ::getCallableFromDefinition + */ + public function testCallbackResolverExceptionHandling($definition, $exception_class, $exception_message) { + $this->expectException($exception_class); + $this->expectExceptionMessage($exception_message); + $this->resolver->getCallableFromDefinition($definition); + } + + /** + * Test cases for ::testCallbackResolverExceptionHandling. + */ + public function callableResolverExceptionHandlingTestCases() { + return [ + 'String function' => [ + 'not_a_callable', + \InvalidArgumentException::class, + 'Class "not_a_callable" does not exist.', + ], + 'Array notation' => [ + ['not_a_callable', 'not_a_callable'], + \InvalidArgumentException::class, + 'The callable definition provided "[not_a_callable,not_a_callable]" is not a valid callable.', + ], + 'Missing method on class, array notation' => [ + [static::class, 'method_not_exists'], + \InvalidArgumentException::class, + 'The callable definition provided "[Drupal\Tests\Core\Utility\CallableResolverTest,method_not_exists]" is not a valid callable.', + ], + 'Missing method on class, static notation' => [ + static::class . '::method_not_exists', + \InvalidArgumentException::class, + 'The callable definition provided was invalid. Either class "Drupal\Tests\Core\Utility\CallableResolverTest" does not have a method "method_not_exists", or it is not callable.', + ], + 'Missing class, static notation' => [ + '\NotARealClass::method', + \InvalidArgumentException::class, + 'Class "\NotARealClass" does not exist.', + ], + 'No method, static notation' => [ + NoMethodCallable::class . "::", + \InvalidArgumentException::class, + 'The callable definition provided was invalid. Could not get class and method from definition "Drupal\Tests\Core\Utility\NoMethodCallable::".', + ], + 'Service not in container' => [ + 'bad_service:method', + \InvalidArgumentException::class, + 'Class "bad_service" does not exist.', + ], + 'Invalid method on valid service' => [ + 'test_service:not_a_callable', + \InvalidArgumentException::class, + 'The callable definition provided was invalid. Either class "Drupal\Tests\Core\Utility\CallableResolverTest" does not have a method "not_a_callable", or it is not callable.', + ], + ]; + } + + /** + * A test static method that returns "foo". + * + * @param string $suffix + * A suffix to append. + * + * @return string + * A test string. + */ + public static function staticMethod($suffix) { + return __METHOD__ . '+' . $suffix; + } + + /** + * A test method that returns "foo". + * + * @param string $suffix + * A suffix to append. + * + * @return string + * A test string. + * + * @throws \Exception + * Throws an exception when called statically. + */ + public function method($suffix) { + return __METHOD__ . '+' . $suffix; + } + + /** + * A test __invoke method. + * + * @param string $suffix + * A suffix to append. + * + * @return string + * A test string. + */ + public function __invoke($suffix) { + return __METHOD__ . '+' . $suffix; + } + +} + +class MockContainerInjection implements ContainerInjectionInterface { + + protected $injected; + + public function __construct($result) { + $this->injected = $result; + } + + public static function create(ContainerInterface $container) { + return new static('foo'); + } + + public function getResult($suffix) { + return __METHOD__ . '-' . $this->injected . '+' . $suffix; + } + +} + +class NoInstantiationMockStaticCallable { + + public function __construct() { + throw new \Exception(sprintf('The class %s should not require instantiation for the static method to be called.', __CLASS__)); + } + + public static function staticMethod($suffix) { + return __METHOD__ . '+' . $suffix; + } + +} + +class NoMethodCallable { +} + +class MockContainerAware implements ContainerAwareInterface { + + use ContainerAwareTrait; + + public function getResult($suffix) { + if (empty($this->container)) { + throw new \Exception('Container was not injected.'); + } + return __METHOD__ . '+' . $suffix; + } + +} -- GitLab