-
Notifications
You must be signed in to change notification settings - Fork 466
refactor: improve query handling in AbstractConnectionResolver
#3125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: improve query handling in AbstractConnectionResolver
#3125
Conversation
|
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
| * | ||
| * @throws \GraphQL\Error\InvariantViolation If the query class is invalid. | ||
| */ | ||
| protected function validate_query_class(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function validate_query_class has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
AbstractConnectionResolver [WIP]AbstractConnectionResolver
|
Tests suites passing for
|
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. |
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