From ec0dec60219010b7f34143455103d7a3c3330287 Mon Sep 17 00:00:00 2001 From: Oleksandr Prypkhan Date: Thu, 31 Jul 2025 17:14:36 +0200 Subject: [PATCH 1/2] Fix callable type mapping to be Closure mapping --- src/Mappers/Root/CallableTypeMapper.php | 10 ++++++++++ src/QueryField.php | 4 ++-- tests/Fixtures/Integration/Models/Blog.php | 5 +++-- tests/QueryFieldTest.php | 2 +- website/docs/type-mapping.mdx | 22 ++++++++++++++++++---- 5 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/Mappers/Root/CallableTypeMapper.php b/src/Mappers/Root/CallableTypeMapper.php index 7571d15d0a..cfe6425299 100644 --- a/src/Mappers/Root/CallableTypeMapper.php +++ b/src/Mappers/Root/CallableTypeMapper.php @@ -42,6 +42,16 @@ public function toGraphQLOutputType(Type $type, OutputType|null $subType, Reflec throw CannotMapTypeException::createForMissingCallableReturnType(); } + // It would also be a good idea to check if the type-hint is actually `Closure(): something`, + // not `callable(): something`, because the latter is currently not supported. But to do so, + // `phpDocumentor` would need to pass in the type of callable, which it doesn't. All + // types that look like callables - are reported as `callable` by phpDocumentor. + // The reason for such a check is that any string may be a callable (referring to a global function), + // so if a string that looks like a callable is returned from a resolver, it will get wrapped + // in `Deferred`, even though it wasn't supposed to be a deferred value. This could be fixed + // by combining `QueryField`'s resolver and `CallableTypeMapper` into one place, but + // that's not currently possible with GraphQLite's design. + return $this->topRootTypeMapper->toGraphQLOutputType($returnType, null, $reflector, $docBlockObj); } diff --git a/src/QueryField.php b/src/QueryField.php index 3027cf3c71..36cc5a7446 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -97,8 +97,8 @@ private function resolveWithPromise(mixed $result, ResolverInterface $originalRe { // Shorthand for deferring field execution. This does two things: // - removes the dependency on `GraphQL\Deferred` from user land code - // - allows inferring the type from PHPDoc (callable(): Type), unlike Deferred, which is not generic - if (is_callable($result)) { + // - allows inferring the type from PHPDoc (Closure(): Type), unlike Deferred, which is not generic + if ($result instanceof Closure) { $result = new Deferred($result); } diff --git a/tests/Fixtures/Integration/Models/Blog.php b/tests/Fixtures/Integration/Models/Blog.php index 6ced0ee506..f26b02f52e 100644 --- a/tests/Fixtures/Integration/Models/Blog.php +++ b/tests/Fixtures/Integration/Models/Blog.php @@ -4,6 +4,7 @@ namespace TheCodingMachine\GraphQLite\Fixtures\Integration\Models; +use Closure; use GraphQL\Deferred; use TheCodingMachine\GraphQLite\Annotations\Field; use TheCodingMachine\GraphQLite\Annotations\Prefetch; @@ -81,9 +82,9 @@ public static function prefetchSubBlogs(iterable $blogs): array return $subBlogs; } - /** @return callable(): User */ + /** @return Closure(): User */ #[Field] - public function author(): callable { + public function author(): Closure { return fn () => new User('Author', 'author@graphqlite'); } } diff --git a/tests/QueryFieldTest.php b/tests/QueryFieldTest.php index a3e05f69f1..5ca92d5210 100644 --- a/tests/QueryFieldTest.php +++ b/tests/QueryFieldTest.php @@ -49,7 +49,7 @@ public function testParametersDescription(): void $this->assertEquals('Foo argument', $queryField->args[0]->description); } - public function testWrapsCallableInDeferred(): void + public function testWrapsClosureInDeferred(): void { $sourceResolver = new ServiceResolver(static fn () => function () { return 123; diff --git a/website/docs/type-mapping.mdx b/website/docs/type-mapping.mdx index 89f7acc557..404ea28c24 100644 --- a/website/docs/type-mapping.mdx +++ b/website/docs/type-mapping.mdx @@ -332,8 +332,22 @@ query { ## Promise mapping -You can defer execution of fields by returning a callable. To specify the field type, add a `@return` PHPDoc annotation -with a return type, like so: `@return callable(): YourTypeHere`. The callable must not have any parameters. +You can defer execution of fields by returning a `Closure`. To specify the field type, add a `@return` PHPDoc annotation +with a return type, like so: `@return Closure(): YourTypeHere`. The closure must not have any parameters. + +:::caution + +Only `Closure` type is supported, which means that all of the following will work: +- arrow functions: `fn () => 123` +- anonymous functions: `function () { return 123; }` +- first-class callables: `random_int(...)`, `Integer::random(...)` etc + +But other callables **will not**: +- callable strings: `'random_int'`, `'Integer::random'` +- callable arrays: `[Integer::class, 'random']`, `[$object, 'method']` +- invokable objects: `new class { function __invoke() { return 123; } }` + +::: An alternative way is to return `\GraphQL\Deferred` instances, along with specifying the type through the `outputType` parameter of field attributes: `#[Field(outputType: SomeGQLType)]`. @@ -348,10 +362,10 @@ class Product // ... /** - * @return callable(): string + * @return Closure(): string */ #[Field] - public function getName(): callable + public function getName(): Closure { return fn() => $this->name; } From d0d5c9a3935c514bbe00df3b9366b8dca1cc83e0 Mon Sep 17 00:00:00 2001 From: Oleksandr Prypkhan Date: Thu, 31 Jul 2025 17:46:19 +0200 Subject: [PATCH 2/2] Fix broken callable mapping --- src/Mappers/CannotMapTypeException.php | 17 +++-- ...leTypeMapper.php => ClosureTypeMapper.php} | 60 ++++++++++++----- src/QueryField.php | 2 - src/SchemaFactory.php | 4 +- tests/AbstractQueryProvider.php | 4 +- tests/Integration/IntegrationTestCase.php | 4 +- ...pperTest.php => ClosureTypeMapperTest.php} | 67 +++++++++++++------ 7 files changed, 109 insertions(+), 49 deletions(-) rename src/Mappers/Root/{CallableTypeMapper.php => ClosureTypeMapper.php} (59%) rename tests/Mappers/Root/{CallableTypeMapperTest.php => ClosureTypeMapperTest.php} (69%) diff --git a/src/Mappers/CannotMapTypeException.php b/src/Mappers/CannotMapTypeException.php index aa1331747d..84fba9bce2 100644 --- a/src/Mappers/CannotMapTypeException.php +++ b/src/Mappers/CannotMapTypeException.php @@ -181,18 +181,23 @@ public static function createForNonNullReturnByTypeMapper(): self return new self('a type mapper returned a GraphQL\Type\Definition\NonNull instance. All instances returned by type mappers should be nullable. It is the role of the NullableTypeMapperAdapter class to make a GraphQL type in a "NonNull". Note: this is an error in the TypeMapper code or in GraphQLite itself. Please check your custom type mappers or open an issue on GitHub if you don\'t have any custom type mapper.'); } - public static function createForUnexpectedCallableParameters(): self + public static function createForUnexpectedCallable(): self { - return new self('callable() type-hint must not specify any parameters.'); + return new self('callable() type-hint is not supported. Use Closure: Closure(): int'); } - public static function createForMissingCallableReturnType(): self + public static function createForUnexpectedClosureParameters(): self { - return new self('callable() type-hint must specify its return type. For instance: callable(): int'); + return new self('Closure() type-hint must not specify any parameters.'); } - public static function createForCallableAsInput(): self + public static function createForMissingClosureReturnType(): self { - return new self('callable() type-hint can only be used as output type.'); + return new self('Closure() type-hint must specify its return type. For instance: Closure(): int'); + } + + public static function createForClosureAsInput(): self + { + return new self('Closure() type-hint can only be used as output type.'); } } diff --git a/src/Mappers/Root/CallableTypeMapper.php b/src/Mappers/Root/ClosureTypeMapper.php similarity index 59% rename from src/Mappers/Root/CallableTypeMapper.php rename to src/Mappers/Root/ClosureTypeMapper.php index cfe6425299..d57476a30e 100644 --- a/src/Mappers/Root/CallableTypeMapper.php +++ b/src/Mappers/Root/ClosureTypeMapper.php @@ -4,53 +4,69 @@ namespace TheCodingMachine\GraphQLite\Mappers\Root; +use Closure; use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\NamedType; use GraphQL\Type\Definition\OutputType; use GraphQL\Type\Definition\Type as GraphQLType; use phpDocumentor\Reflection\DocBlock; +use phpDocumentor\Reflection\Fqsen; use phpDocumentor\Reflection\Type; use phpDocumentor\Reflection\Types\Callable_; +use phpDocumentor\Reflection\Types\Compound; +use phpDocumentor\Reflection\Types\Object_; use ReflectionMethod; use ReflectionProperty; use TheCodingMachine\GraphQLite\Mappers\CannotMapTypeException; +use function count; +use function iterator_to_array; + /** * This mapper maps callable types into their return types, so that fields can defer their execution. */ -class CallableTypeMapper implements RootTypeMapperInterface +class ClosureTypeMapper implements RootTypeMapperInterface { + private Object_ $closureType; + public function __construct( private readonly RootTypeMapperInterface $next, private readonly RootTypeMapperInterface $topRootTypeMapper, ) { + $this->closureType = new Object_(new Fqsen('\\' . Closure::class)); } public function toGraphQLOutputType(Type $type, OutputType|null $subType, ReflectionMethod|ReflectionProperty $reflector, DocBlock $docBlockObj): OutputType&GraphQLType { - if (! $type instanceof Callable_) { + // This check exists because any string may be a callable (referring to a global function), + // so if a string that looks like a callable is returned from a resolver, it will get wrapped + // in `Deferred`, even though it wasn't supposed to be a deferred value. This could be fixed + // by combining `QueryField`'s resolver and `CallableTypeMapper` into one place, but + // that's not currently possible with GraphQLite's design. + if ($type instanceof Callable_) { + throw CannotMapTypeException::createForUnexpectedCallable(); + } + + if (! $type instanceof Compound || ! $type->contains($this->closureType)) { return $this->next->toGraphQLOutputType($type, $subType, $reflector, $docBlockObj); } - if ($type->getParameters()) { - throw CannotMapTypeException::createForUnexpectedCallableParameters(); + $allTypes = iterator_to_array($type); + + if (count($allTypes) !== 2) { + return $this->next->toGraphQLOutputType($type, $subType, $reflector, $docBlockObj); } - $returnType = $type->getReturnType(); + $callableType = $this->findCallableType($allTypes); + $returnType = $callableType?->getReturnType(); if (! $returnType) { - throw CannotMapTypeException::createForMissingCallableReturnType(); + throw CannotMapTypeException::createForMissingClosureReturnType(); } - // It would also be a good idea to check if the type-hint is actually `Closure(): something`, - // not `callable(): something`, because the latter is currently not supported. But to do so, - // `phpDocumentor` would need to pass in the type of callable, which it doesn't. All - // types that look like callables - are reported as `callable` by phpDocumentor. - // The reason for such a check is that any string may be a callable (referring to a global function), - // so if a string that looks like a callable is returned from a resolver, it will get wrapped - // in `Deferred`, even though it wasn't supposed to be a deferred value. This could be fixed - // by combining `QueryField`'s resolver and `CallableTypeMapper` into one place, but - // that's not currently possible with GraphQLite's design. + if ($callableType->getParameters()) { + throw CannotMapTypeException::createForUnexpectedClosureParameters(); + } return $this->topRootTypeMapper->toGraphQLOutputType($returnType, null, $reflector, $docBlockObj); } @@ -61,11 +77,23 @@ public function toGraphQLInputType(Type $type, InputType|null $subType, string $ return $this->next->toGraphQLInputType($type, $subType, $argumentName, $reflector, $docBlockObj); } - throw CannotMapTypeException::createForCallableAsInput(); + throw CannotMapTypeException::createForClosureAsInput(); } public function mapNameToType(string $typeName): NamedType&GraphQLType { return $this->next->mapNameToType($typeName); } + + /** @param array $types */ + private function findCallableType(array $types): Callable_|null + { + foreach ($types as $type) { + if ($type instanceof Callable_) { + return $type; + } + } + + return null; + } } diff --git a/src/QueryField.php b/src/QueryField.php index 36cc5a7446..0d6e011b6c 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -21,8 +21,6 @@ use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; use TheCodingMachine\GraphQLite\Parameters\SourceParameter; -use function is_callable; - /** * A GraphQL field that maps to a PHP method automatically. * diff --git a/src/SchemaFactory.php b/src/SchemaFactory.php index 92f263d448..037910f281 100644 --- a/src/SchemaFactory.php +++ b/src/SchemaFactory.php @@ -35,7 +35,7 @@ use TheCodingMachine\GraphQLite\Mappers\PorpaginasTypeMapper; use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper; -use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper; +use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper; @@ -400,7 +400,7 @@ public function createSchema(): Schema $lastTopRootTypeMapper = new LastDelegatingTypeMapper(); $topRootTypeMapper = new NullableTypeMapperAdapter($lastTopRootTypeMapper); $topRootTypeMapper = new VoidTypeMapper($topRootTypeMapper); - $topRootTypeMapper = new CallableTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper); + $topRootTypeMapper = new ClosureTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper); $errorRootTypeMapper = new FinalRootTypeMapper($recursiveTypeMapper); $rootTypeMapper = new BaseTypeMapper($errorRootTypeMapper, $recursiveTypeMapper, $topRootTypeMapper); diff --git a/tests/AbstractQueryProvider.php b/tests/AbstractQueryProvider.php index 132616dc8b..e9a25dc48e 100644 --- a/tests/AbstractQueryProvider.php +++ b/tests/AbstractQueryProvider.php @@ -41,7 +41,7 @@ use TheCodingMachine\GraphQLite\Mappers\Parameters\ResolveInfoParameterHandler; use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper; -use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper; +use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper; @@ -360,7 +360,7 @@ protected function buildRootTypeMapper(): RootTypeMapperInterface $lastTopRootTypeMapper = new LastDelegatingTypeMapper(); $topRootTypeMapper = new NullableTypeMapperAdapter($lastTopRootTypeMapper); $topRootTypeMapper = new VoidTypeMapper($topRootTypeMapper); - $topRootTypeMapper = new CallableTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper); + $topRootTypeMapper = new ClosureTypeMapper($topRootTypeMapper, $lastTopRootTypeMapper); $errorRootTypeMapper = new FinalRootTypeMapper($this->getTypeMapper()); $rootTypeMapper = new BaseTypeMapper( diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index b6b34e0936..c3613f26a6 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -41,7 +41,7 @@ use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapper; use TheCodingMachine\GraphQLite\Mappers\RecursiveTypeMapperInterface; use TheCodingMachine\GraphQLite\Mappers\Root\BaseTypeMapper; -use TheCodingMachine\GraphQLite\Mappers\Root\CallableTypeMapper; +use TheCodingMachine\GraphQLite\Mappers\Root\ClosureTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\CompoundTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\EnumTypeMapper; use TheCodingMachine\GraphQLite\Mappers\Root\FinalRootTypeMapper; @@ -297,7 +297,7 @@ public function createContainer(array $overloadedServices = []): ContainerInterf ); }, RootTypeMapperInterface::class => static function (ContainerInterface $container) { - return new CallableTypeMapper( + return new ClosureTypeMapper( new VoidTypeMapper( new NullableTypeMapperAdapter( $container->get('topRootTypeMapper') diff --git a/tests/Mappers/Root/CallableTypeMapperTest.php b/tests/Mappers/Root/ClosureTypeMapperTest.php similarity index 69% rename from tests/Mappers/Root/CallableTypeMapperTest.php rename to tests/Mappers/Root/ClosureTypeMapperTest.php index 53d9c9d147..7053ac5d00 100644 --- a/tests/Mappers/Root/CallableTypeMapperTest.php +++ b/tests/Mappers/Root/ClosureTypeMapperTest.php @@ -2,6 +2,7 @@ namespace TheCodingMachine\GraphQLite\Mappers\Root; +use Closure; use Generator; use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\NamedType; @@ -10,10 +11,12 @@ use GraphQL\Type\Definition\StringType; use GraphQL\Type\Definition\Type as GraphQLType; use phpDocumentor\Reflection\DocBlock; +use phpDocumentor\Reflection\Fqsen; use phpDocumentor\Reflection\Type; use phpDocumentor\Reflection\Types\Array_; use phpDocumentor\Reflection\Types\Callable_; use phpDocumentor\Reflection\Types\CallableParameter; +use phpDocumentor\Reflection\Types\Compound; use phpDocumentor\Reflection\Types\Nullable; use phpDocumentor\Reflection\Types\Object_; use phpDocumentor\Reflection\Types\String_; @@ -25,9 +28,9 @@ use TheCodingMachine\GraphQLite\Fixtures\TestObject2; use TheCodingMachine\GraphQLite\Mappers\CannotMapTypeException; -#[CoversClass(CallableTypeMapper::class)] +#[CoversClass(ClosureTypeMapper::class)] #[CoversClass(CannotMapTypeException::class)] -class CallableTypeMapperTest extends AbstractQueryProvider +class ClosureTypeMapperTest extends AbstractQueryProvider { public function testMapsCallableReturnTypeUsingTopRootMapper(): void { @@ -42,51 +45,77 @@ public function testMapsCallableReturnTypeUsingTopRootMapper(): void ->with($returnType, null, $reflection, $docBlock) ->willReturn(GraphQLType::string()); - $mapper = new CallableTypeMapper( + $mapper = new ClosureTypeMapper( $this->createMock(RootTypeMapperInterface::class), $topRootMapper, ); - $result = $mapper->toGraphQLOutputType(new Callable_(returnType: $returnType), null, $reflection, $docBlock); + $type = new Compound([ + new Callable_(returnType: $returnType), + new Object_(new Fqsen('\\' . Closure::class)) + ]); + + $result = $mapper->toGraphQLOutputType($type, null, $reflection, $docBlock); $this->assertSame(GraphQLType::string(), $result); } - public function testThrowsWhenUsingCallableWithParameters(): void + public function testThrowsWhenUsingCallable(): void { - $this->expectExceptionObject(CannotMapTypeException::createForUnexpectedCallableParameters()); + $this->expectExceptionObject(CannotMapTypeException::createForUnexpectedCallable()); - $mapper = new CallableTypeMapper( + $mapper = new ClosureTypeMapper( $this->createMock(RootTypeMapperInterface::class), $this->createMock(RootTypeMapperInterface::class) ); - $type = new Callable_( - parameters: [ - new CallableParameter(new String_()) - ] + $mapper->toGraphQLOutputType(new Callable_(), null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock()); + } + + public function testThrowsWhenUsingClosureWithParameters(): void + { + $this->expectExceptionObject(CannotMapTypeException::createForUnexpectedClosureParameters()); + + $mapper = new ClosureTypeMapper( + $this->createMock(RootTypeMapperInterface::class), + $this->createMock(RootTypeMapperInterface::class) ); + $type = new Compound([ + new Callable_( + parameters: [ + new CallableParameter(new String_()) + ], + returnType: new String_() + ), + new Object_(new Fqsen('\\' . Closure::class)) + ]); + $mapper->toGraphQLOutputType($type, null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock()); } - public function testThrowsWhenUsingCallableWithoutReturnType(): void + public function testThrowsWhenUsingClosureWithoutReturnType(): void { - $this->expectExceptionObject(CannotMapTypeException::createForMissingCallableReturnType()); + $this->expectExceptionObject(CannotMapTypeException::createForMissingClosureReturnType()); - $mapper = new CallableTypeMapper( + $mapper = new ClosureTypeMapper( $this->createMock(RootTypeMapperInterface::class), $this->createMock(RootTypeMapperInterface::class) ); - $mapper->toGraphQLOutputType(new Callable_(), null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock()); + $type = new Compound([ + new Callable_(), + new Object_(new Fqsen('\\' . Closure::class)) + ]); + + $mapper->toGraphQLOutputType($type, null, new ReflectionMethod(__CLASS__, 'testSkipsNonCallables'), new DocBlock()); } - public function testThrowsWhenUsingCallableAsInputType(): void + public function testThrowsWhenUsingClosureAsInputType(): void { - $this->expectExceptionObject(CannotMapTypeException::createForCallableAsInput()); + $this->expectExceptionObject(CannotMapTypeException::createForClosureAsInput()); - $mapper = new CallableTypeMapper( + $mapper = new ClosureTypeMapper( $this->createMock(RootTypeMapperInterface::class), $this->createMock(RootTypeMapperInterface::class) ); @@ -115,7 +144,7 @@ public function testSkipsNonCallables(callable $createType): void ->with('Name') ->willReturn(GraphQLType::float()); - $mapper = new CallableTypeMapper($next, $this->createMock(RootTypeMapperInterface::class)); + $mapper = new ClosureTypeMapper($next, $this->createMock(RootTypeMapperInterface::class)); $this->assertSame(GraphQLType::string(), $mapper->toGraphQLOutputType($type, null, $reflection, $docBlock)); $this->assertSame(GraphQLType::int(), $mapper->toGraphQLInputType($type, null, 'arg1', $reflection, $docBlock));