Merged
Conversation
Allows users to override built-in scalars (e.g. a custom String that trims whitespace) on a per-schema basis, without the global side effects of Type::overrideStandardTypes(). Users pass custom scalars via the types config or typeLoader. The executor re-resolves standard scalars from the schema so overrides apply even for fields declared with Type::string() etc. #1424 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of a separate resolveStandardScalar()/findStandardScalarOverride() code path, flip getType() to check loadType() before standard types. This lets the typeLoader naturally override built-in scalars like String, eliminating 3 methods/properties and ~65 lines of duplicate resolution logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
I love it. This is perfect. |
Collaborator
|
Tested it out and it works fine! 🤩 Next step is to deprecate all those overrides? |
Collaborator
Author
|
No issues in Lighthouse. Just worked out name changes and deprecations I want to keep from #1868, it had some good stuff. Will push soon. |
… standard/internal names
Aligns with GraphQL spec terminology ("built-in scalars", "built-in directives").
New well-named alternatives ship alongside deprecated shims for backward compat:
- Type::builtInScalars() / Type::BUILT_IN_SCALAR_NAMES — replaces getStandardTypes() / STANDARD_TYPE_NAMES
- Type::builtInTypes() gains @api — replaces builtInTypes() (already well-named)
- Directive::builtInDirectives() — replaces getInternalDirectives()
- Directive::isBuiltInDirective() — replaces isSpecifiedDirective()
- GraphQL facade methods deprecated, pointing to Type/Directive counterparts
- Type constants included in generated class-reference docs (stable public API)
- Internal references updated; StandardTypesTest left as deprecated regression test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Did the same testing as in #1868 (comment) , all ✅ |
Collaborator
|
@spawnia Shall we merge it? |
Lower retry_threshold from 5 to 2, increase iterations for BuildSchemaBench (2→5) and HugeSchemaBench (3→5), and add 500ms sleep between iterations for those two to allow CPU frequency/cache settling. This cut per-benchmark rstdev from ~1.5–3.8% down to ~0.6–1.3%, eliminating false signals like the spurious +9% regression on benchManyNestedDeferreds that appeared in branch comparisons.
Collaborator
Author
|
Just got around to finalizing this with some benchmarks. I saw no significant regressions. |
Collaborator
Author
|
Released with https://github.com/webonyx/graphql-php/releases/tag/v15.31.0. Thanks @ruudk and @mfn for the feedback! |
ruudk
added a commit
to ruudk/graphql-php
that referenced
this pull request
Mar 16, 2026
…calars When a type loader returns a custom instance for a built-in scalar (e.g. ID, String), `Schema::getType()` stores and returns that instance. However, field argument definitions in the schema still reference the built-in singletons (e.g. `Type::id()`). During query validation, `VariablesInAllowedPosition` resolves variable types via `Schema::getType()` and compares them against argument types using `TypeComparators`. Both `isEqualType()` and `isTypeSubTypeOf()` use strict identity checks (`===`), which fail when comparing two different instances that represent the same named type. This causes queries with variables of overridden built-in scalar types to fail validation with errors like: Variable "$id" of type "ID!" used in position expecting type "ID!". The fix adds a name-based equality check for `NamedType` instances in both `isEqualType()` and `isTypeSubTypeOf()`, falling back to comparing type names when the identity check fails. In a valid schema, each type name is unique, so two named types with the same name are semantically equivalent even if they are different object instances. Ref webonyx#1869 Fixes webonyx#1874
ruudk
added a commit
to ruudk/graphql-php
that referenced
this pull request
Mar 16, 2026
When a type loader returns a custom instance for a built-in scalar (e.g.
ID, String), two problems occur because field definitions still reference
the built-in singletons (e.g. Type::id()):
1. Variable validation fails
VariablesInAllowedPosition resolves variable types via
Schema::getType(), which returns the type loader's instance.
TypeComparators::isEqualType() and isTypeSubTypeOf() compare this
against field argument types using strict identity (===). Since the
type loader instance differs from the built-in singleton, the check
fails with errors like:
Variable \"$id\" of type \"ID!\" used in position expecting type \"ID!\".
Fix: add a name-based equality check for NamedType instances in both
isEqualType() and isTypeSubTypeOf(). In a valid schema, each type
name is unique, so two named types with the same name are semantically
equivalent even if they are different object instances.
2. Input coercion uses wrong parseValue()
Value::coerceInputValue() calls parseValue() on the type from the
field definition (the built-in singleton), not the custom override
from the type loader. This means custom parsing logic (e.g.
converting a base64 global ID string into a domain object) is
skipped, causing input validation to fail.
This is the input-side equivalent of the output fix already present
in ReferenceExecutor::completeValue() (lines 920-923).
Fix: add an optional Schema parameter to coerceInputValue(). When
provided and the type is a ScalarType, resolve the actual type from
the schema to ensure the correct parseValue() is called. The schema
is passed from Values::getVariableValues() and propagated through
all recursive calls.
Ref webonyx#1869
Fixes webonyx#1874
ruudk
added a commit
to ruudk/graphql-php
that referenced
this pull request
Mar 16, 2026
When a type loader returns a custom instance for a built-in scalar (e.g.
ID, String), two problems occur because field definitions still reference
the built-in singletons (e.g. Type::id()):
1. Variable validation fails
VariablesInAllowedPosition resolves variable types via
Schema::getType(), which returns the type loader's instance.
TypeComparators::isEqualType() and isTypeSubTypeOf() compare this
against field argument types using strict identity (===). Since the
type loader instance differs from the built-in singleton, the check
fails with errors like:
Variable \"$id\" of type \"ID!\" used in position expecting type \"ID!\".
Fix: add a name-based equality check for NamedType instances in both
isEqualType() and isTypeSubTypeOf(). In a valid schema, each type
name is unique, so two named types with the same name are semantically
equivalent even if they are different object instances.
2. Input coercion uses wrong parseValue()
Value::coerceInputValue() calls parseValue() on the type from the
field definition (the built-in singleton), not the custom override
from the type loader. This means custom parsing logic (e.g.
converting a base64 global ID string into a domain object) is
skipped, causing input validation to fail.
This is the input-side equivalent of the output fix already present
in ReferenceExecutor::completeValue() (lines 920-923).
Fix: add an optional Schema parameter to coerceInputValue(). When
provided and the type is a ScalarType, resolve the actual type from
the schema to ensure the correct parseValue() is called. The schema
is passed from Values::getVariableValues() and propagated through
all recursive calls.
Ref webonyx#1869
Fixes webonyx#1874
ruudk
added a commit
to ruudk/graphql-php
that referenced
this pull request
Mar 16, 2026
When a type loader returns a custom instance for a built-in scalar (e.g.
ID, String), two problems occur because field definitions still reference
the built-in singletons (e.g. Type::id()):
1. Variable validation fails
VariablesInAllowedPosition resolves variable types via
Schema::getType(), which returns the type loader's instance.
TypeComparators::isEqualType() and isTypeSubTypeOf() compare this
against field argument types using strict identity (===). Since the
type loader instance differs from the built-in singleton, the check
fails with errors like:
Variable \"$id\" of type \"ID!\" used in position expecting type \"ID!\".
Fix: add a name-based equality check for NamedType instances in both
isEqualType() and isTypeSubTypeOf(). In a valid schema, each type
name is unique, so two named types with the same name are semantically
equivalent even if they are different object instances.
2. Input coercion uses wrong parseValue()
Value::coerceInputValue() calls parseValue() on the type from the
field definition (the built-in singleton), not the custom override
from the type loader. This means custom parsing logic (e.g.
converting a base64 global ID string into a domain object) is
skipped, causing input validation to fail.
This is the input-side equivalent of the output fix already present
in ReferenceExecutor::completeValue() (lines 920-923).
Fix: add an optional Schema parameter to coerceInputValue(). When
provided and the type is a ScalarType, resolve the actual type from
the schema to ensure the correct parseValue() is called. The schema
is passed from Values::getVariableValues() and propagated through
all recursive calls.
Ref webonyx#1869
Fixes webonyx#1874
ruudk
added a commit
to ruudk/graphql-php
that referenced
this pull request
Mar 16, 2026
When a type loader returns a custom instance for a built-in scalar (e.g.
ID, String), two problems occur because field definitions still reference
the built-in singletons (e.g. Type::id()):
1. Variable validation fails
VariablesInAllowedPosition resolves variable types via
Schema::getType(), which returns the type loader's instance.
TypeComparators::isEqualType() and isTypeSubTypeOf() compare this
against field argument types using strict identity (===). Since the
type loader instance differs from the built-in singleton, the check
fails with errors like:
Variable \"$id\" of type \"ID!\" used in position expecting type \"ID!\".
Fix: add a name-based equality check for NamedType instances in both
isEqualType() and isTypeSubTypeOf(). In a valid schema, each type
name is unique, so two named types with the same name are semantically
equivalent even if they are different object instances.
2. Input coercion uses wrong parseValue()
Value::coerceInputValue() calls parseValue() on the type from the
field definition (the built-in singleton), not the custom override
from the type loader. This means custom parsing logic (e.g.
converting a base64 global ID string into a domain object) is
skipped, causing input validation to fail.
This is the input-side equivalent of the output fix already present
in ReferenceExecutor::completeValue() (lines 920-923).
Fix: add an optional Schema parameter to coerceInputValue(). When
provided and the type is a ScalarType, resolve the actual type from
the schema to ensure the correct parseValue() is called. The schema
is passed from Values::getVariableValues() and propagated through
all recursive calls.
Ref webonyx#1869
Fixes webonyx#1874
ruudk
added a commit
that referenced
this pull request
Mar 16, 2026
…rides Per-schema scalar overrides (#1869) allow type loaders to return custom instances for built-in scalars (ID, String, Int, Float, Boolean). This introduced two bugs where the custom instance and the built-in singleton diverge, causing identity checks to fail. Bug 1: Variable validation rejects valid queries. TypeComparators uses === to compare types. When a variable type is resolved via Schema::getType() (returning the custom instance) but the argument type references the built-in singleton, the identity check fails and VariablesInAllowedPosition reports a spurious error: "Variable "$id" of type "ID!" used in position expecting type "ID!"." Fix: Add name-based equality in isEqualType() and isTypeSubTypeOf() for built-in scalars, so two ScalarType instances with the same built-in name are considered equal. Bug 2: Input coercion calls the wrong parseValue(). Value::coerceInputValue() calls parseValue() on the type from field definitions (the built-in singleton), not the custom override registered via the type loader. Custom parsing logic (e.g. decoding a global ID) is silently skipped. Fix: Pass the Schema into coerceInputValue() and resolve built-in scalars from the schema before calling parseValue(), mirroring the output-side fix already in ReferenceExecutor::completeValue(). Additionally: - Add Type::isBuiltInScalar() to centralize the instanceof + name check used across TypeComparators, Value, and ReferenceExecutor. - Scope the ReferenceExecutor schema type resolution (from #1869) to built-in scalars only, matching the narrower fix elsewhere. - Replace silent fallbacks with assertions when the schema returns an unexpected type for a built-in scalar name. Fixes #1874 Ref #1869
spawnia
pushed a commit
that referenced
this pull request
Mar 16, 2026
…rides (#1876) Per-schema scalar overrides (#1869) allow type loaders to return custom instances for built-in scalars (ID, String, Int, Float, Boolean). This introduced two bugs where the custom instance and the built-in singleton diverge, causing identity checks to fail. Bug 1: Variable validation rejects valid queries. TypeComparators uses === to compare types. When a variable type is resolved via Schema::getType() (returning the custom instance) but the argument type references the built-in singleton, the identity check fails and VariablesInAllowedPosition reports a spurious error: "Variable "$id" of type "ID!" used in position expecting type "ID!"." Fix: Add name-based equality in isEqualType() and isTypeSubTypeOf() for built-in scalars, so two ScalarType instances with the same built-in name are considered equal. Bug 2: Input coercion calls the wrong parseValue(). Value::coerceInputValue() calls parseValue() on the type from field definitions (the built-in singleton), not the custom override registered via the type loader. Custom parsing logic (e.g. decoding a global ID) is silently skipped. Fix: Pass the Schema into coerceInputValue() and resolve built-in scalars from the schema before calling parseValue(), mirroring the output-side fix already in ReferenceExecutor::completeValue(). Additionally: - Add Type::isBuiltInScalar() to centralize the instanceof + name check used across TypeComparators, Value, and ReferenceExecutor. - Scope the ReferenceExecutor schema type resolution (from #1869) to built-in scalars only, matching the narrower fix elsewhere. - Replace silent fallbacks with assertions when the schema returns an unexpected type for a built-in scalar name. Fixes #1874 Ref #1869
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Allow schemas to override built-in scalars (
String,Int,Float,Boolean,ID) on a per-schema basis, without affecting other schemas or the global singletons.Overrides can be provided via
typesconfig ortypeLoader, both in eager and lazy schemas.This also aligns the public API with GraphQL spec terminology — the spec uses "built-in" exclusively, never "standard" or "internal".
New well-named entry points (
Type::builtInScalars(),Directive::builtInDirectives(), etc.) are added as@api; old names stay as deprecated backward-compatible shims.The global
Type::overrideStandardTypes()is deprecated in favour of the per-schema approach.Closes #1424
Benchmarks
Pending — local WSL results showed too much system noise for reliable comparison. Will run on dedicated hardware.
Test plan
make itpasses (1907 tests, all green)