Skip to content

Commit 3f4d98e

Browse files
spawniaspawnia
authored
Stop calling typeLoader for built-in scalar names (#1884)
Calling the typeLoader for built-in scalar names (String, Int, Float, Boolean, ID) breaks downstream consumers whose typeLoaders were not designed to handle these names. Remove typeLoader-based scalar override support, keeping only types-based overrides. Guard loadType() so the typeLoader is never invoked for built-in scalar names. Fixes #1874
1 parent 317a71e commit 3f4d98e

8 files changed

Lines changed: 141 additions & 94 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co
99

1010
## Unreleased
1111

12+
### Changed
13+
14+
- Exclusively support `types` for per-schema scalar overrides, not `typeLoader` https://github.com/webonyx/graphql-php/pull/1884
15+
1216
## v15.31.1
1317

1418
### Fixed

‎benchmarks/ScalarOverrideBench.php‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ class ScalarOverrideBench
2323
{
2424
private Schema $schemaBaseline;
2525

26-
private Schema $schemaTypeLoader;
27-
2826
private Schema $schemaTypes;
2927

3028
public function setUp(): void
@@ -47,21 +45,6 @@ public function setUp(): void
4745
'query' => $queryTypeBaseline,
4846
]);
4947

50-
$queryTypeLoader = new ObjectType([
51-
'name' => 'Query',
52-
'fields' => [
53-
'greeting' => [
54-
'type' => Type::string(),
55-
'resolve' => static fn (): string => 'hello world',
56-
],
57-
],
58-
]);
59-
$typesForLoader = ['Query' => $queryTypeLoader, 'String' => $uppercaseString];
60-
$this->schemaTypeLoader = new Schema([
61-
'query' => $queryTypeLoader,
62-
'typeLoader' => static fn (string $name): ?Type => $typesForLoader[$name] ?? null,
63-
]);
64-
6548
$queryTypeTypes = new ObjectType([
6649
'name' => 'Query',
6750
'fields' => [
@@ -82,11 +65,6 @@ public function benchGetTypeWithoutOverride(): void
8265
$this->schemaBaseline->getType('String');
8366
}
8467

85-
public function benchGetTypeWithTypeLoaderOverride(): void
86-
{
87-
$this->schemaTypeLoader->getType('String');
88-
}
89-
9068
public function benchGetTypeWithTypesOverride(): void
9169
{
9270
$this->schemaTypes->getType('String');
@@ -97,11 +75,6 @@ public function benchExecuteWithoutOverride(): void
9775
GraphQL::executeQuery($this->schemaBaseline, '{ greeting }');
9876
}
9977

100-
public function benchExecuteWithTypeLoaderOverride(): void
101-
{
102-
GraphQL::executeQuery($this->schemaTypeLoader, '{ greeting }');
103-
}
104-
10578
public function benchExecuteWithTypesOverride(): void
10679
{
10780
GraphQL::executeQuery($this->schemaTypes, '{ greeting }');

‎docs/schema-definition.md‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,14 @@ with complex input values (see [Mutations and Input Types](type-definitions/inpu
7878
The schema constructor expects an instance of [`GraphQL\Type\SchemaConfig`](class-reference.md#graphqltypeschemaconfig)
7979
or an array with the following options:
8080

81-
| Option | Type | Notes |
82-
| ------------ | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
83-
| query | `ObjectType` or `callable(): ?ObjectType` or `null` | **Required.** Object type (usually named `Query`) containing root-level fields of your read API |
84-
| mutation | `ObjectType` or `callable(): ?ObjectType` or `null` | Object type (usually named `Mutation`) containing root-level fields of your write API |
85-
| subscription | `ObjectType` or `callable(): ?ObjectType` or `null` | Reserved for future subscriptions implementation. Currently presented for compatibility with introspection query of **graphql-js**, used by various clients (like Relay or GraphiQL) |
86-
| directives | `array<Directive>` | A full list of [directives](type-definitions/directives.md) supported by your schema. By default, contains built-in **@skip** and **@include** directives.<br><br> If you pass your own directives and still want to use built-in directives - add them explicitly. For example:<br><br> _array_merge(GraphQL::getStandardDirectives(), [$myCustomDirective]);_ |
87-
| types | `array<ObjectType>` | List of object types which cannot be detected by **graphql-php** during static schema analysis.<br><br>Most often this happens when the object type is never referenced in fields directly but is still a part of a schema because it implements an interface which resolves to this object type in its **resolveType** callable. <br><br> Note that you are not required to pass all of your types here - it is simply a workaround for a concrete use-case. |
88-
| typeLoader | `callable(string $name): Type` | Expected to return a type instance given the name. Must always return the same instance if called multiple times, see [lazy loading](#lazy-loading-of-types). See section below on lazy type loading. |
81+
| Option | Type | Notes |
82+
| ------------ | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
83+
| query | `ObjectType` or `callable(): ?ObjectType` or `null` | **Required.** Object type (usually named `Query`) containing root-level fields of your read API |
84+
| mutation | `ObjectType` or `callable(): ?ObjectType` or `null` | Object type (usually named `Mutation`) containing root-level fields of your write API |
85+
| subscription | `ObjectType` or `callable(): ?ObjectType` or `null` | Reserved for future subscriptions implementation. Currently presented for compatibility with introspection query of **graphql-js**, used by various clients (like Relay or GraphiQL) |
86+
| directives | `array<Directive>` | A full list of [directives](type-definitions/directives.md) supported by your schema. By default, contains built-in **@skip** and **@include** directives.<br><br> If you pass your own directives and still want to use built-in directives - add them explicitly. For example:<br><br> _array_merge(GraphQL::getStandardDirectives(), [$myCustomDirective]);_ |
87+
| types | `iterable<Type&NamedType>` | Additional types to register in the schema.<br><br>Use this for object types that are not directly referenced in fields but implement an interface that resolves to them via **resolveType**.<br><br>Can also contain custom scalar types named like built-in scalars (`String`, `Int`, etc.) to [override them](type-definitions/scalars.md#overriding-built-in-scalars) on a per-schema basis. |
88+
| typeLoader | `callable(string $name): Type` | Expected to return a type instance given the name. Must always return the same instance if called multiple times, see [lazy loading](#lazy-loading-of-types). See section below on lazy type loading. |
8989

9090
### Using config class
9191

‎docs/type-definitions/scalars.md‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,30 @@ $emailType = new CustomScalarType([
106106

107107
Keep in mind the passed functions will be called statically, so a passed in `callable`
108108
such as `[Foo::class, 'bar']` should only reference static class methods.
109+
110+
## Overriding Built-in Scalars
111+
112+
You can override built-in scalars (`String`, `Int`, `Float`, `Boolean`, `ID`) on a per-schema basis by passing a custom scalar with the same name through the `types` option.
113+
This works with or without a `typeLoader`:
114+
115+
```php
116+
use GraphQL\Type\Definition\CustomScalarType;
117+
use GraphQL\Type\Definition\Type;
118+
use GraphQL\Type\Schema;
119+
120+
$uppercaseString = new CustomScalarType([
121+
'name' => 'String',
122+
'serialize' => static fn ($value): string => strtoupper((string) $value),
123+
]);
124+
125+
$schema = new Schema([
126+
'query' => $queryType,
127+
'typeLoader' => $myTypeLoader,
128+
'types' => [$uppercaseString],
129+
]);
130+
```
131+
132+
The custom scalar replaces the built-in one throughout the entire schema, affecting both serialization of results and coercion of inputs.
133+
134+
> **Note:** The `typeLoader` is never called for built-in scalar names.
135+
> Always use `types` to override them.

‎src/Type/Definition/Type.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ public static function overrideStandardTypes(array $types): void
203203
throw new InvariantViolation("Expecting instance of {$typeClass}, got {$notType}");
204204
}
205205

206-
if (! in_array($type->name, self::BUILT_IN_SCALAR_NAMES, true)) {
206+
if (! self::isBuiltInScalarName($type->name)) {
207207
$standardTypeNames = implode(', ', self::BUILT_IN_SCALAR_NAMES);
208208
$notStandardTypeName = Utils::printSafe($type->name);
209209
throw new InvariantViolation("Expecting one of the following names for a standard type: {$standardTypeNames}; got {$notStandardTypeName}");
@@ -231,6 +231,12 @@ public static function isBuiltInScalar($type): bool
231231
&& in_array($type->name, self::BUILT_IN_SCALAR_NAMES, true);
232232
}
233233

234+
/** Checks if the given name is one of the built-in scalar type names (ID, String, Int, Float, Boolean). */
235+
public static function isBuiltInScalarName(string $name): bool
236+
{
237+
return in_array($name, self::BUILT_IN_SCALAR_NAMES, true);
238+
}
239+
234240
/**
235241
* Determines if the given type is an input type.
236242
*

‎src/Type/Schema.php‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -188,22 +188,6 @@ public function getTypeMap(): array
188188
$allReferencedTypes[$name] = $override;
189189
}
190190

191-
if (isset($this->config->typeLoader)) {
192-
foreach (Type::BUILT_IN_SCALAR_NAMES as $scalarName) {
193-
if (isset($scalarOverrides[$scalarName])) {
194-
continue;
195-
}
196-
197-
$type = ($this->config->typeLoader)($scalarName);
198-
if ($type instanceof ScalarType
199-
&& $type->name === $scalarName
200-
&& $type !== $builtInScalars[$scalarName]
201-
) {
202-
$allReferencedTypes[$scalarName] = $type;
203-
}
204-
}
205-
}
206-
207191
$this->resolvedTypes = $allReferencedTypes;
208192
$this->fullyLoaded = true;
209193
}
@@ -362,11 +346,18 @@ public function hasType(string $name): bool
362346
*/
363347
private function loadType(string $typeName): ?Type
364348
{
365-
if (! isset($this->config->typeLoader)) {
349+
$typeLoader = $this->config->typeLoader;
350+
351+
if (! isset($typeLoader)) {
366352
return $this->getTypeMap()[$typeName] ?? null;
367353
}
368354

369-
$type = ($this->config->typeLoader)($typeName);
355+
// TODO https://github.com/webonyx/graphql-php/issues/1874 - reconsider supporting typeLoader-based scalar overrides in the next major version
356+
if (Type::isBuiltInScalarName($typeName)) {
357+
return null;
358+
}
359+
360+
$type = $typeLoader($typeName);
370361
if ($type === null) {
371362
return null;
372363
}

‎tests/Executor/ExecutorLazySchemaTest.php‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ public function testSimpleQuery(): void
213213
'Query.fields',
214214
'SomeObject',
215215
'SomeObject.fields',
216-
'String',
217216
];
218217
self::assertSame($expected, $result->toArray(DebugFlag::INCLUDE_DEBUG_MESSAGE));
219218
self::assertSame($expectedExecutorCalls, $this->calls);
@@ -380,7 +379,6 @@ public function testDeepQuery(): void
380379
'Query' => true,
381380
'SomeObject' => true,
382381
'OtherObject' => true,
383-
'String' => true,
384382
],
385383
$this->loadedTypes
386384
);
@@ -389,7 +387,6 @@ public function testDeepQuery(): void
389387
'Query.fields',
390388
'SomeObject',
391389
'SomeObject.fields',
392-
'String',
393390
],
394391
$this->calls
395392
);

0 commit comments

Comments
 (0)