feat: AbstractConnectionResolver:set_query_class() created #2821

kidunot89 · 2023-05-23T21:40:15Z

What does this implement/fix? Explain your changes.

While it started as a fix for the queryClass connection registration argument which is broken atm of creation in WPGraphQL v1.14.3, this PR ultimately does not fix queryClass but instead make the query class something to be set on the resolver level instead of the registration level. There is some functionality being lost here but it was actually removed a long time ago by @jasonbahl. Then patch back in by myself, @kidunot89, but in a really hacky way. Then broken again by
@justlevine. And this will be the introduction of something new but achieves the same desired effect as the queryClass argument.

Adds query_class and set_query_class() to AbstractConnectionResolver class.
Updates Comment, Post, User, and Term connection resolver classes to source the $query_class.
Adds testRegisteringConnectionsWithCustomQueryClasses to the ConnectionRegistrationTest case
WP_Query_Custom added to tests/_support/utils/class-wp-query-custom.php for testing.

Example registration call using the new functionality.

register_graphql_connection([
	'fromType'      => 'RootQuery',
	'toType'        => 'Post',
	'fromFieldName' => 'customPosts',
	'resolve'       => function ( $source, $args, $context, $info ) {
		$resolver = new \WPGraphQL\Data\Connection\PostObjectConnectionResolver( $source, $args, $context, $info, 'post' );
		$resolver->set_query_class( \WP_Query_Custom::class );
		return $resolver->get_connection();
	},
]);

Does this close any currently open issues?

…

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?

…

Where has this been tested?

Operating System: …

WordPress Version: …

justlevine · 2023-05-24T07:03:01Z

First off, love this! (@renatonascalves just suggested something like this so the timing is perfect! )

Couple questions (for context 😬) before I look at the actual code:

While it started as a fix for the queryClass connection registration argument which is broken atm of creation in WPGraphQL v1.14.3.

Can you be more specific about what's currently broken?

There is some functionality being lost here

