perf: refactor AppContext to lazy-load dataloaders
#3380
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors AppContext, updating how DataLoader instances are instantiated and improving PHPStan type hinting. Key changes include deprecating the graphql_data_loaders filter in favor of graphql_data_loader_classes, the introduction of a DEFAULT_LOADERS constant for loader class definitions, and enhanced type hinting for the get_loader() method.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Data/Cursor/AbstractCursor.php | Updates PHPDoc for the $field parameter to enforce array<string,mixed> instead of array<string,mixed> |
| src/AppContext.php | Refactors DataLoader instantiation, introduces DEFAULT_LOADERS constant, and improves PHPStan generics in get_loader() |
Comments suppressed due to low confidence (1)
src/AppContext.php:190
- Instantiating loaders without passing the AppContext instance may lead to issues if the loader constructors require context (as seen previously). Consider passing $this to the loader constructors if they expect it.
return new $loader_class();
AppContext to lazy-load dataloaders
There was a problem hiding this comment.
Pull Request Overview
This PR refactors AppContext to lazy-load DataLoader instances instead of pre-instantiating them, deprecates the old graphql_data_loaders filter in favor of graphql_data_loader_classes, and tightens a docblock in AbstractCursor.
- Introduces
DEFAULT_LOADERSand aloader_classesproperty to hold loader class strings. - Adds
prepare_data_loadersto apply filters and defer instantiation. - Updates
get_loaderto instantiate on first use and adds a magic__getfor deprecation warnings. - Refines the docblock for
is_valid_offset_and_node’s$fieldparameter inAbstractCursor.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Data/Cursor/AbstractCursor.php | Tightened the @param docblock for is_valid_offset_and_node from mixed |
| src/AppContext.php | Refactored to lazy-load loaders, introduced loader_classes/prepare_data_loaders, deprecated the old filter, and added __get deprecation warning |
Comments suppressed due to low confidence (3)
src/AppContext.php:123
- The $loaders property is declared public, so the magic __get method will never be invoked to warn about deprecation. Consider changing its visibility to protected or private to ensure __get is triggered.
public $loaders = [];
src/AppContext.php:172
- Consider adding unit tests for the new lazy-loading behavior in prepare_data_loaders, verifying that loader classes are applied via filters and instances are created on first access.
private function prepare_data_loaders(): void {
src/Data/Cursor/AbstractCursor.php:152
- The docblock restricts $field to array<string,mixed>, but the method signature previously allowed mixed; ensure this tighter type reflects actual usage or update the validation logic accordingly.
* @param array<string,mixed> $field Threshold configuration.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Code Climate has analyzed commit 25e9bf9 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the AppContext to lazy-load data loaders and improves PHPStan type hinting while deprecating the old graphql_data_loaders filter. Key changes include:
- Deferring loader instantiation until accessed and introducing a new graphql_data_loader_classes filter.
- Updating docblocks and type hints for better static analysis and code clarity.
- Fixing documentation typos and clarifying inline comments.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Data/Cursor/AbstractCursor.php | Updated parameter type hint for field configuration in docblock. |
| src/AppContext.php | Refactored lazy-loading of data loaders and updated constructor/docblocks accordingly. |
| docs/using-data-from-custom-database-tables.md | Corrected typos and improved clarity in documentation examples. |
Comments suppressed due to low confidence (2)
docs/using-data-from-custom-database-tables.md:569
- Typo detected: 'Noticication' should be corrected to 'Notification'.
'toType' => 'Noticication',
src/Data/Cursor/AbstractCursor.php:152
- The updated type hint restricts $field to arrays. Please verify that all call sites pass an array to prevent runtime issues.
* @param array<string,mixed> $field Threshold configuration.
What does this implement/fix? Explain your changes.
This PR refactors
AppContextin the following ways:graphql_data_loadersfilter in favor of the newgraphql_data_loader_classesfilter. (so you can replace the loader before it's instantiatedAppContext::$loadershas been deprecated, in lieu of changing the property's visibility toprotected/privatein a future breaking release.::get_loader()to correctly type-hint the loader instance for the given key.E.g. Previously, this would have thrown a PHPStan error that
::get_public_users()doesn't exist onAbstractDataLoaderThis is a nonbreaking change. Users still relying on
graphql_data_loaderswill have all theirAppContext::$loaderspre-instantiated as before.Does this close any currently open issues?
Any other comments?