chore: improve type safety of TypeRegistry and schema registration
#3382
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves type safety across the codebase by updating phpdoc annotations and refining type definitions. Key changes include:
- Enhanced generic annotations and templates in DebugLog and TypeRegistry.
- Reworked schema configuration with a builder pattern in SchemaRegistry.
- Updated docblocks in access-functions to enforce array-based configuration.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Utils/DebugLog.php | Updated phpdoc annotations to use generics for improved type safety |
| src/Registry/TypeRegistry.php | Added TypeDef template and refined type loader and eager map logic |
| src/Registry/SchemaRegistry.php | Migrated to a builder-pattern approach for schema configuration |
| access-functions.php | Revised parameter annotations for better type consistency |
Comments suppressed due to low confidence (1)
access-functions.php:225
- The docblock now enforces that $config should be an array. Verify that all callers of register_graphql_interface_type pass an array to avoid unexpected issues. Consider adding a runtime check to enforce the expected type.
* @param array<string,mixed> $config The Type config
|
Code Climate has analyzed commit 3eaa153 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
justlevine
changed the title
TypeRegistry and schema registration
| /** | ||
| * Filter the type before it is loaded into the registry. | ||
| * | ||
| * @param ?TypeDef $type The type to load. |
There was a problem hiding this comment.
This is what's letting us do all this type-narrowing without it being a real breaking change, since InstrumentSchema::instrument_resolvers() would fatal error if anything other than a Type is passed here.
wp-graphql/src/Utils/InstrumentSchema.php
Line 23 in 161565b
Which has been in the codebase since 2021: 6c7cc42
There was a problem hiding this comment.
@justlevine just to clarify, "type-narrowing" in this case is referring to PHP/PHPStan types and not GraphQL Types? The intent of this PR is all about PHP Types, so that's my assumption but wanted to make sure I'm not missing something in regards to GraphQL Types here.
There was a problem hiding this comment.
Ok, following your linked code in InstrumentSchema, I'm following what you're saying. We were already doing type checks during execution so the Type declaration is safe to add as anything that would break now would have already been broken. 👌
There was a problem hiding this comment.
Haha yes PHP type namrrowing.
justlevine
added
status: in review
3 participants
What does this implement/fix? Explain your changes.
This PR improves the type safety across the codebase by reducing the use of
mixedand addressing the resulting PHPStan errors.Additionally, the SchemaConfig syntax was updated to use the [
::create()syntax https://webonyx.github.io/graphql-php/schema-definition/#using-config-class], and wrapped in callable for an ostensible perf improvement.Does this close any currently open issues?
Any other comments?