What specifically are we losing (and do you mean vs what's currently working or what's currently supposed to work but is anyway broken (per 1).

The last several attempts at isolated fixes/enhancements to the Connection Resolver all resulted in regressions around cases we don't currently have unit tests for. To try and avoid that from happening again, I've been working to improve the class holistically in #2749 from which we would then backport the individual changes in a nonbreaking manner.

My recommendation for this would be the same if it's "just" an enhancement and not tackling a specific regression.

src/Data/Connection/AbstractConnectionResolver.php

justlevine · 2023-05-24T09:32:48Z

src/Data/Connection/AbstractConnectionResolver.php

+	 *
+	 * @return void
+	 */
+	public function set_query_class( $class ) {


I've got concerns/questions from a "holistic" POV - nothing that follows should be considering blocking since the same issues exist with the current method of setting the query class via WPConnectionType.

I love the fact that this pattern makes local overloading in a resolver possible, and brings Comment/User connection queries the same extensibility patten, but I'm concerned about its availability on AbstractConnectionResolver, when only 4 (including those in this PR) out of the bundled 13 Connection resolvers actually use a $query_class. Unlike when it was buried in WPConnectionType, the new pattern will see more (mis-)use, so I think it's an opportunity to consider this aspect of the DX.

Similarly, I'm concerned around the lack of checks around the provided $query_class. We dont want to enforce a WP_Query child, but we do need to make sure that the provided class string (SWP_Query, WC_Query, Tribe_Query, FWP_Query, etc etc) is actually compatible with the Connection resolver relying on it.

The latter issue can be solved with an overloadable is_valid_query_class( string $class ) that could for example check for method_exists( $class, 'query' ).

We could even in theory use the same is_valid_query_class() to throw an exception if ! empy( $this->query_class ) on the Connection Resolvers that don't use it (would need to figure out backwards-compatibility), but I think we should first confirm that AbstractConnectionResolver is really the ideal home for this instead of adding what's possibly tech debt to the majority of the included connection resolvers.

Actually it's more like 6 or 7 out of 13, because they extend one of the 4 connection resolvers and not the AbstractConnectionResolver class directly, Some of them don't even have a get_query() function and default to parent get_query() function in one of the 4. I agree there should be class_exists( $this->query_class ) check somewhere and a is_valid_query_class() function is a good idea. I'll add it immediately.

because they extend one of the 4 connection resolvers and not the AbstractConnectionResolver class directly, Some of them don't even have a get_query() function and default to parent get_query() function in one of the 4.

Actual numbers aside ☝️is the crux of my concern: we're exposing a setter that semantically should apply on any connection but will be ignored by most of them - something that becomes less obvious from the code the better we are at DRYing out the implementing classes. But I don't have a good alternative 🤔

The only clean solution I can think of would be make two child classes of the AbstractConnectionResolver that all other extend. One would be something like QueryConnectionResolver and the other would be something like ConnectionResolver. You get the picture 🤷🏿‍♂️

Or breaking up AbstractConnectionResolver into traits, but I think either is bad DX in this particular situation since it extenders then need to know which intermediate class / trait is right for a particular WP type, but I don't see a way to have those classes/traits to use existing WP namings. 🤷‍♂️

Exactly, but the connection resolvers not using $query_class are highly unlikely to be extended for usage. EnqueuedScripts, EnqueuedStylesheet, Plugin, Taxonomy, Theme, UserRole, and ContentType. So honestly we should be fine.

justlevine · 2023-05-24T09:33:50Z

src/Data/Connection/CommentConnectionResolver.php

 	 */
 	public function get_query() {
-		return new WP_Comment_Query( $this->query_args );
+		// Get query class.
+		$queryClass = ! empty( $this->query_class )


Seeing this alone makes me way happier than it has any right to. 🙌🙌🙌

justlevine · 2023-05-24T09:41:08Z

tests/wpunit/ConnectionRegistrationTest.php

@@ -677,4 +679,45 @@ protected function assertValidTypes( $actual ) : void {
 		$this->assertEquals( 'TestObject', $actual['data']['testConnection']['nodes'][0]['__typename'] );
 	}

+	public function testRegisteringConnectionsWithCustomQueryClasses() {


We should also have tests for some common edge cases: bad class name, incompatible class, and using the wrong WP_{$type}_Query are the ones that jump to mind.

justlevine · 2023-05-24T09:42:26Z

tests/wpunit/_bootstrap.php

@@ -0,0 +1,3 @@
+<?php
+
+require __DIR__ . '/../_support/Utils/class-wp-query-custom.php';


Can we do this via the autoloader instead?

Tried too, and it didn't work. It was namespaced and everything. For some reason GraphQL wouldn't see the class and would throw so I eventually devolve into that in the bootstrap file, and I just removed all my PSR dev configuration.

Feel free to take a crack at after I make your requested changes.

codeclimate

The PR diff size of 9896 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 9958 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 9956 lines exceeds the maximum allowed for the inline comments feature.

justlevine · 2023-05-24T16:34:41Z

Couple questions (for context 😬) before I look at the actual code:

While it started as a fix for the queryClass connection registration argument which is broken atm of creation in WPGraphQL v1.14.3.

Can you be more specific about what's currently broken?

There is some functionality being lost here

What specifically are we losing (and do you mean vs what's currently working or what's currently supposed to work but is anyway broken (per 1).

Summarized from a Slack Convo with @kidunot89 (correct me if I made a mistake):

In v1.14.3, the custom queryClass passed via AppContext is ignored. This is a regression (for which I believe there is not an existing GH issue).
This PR doesn't address the regression itself, since using AppContext to overload the query type is "hacky". Instead it provides an enhancement, offering a better way to overload the query class (from within the field resolver for the individual connection).

We should probably still address the actual regression in a separate PR.

With this in mind, I'm going to port this over to #2749 - even if we don't decide to wait on that PR merging this, I'm betting the effort will still unearth some code suggestions we can implement here independently.

See wp-graphql#2821

justlevine · 2023-05-25T15:16:44Z

I've updated #2749 to implement this pattern.
This comment details the differences between this PR vs the holistic approach over there.

(If this PR goes forward independently, I highly recommend backporting at least some of the changes).

- add autoload-dev rule - update namespace for WP_Query_Custom class

codeclimate

The PR diff size of 9933 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-06-13T15:29:08Z

Code Climate has analyzed commit a74dcb9 and detected 0 issues on this pull request.

View more on Code Climate.

See wp-graphql#2821

feat: AbstractConnectionResolver:set_query_class() created

8f3beca

kidunot89 requested review from jasonbahl and justlevine May 23, 2023 21:40

kidunot89 added 2 commits May 23, 2023 17:58

chore: Linter compliance met

f166711

chore: Docblocks cleaned up.

d57dbe4

kidunot89 mentioned this pull request May 23, 2023

feat: Improved events filtering on the events query. the-events-calendar/ql-events#54

Merged

1 task

justlevine reviewed May 24, 2023

View reviewed changes

fix: Some error handling implemented and unit test expanded.

aaa871f

codeclimate bot reviewed May 24, 2023

View reviewed changes

kidunot89 requested a review from justlevine May 24, 2023 15:17

fix: connection resolver classes refactored.

d7da12f

codeclimate bot reviewed May 24, 2023

View reviewed changes

chore: Linter compliance met

7f8f2d2

codeclimate bot reviewed May 24, 2023

View reviewed changes

justlevine added a commit to justlevine/wp-graphql that referenced this pull request May 25, 2023

feat: add suport for resolver-level $query_class

aad0106

See wp-graphql#2821

justlevine mentioned this pull request May 25, 2023

refactor!: [WIP] Exploring a ConnectionResolver 2.0 #2749

Draft

8 tasks

- remove bootstrap

4aea602

- add autoload-dev rule - update namespace for WP_Query_Custom class

codeclimate bot reviewed Jun 2, 2023

View reviewed changes

jasonbahl added 2 commits June 13, 2023 09:25

- reset composer.json and composer.lock to the develop state

0ecb285

Merge branch 'develop' into feat/set-query-class

a74dcb9

justlevine added a commit to justlevine/wp-graphql that referenced this pull request Sep 16, 2023

feat: add suport for resolver-level $query_class

4c6e03c

See wp-graphql#2821

jasonbahl added the Status: Discussion Requires a discussion to proceed label Sep 19, 2023

justlevine mentioned this pull request Feb 2, 2024

fix: php deprecation error for dynamic properties on AppContext class #3033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: AbstractConnectionResolver:set_query_class() created #2821

feat: AbstractConnectionResolver:set_query_class() created #2821

kidunot89 commented May 23, 2023 •

edited

justlevine commented May 24, 2023 •

edited

justlevine May 24, 2023 •

edited

kidunot89 May 24, 2023

justlevine May 24, 2023

kidunot89 May 24, 2023

justlevine May 24, 2023

kidunot89 May 24, 2023 •

edited

justlevine May 24, 2023

justlevine May 24, 2023

justlevine May 24, 2023

kidunot89 May 24, 2023

kidunot89 May 24, 2023

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

justlevine commented May 24, 2023

justlevine commented May 25, 2023

codeclimate bot left a comment

codeclimate bot commented Jun 13, 2023

		@@ -0,0 +1,3 @@
		<?php

		require __DIR__ . '/../_support/Utils/class-wp-query-custom.php';

feat: AbstractConnectionResolver:set_query_class() created #2821

Are you sure you want to change the base?

feat: AbstractConnectionResolver:set_query_class() created #2821

Conversation

kidunot89 commented May 23, 2023 • edited

What does this implement/fix? Explain your changes.

Example registration call using the new functionality.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

Where has this been tested?

justlevine commented May 24, 2023 • edited

justlevine May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidunot89 May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

justlevine commented May 24, 2023

justlevine commented May 25, 2023

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Jun 13, 2023

kidunot89 commented May 23, 2023 •

edited

justlevine commented May 24, 2023 •

edited

justlevine May 24, 2023 •

edited

kidunot89 May 24, 2023 •

edited