Add support for @specifiedBy directive#1913
Conversation
Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/31ffabfc-b1cd-46f4-bf81-7753830e77f2 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/31ffabfc-b1cd-46f4-bf81-7753830e77f2 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds first-class support for the GraphQL @specifiedBy directive across schema construction, extension, and printing, so custom scalars can carry/emit a specifiedByURL and the directive appears among built-in directives (including introspection schema output).
Changes:
- Introduces
Directive::specifiedByDirective()/ constants and registers it as a built-in directive; ensuresBuildSchemaincludes it by default. - Threads
specifiedByURLthrough scalar type config, SDL building (ASTDefinitionBuilder), schema extension (SchemaExtender), and SDL printing (SchemaPrinter). - Updates tests to reflect the new built-in directive and enabled specifiedBy-related behaviors.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Type/Definition/Directive.php | Adds built-in @specifiedBy directive definition and constants. |
| src/Type/Definition/ScalarType.php | Adds specifiedByURL to scalar config + instance property. |
| src/Type/Definition/CustomScalarType.php | Updates PHPDoc config types to include specifiedByURL. |
| src/Utils/ASTDefinitionBuilder.php | Extracts specifiedByURL from scalar AST directives and passes into scalar config. |
| src/Utils/BuildSchema.php | Ensures @specifiedBy is included when not explicitly declared in SDL. |
| src/Utils/SchemaExtender.php | Propagates specifiedByURL when extending scalars (incl. reading from extension AST). |
| src/Utils/SchemaPrinter.php | Prints @specifiedBy(url: ...) on scalar definitions when present. |
| src/Language/Printer.php | Adjusts string printing to avoid escaping unicode (and slashes) in JSON encoding. |
| src/Validator/Rules/QueryComplexity.php | Attempts to ignore @specifiedBy during directive-based exclusion logic. |
| tests/Type/IntrospectionTest.php | Updates expected introspection result to include specifiedBy directive. |
| tests/Utils/BreakingChangesFinderTest.php | Includes specifiedByDirective() in removed-directive detection test. |
| tests/Utils/BuildSchemaTest.php | Updates directive counts and enables specifiedBy-related schema build tests. |
| tests/Utils/SchemaExtenderTest.php | Enables and extends scalar extension tests for @specifiedBy. |
| tests/Utils/SchemaPrinterTest.php | Updates expected introspection schema output to include @specifiedBy directive definition. |
| tests/Validator/ValidatorTestCase.php | Ensures test schema includes all built-in/internal directives via getInternalDirectives(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/ba89199d-dc5f-484d-ad1d-983d4fa1a3c8 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
…ExcludesField Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/67e190bd-9ad8-4bc7-ac58-927893b6f552 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/800a98e7-d1e0-4552-bbe1-56a5a001e77b Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: ccb5788 | Previous: 516401c | Ratio |
|---|---|---|---|
DeferredBench::benchManyNestedDeferreds |
21.856 ms |
12.8 ms |
1.71 |
This comment was automatically generated by workflow using github-action-benchmark.
…iedBy Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/0c7ffff2-1fb5-408f-8217-31ac2fc73817 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
…f), descriptions (behavior not behaviour) Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/5536e145-b412-4cca-9f9a-847ccaa65158 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
…ken comment Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/2ff7dec4-1ba2-4840-9f33-af851e349e24 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
…rammar, Printer comment Agent-Logs-Url: https://github.com/webonyx/graphql-php/sessions/ec97a2ec-237f-4065-aa53-2871b3ca7e17 Co-authored-by: spawnia <12158000+spawnia@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Found one behavior issue worth fixing before merge:
// SDL overrides the built-in directive shape
directive @specifiedBy(url: String) on SCALAR
scalar Foo @specifiedBy(url: "https://example.com/foo")ASTDefinitionBuilder::getSpecifiedByURL() and SchemaExtender::extendScalarType() currently treat any directive named @specifiedBy with a string url argument as the built-in directive. That breaks the existing override behavior: once a schema defines its own @specifiedBy, BuildSchema correctly does not add Directive::specifiedByDirective(), but the scalar URL is still copied into ScalarType::$specifiedByURL and later exposed/printed as if it came from the built-in spec directive.
I think extraction should only happen when the active directive definition is the built-in @specifiedBy, not purely by directive name and argument shape. Otherwise custom directives with the same name leak into introspection/SDL semantics they did not opt into.
|
Re: review comment about custom This concern was addressed in 22c0ab1 — the extraction deliberately reads the URL directly from the AST (matching by directive name + argument name + StringValueNode) instead of using The current behavior when a schema overrides When the override is incompatible (different argument name or non-string type), This matches how graphql-js handles it: name-based matching with |
|
Released as v15.33.0. |
graphql-php parsed and validated
@specifiedByin SDL without ever doing anything with it. The URL was silently discarded, so custom scalar types had no way to carry their specification link through the library's own toolchain: it disappeared on introspection, on schema printing, on client-schema reconstruction, and on schema extension. Developers who relied on@specifiedByto document their scalar types (e.g.scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")) received no error and no warning—the information simply vanished, making schema round-trips lossy and introspection-based tooling blind to the spec link.The GraphQL specification and the reference implementation (graphql-js) have shipped
@specifiedByfor several years. Closing this gap matters because:@specifiedBy; importing them via SDL or introspection into graphql-php must not silently drop data.__type { specifiedByURL }to link scalar types to their machine-readable specification. Without this, graphql-php APIs are less discoverable.BuildSchema,BuildClientSchema,SchemaExtender, andSchemaPrinterare all supposed to preserve the full semantics of a schema; losingspecifiedByURLwas a silent correctness bug in each of them.This PR brings every layer of the library into alignment: the type definition, introspection, SDL parsing, schema extension, and printing all now treat
specifiedByURLas a first-class, preserved property—matching graphql-js behaviour and the specification.A latent bug in
QueryComplexitywas found and fixed in scope:directiveExcludesField()returned early on the first unrecognised directive, so a field annotated@foo @skip(if: true)was incorrectly included in complexity calculations. Variable coercion is now lazy and cached so it only runs when@includeor@skipis actually present.