refactor: improve query handling in AbstractConnectionResolver#3125
Merged
jasonbahl merged 5 commits intowp-graphql:developfrom May 8, 2024
Merged
refactor: improve query handling in AbstractConnectionResolver#3125jasonbahl merged 5 commits intowp-graphql:developfrom
AbstractConnectionResolver#3125jasonbahl merged 5 commits intowp-graphql:developfrom
Conversation
Collaborator
Author
|
Codeclimate errors are regarding preexisting code. It's just getting flagged again because the code was relocated to new method names (in this or parent PRs). |
6c05e8b to
b8dd6a6
Compare
AbstractConnectionResolver [WIP]AbstractConnectionResolver
Collaborator
Author
|
Tests suites passing for
|
8 tasks
jasonbahl
previously approved these changes
May 8, 2024
The merge-base changed after approval.
|
Code Climate has analyzed commit 0a75ffa and detected 5 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Merged
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.
What does this implement/fix? Explain your changes.
This PR improves query handling in AbstractConnectionResolver by making the following changes:
AbstractConnectionResolver::$query_class, ::query_class()and::get_query_class()` methods.::$query_class()is instantiated once by calling::get_query_class().::get_query_class()looks for the$context->queryClass, falling back to the default::query_class()used by child Connection Resolvers (e.g.PostObjectConnectionResolver). It is then filtered via the newgraphql_connection_query_class` filter before being stored on the class property.::query_class()is set to null in the child class, the child class will need to overload the::query()method (see below) or get an error. Similarly an error will be thrown if$context->queryClassis set, but the Resolver doesn't use::query_class().::is_valid_query_class()method which is used by the new::validate_query_class()method to ensure that overloaded/filtered::$query_classes are still compatible with the::query().::get_query()is now implemented in the class, and serves to instantiateAbstractConnectionResolver::$queryonly once.::get_query()calls the newAbstractConnectionResolver::query()method, which if not overloaded by a child class, instantiates anew ::$query_class().graphql_connection_pre_get_queryfilter has been added to allow extenders to short-circuit the::query()logic without extending the class.Less importantly,
AbstractConnectionResolvernow uses a PHPStan Generic Type to correctly type-hint queries/query-classes and their associated methods. Extenders who wish to opt into this feature, can use a single@extendsdoc-block comment on the Connection Resolver class, instead of needing to overload the typehints for all the relevant properties and methods.Exceptions have been replaced with anInvariantExceptionwhen relevant.There are no breaking changes in this PR.
Important
This PR requires #3124 which should be merged first.
The relevant diff for this PR can be seen here
Todo
$context->queryClassDoes this close any currently open issues?
fixes #2715
closes #2678
Part of #2749
Any relevant logs, error output, GraphiQL screenshots, etc?
Correct type hints using a non-overloaded
AbstractConnectionResolver::$queryor::get_query_method()Any other comments?
Backwards compatibility is ensured by reinstantiating
::$queryand manually applying thegraphql_connection_queryfilter inAbstractConnectionResolver::execute_and_get_ids(). Similarly, direct calls to::$queryin child classes have been left intact. Developers will need to opt into benefit from the new lifecycle and DX improvements, but existing::get_query()overloads will continue to work as before - quirks and all.Where has this been tested?
Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.1.15)
WordPress Version: 6.5.2