refactor!: [WIP] Exploring a ConnectionResolver 2.0 #2749
Conversation
justlevine
added
Type: Enhancement
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
justlevine
force-pushed
the
proposal/v2-connection-resolver
branch
from
9c2ed83
to
45a36bf
Compare
|
aad0106 refactors the ConnectionResolver to support overloading the
protected function query_class() ?: string {
return 'WC_Query';
}
...$connectionConfig,
'resolve' => function( $source, $args, $context, $info ) {
$resolver = new PostObjectConnectionResolver( $source, $args, $context, $info );
$resolver->set_query_class( WC_Query::class ); // If "" is passed, revert to default query class.
return $resolver->get_connection();
},
protected function is_valid_query_class( string $query_class ) : bool {
return method_exists( $query_class, 'get_results' ); // Instead of the default `WP_Query::query()`
}As a result of these changes:
|
justlevine
force-pushed
the
proposal/v2-connection-resolver
branch
from
aad0106
to
218c30d
Compare
| * @throws \Exception | ||
| */ | ||
| public function get_query_args() { | ||
| public function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Method prepare_query_args has 60 lines of code (exceeds 40 allowed). Consider refactoring.
| */ | ||
| public function get_query_args() { | ||
| protected function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Method prepare_query_args has 101 lines of code (exceeds 40 allowed). Consider refactoring.
| */ | ||
| public function get_query_args() { | ||
| public function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| * @throws \Exception | ||
| */ | ||
| public function get_query_args() { | ||
| public function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 25 (exceeds 5 allowed). Consider refactoring.
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| protected function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
justlevine
changed the title
| public function get_args(): array { | ||
| $args = $this->args; | ||
|
|
||
| protected function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 17 (exceeds 5 allowed). Consider refactoring.
| public function get_args(): array { | ||
| $args = $this->args; | ||
|
|
||
| protected function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Method prepare_args has 48 lines of code (exceeds 40 allowed). Consider refactoring.
| */ | ||
| public function get_query_args() { | ||
| protected function prepare_query_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 40 (exceeds 5 allowed). Consider refactoring.
| $site_plugins = apply_filters( 'all_plugins', get_plugins() ); | ||
| $mu_plugins = apply_filters( 'show_advanced_plugins', true, 'mustuse' ) ? get_mu_plugins() : []; | ||
| $dropin_plugins = apply_filters( 'show_advanced_plugins', true, 'dropins' ) ? get_dropins() : []; | ||
| protected function query( array $query_args ) { |
There was a problem hiding this comment.
Function query has a Cognitive Complexity of 38 (exceeds 5 allowed). Consider refactoring.
|
rebased on |
|
Even with the indeterminate model/dataloader pattern unsolved, I'd love to start backporting some of the nonbreaking changes (and close several existing Issues). @jasonbahl What can I do to make this proposal easier for you to review and give feedback on? |
justlevine
force-pushed
the
proposal/v2-connection-resolver
branch
from
8b6b3e0
to
fb3ddfc
Compare
| @@ -39,13 +32,13 @@ public function get_loader_name() { | |||
| * | |||
| * @throws \Exception | |||
| */ | |||
| public function get_query_args() { | |||
| public function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Method prepare_query_args has 70 lines of code (exceeds 40 allowed). Consider refactoring.
| public function get_args(): array { | ||
| $args = $this->args; | ||
|
|
||
| protected function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.
| * | ||
| * @throws \GraphQL\Error\InvariantViolation If the query class is invalid. | ||
| */ | ||
| protected function validate_query_class(): void { |
There was a problem hiding this comment.
Function validate_query_class has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
| @@ -39,13 +32,13 @@ public function get_loader_name() { | |||
| * | |||
| * @throws \Exception | |||
| */ | |||
| public function get_query_args() { | |||
| public function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 26 (exceeds 5 allowed). Consider refactoring.
| public function get_args(): array { | ||
| $args = $this->args; | ||
|
|
||
| public function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
| public function get_args(): array { | ||
| $args = $this->args; | ||
|
|
||
| public function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.
justlevine
force-pushed
the
proposal/v2-connection-resolver
branch
from
c7ad0bf
to
152cb6c
Compare
| @@ -32,13 +32,13 @@ protected function loader_name(): string { | |||
| * | |||
| * @throws \Exception | |||
| */ | |||
| public function get_query_args() { | |||
| protected function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Method prepare_query_args has 70 lines of code (exceeds 40 allowed). Consider refactoring.
| @@ -22,35 +22,35 @@ public function __construct( $source, array $args, AppContext $context, ResolveI | |||
| /** | |||
| * {@inheritDoc} | |||
| */ | |||
| public function get_query_args() { | |||
| protected function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| @@ -40,15 +40,15 @@ public function __construct( $source, array $args, AppContext $context, ResolveI | |||
| /** | |||
| * {@inheritDoc} | |||
| */ | |||
| public function get_query_args() { | |||
| protected function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| @@ -32,13 +32,13 @@ protected function loader_name(): string { | |||
| * | |||
| * @throws \Exception | |||
| */ | |||
| public function get_query_args() { | |||
| protected function prepare_query_args( array $args ): array { | |||
There was a problem hiding this comment.
Function prepare_query_args has a Cognitive Complexity of 26 (exceeds 5 allowed). Consider refactoring.
| public function get_args(): array { | ||
| $args = $this->get_unfiltered_args(); | ||
|
|
||
| protected function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
justlevine
force-pushed
the
proposal/v2-connection-resolver
branch
from
152cb6c
to
eeaed24
Compare
| public function get_args(): array { | ||
| $args = $this->get_unfiltered_args(); | ||
|
|
||
| protected function prepare_args( array $args ): array { |
There was a problem hiding this comment.
Function prepare_args has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.
| public function has_next_page() { | ||
| if ( ! empty( $this->args['first'] ) ) { | ||
| return ! empty( $this->ids ) && count( $this->ids ) > $this->get_query_amount(); | ||
| public function has_next_page(): bool { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| public function has_previous_page() { | ||
| if ( ! empty( $this->args['last'] ) ) { | ||
| return ! empty( $this->ids ) && count( $this->ids ) > $this->get_query_amount(); | ||
| public function has_previous_page(): bool { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
|
The next batch of backports is up, focusing on improving DX and instantiating class properties only once:
Once this batch is merged, I'll do one more sweep, but I think we're getting close to being able to backport as many of these changes as possible without breaking backwards-compatibility. cc @jasonbahl |
|
Code Climate has analyzed commit 025d45e and detected 17 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 explores what an "ideal"
ConnectionResolverDX would look like if we didnt need to worry about breaking changes.The
AbstractConnectionResolveris an essential yet complex part of the codebase and API. The existing class complicates dx and makes it hard to extend, and also obfuscates what a holistic Connection Resolver would look like without the tech debt.The goal is to is to use this as a springboard to map out a path to a theoretical breaking release and the nonbreaking improvements we can already make.
Guiding Principles
Implementation Details
For better reviewability, changes have been made in situ to
AbstractConnectionResolver, and aConnectionResolverShimexists separately to illustrate the necessary changes from the existing class.The gist of these changes boil down to splitting existing methods into those that handle the actual logic, and wrappers that filter the data and set the properties.
A working set of public getter methods allows for simpler yet more powerful interaction with the resolver instance (either in a filter or in a
resolvecallback).There is also a doc at
/Data/Connection/connection-resolvers.mdto explain the tricky lifecycle and best ways to interact with / extend the class.To review
Note
Changes cherrypicked from this branch into separate PRs are rebased back into this PR.
To see the relevant diff of remaining changes visit https://github.com/wp-graphql/wp-graphql/pull/2749/files/7415eca0ebd3f24c6a5c5e64289be655431bac50..217135b426d039fb95a407d31ed0e574608a533b
To do
AppContext.resolveFn()).Does this close any currently open issues?
and hopefully a few others both here and downstream.
Any relevant logs, error output, GraphiQL screenshots, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Any other comments?
ConnectionResolverShimis not a full working shim. Method signatures (visibility, arg/return types) need to be updated in the implementing class for it to work. In a true backwards-compatible shim, we would most likely makeAbstractConnectionResolverour shim, and move the v2 to a newConnectionResolverclass.Where has this been tested?
Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.1.14)
WordPress Version: 6.4.3