Let explicitly resolved directive classes win in classes() - #2787
Merged
Conversation
DirectiveLocator::disable() marks a directive as non-existent: its definition is left out of the schema and using it fails validation. This enables enforcing project conventions that a built-in directive would bypass. Explicitly resolved classes now also take precedence in classes(), so definitions() can no longer print a directive that differs from the one that is executed.
Disabling a directive at resolution time is unsafe: Lighthouse resolves the fields generated by @paginate, @node and federation through @field, so disabling @field broke those schemas. Whether a built-in directive is used internally is not something users can know, so the API cannot be offered as it was. Restricting what may appear in a hand-written schema belongs to whoever parses that schema.
spawnia
changed the title
spawnia
commented
Sep 2, 2026
Mutate and return the resolved classnames directly, caching the scan.
There was a problem hiding this comment.
🟡 Changes recommended
Explicit aliases can duplicate directive classes and produce invalid definitions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns directive discovery and definitions with explicitly resolved directive classes.
Changes:
- Prioritizes
setResolved()mappings during namespace scans. - Adds regression coverage and override documentation.
- Updates the changelog.
File summaries
| File | Description |
|---|---|
src/Schema/DirectiveLocator.php |
Integrates resolved classes into discovery. |
tests/Unit/Schema/DirectiveLocatorTest.php |
Tests explicit precedence. |
docs/master/custom-directives/getting-started.md |
Documents single-directive overrides. |
CHANGELOG.md |
Records the behavior change. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
setResolved() is not marked as public API, no need to document it.
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
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.
DirectiveLocator::setResolved() already took precedence in resolve(), but classes() only scanned the namespaces, so definitions() could print a directive definition that differs from the directive that actually executes. This also makes it possible to pin a single directive name across namespaces, e.g. to keep a built-in directive that a plugin shadows.
The branch history starts from an attempt to also add DirectiveLocator::disable(), which turned out to be unsafe and was dropped: Lighthouse resolves the fields generated by @paginate, @node and federation through @field, so disabling @field broke those schemas. Users cannot know which built-ins are used internally, so a ban has to be enforced by whoever parses the hand-written schema instead.