feat: Refactor NodeResolver::resolve_uri() to use WP_Query #2680

justlevine · 2022-12-17T14:28:26Z

What does this implement/fix? Explain your changes.

This pr refactors NodeResolver::resolve_uri() to use WP_Query, instead of the current approach where we try to replicate the class logic locally.

This brings the resolution process more in line with WP::main(), mitigating edge cases and reducing code complexity.

In addition, it adds the following WP filters to the method:

graphql_resolve_uri_query_class . Used to swap out WP_Query with a specialized class, e.g. WC_Query from woo.
graphql_resolve_uri . Used to short-circuit resolve_uri() after the query has been run, but before our own attempts to resolve it to a WPGraphQL node, e.g. if we want a CPT to use a special dataloader.
graphql_post_resolve_uri. Used to provide a WPGraphQL node if resolve_uri() is unable to do so, e.g. a plugin-specific Edge case.

Does this close any currently open issues?

fixes #2178
fixes #2190
closes #1910
(possibly others)

Part of #2366

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?

This PR includes test: backfill nodeByUri query testing #2659, which should be merged first.
Since we're no longer routing via our own conditionals, $extra_query_vars['nodeType'] and $extra_query_vars['taxonomy'] are now set in more places to let us validate the type we're receiving is compatible with the GraphQL type we resolve to.
I removed nodeByUri tests for when pretty permalinks are disabled for now. The reason is that parse_request() (from before this PR) marks those as a 404, which we were able to ignore when we were using our own logic before calling WP_Query, but no longer. I believe this is a quirk with Codeception, and not an issue with the code itself, and I am looking into workarounds to reinstate those tests.
Before merging, we should test with popular WPGraphQL Extensions, just in case they're doing something funky to work around the existing bugs.
While I dont expect there to be any performance hits (if anything, maybe a small perf improvement because WP_Query results are usually cached, and we're hitting it earlier in the lifecycle), but we should do some profiling to be 100% sure. This is beyond my skillset.
Date archives still resolve null, since without Introduce Archive Interfaces #2491 we have nowhere to resolve them to. We could put a graphql_debug() saying theyre not currently supported, and fallback to the ContentType, but there isnt precedent for that in the codebase.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php8.0.19)

WordPress Version: 6.1.1

…1639 )

This is a codeception quick we need to work around.

codeclimate · 2022-12-17T14:28:53Z

src/Data/NodeResolver.php

-				$page_on_front = get_option( 'page_on_front', 0 );
-				$post          = get_post( absint( $page_on_front ) );
-				return ! empty( $post ) ? $this->context->get_loader( 'post' )->load_deferred( $post->ID ) : null;
+				return ! empty( $post_type_object->name ) ? $this->context->get_loader( 'post_type' )->load_deferred( $post_type_object->name ) : null;


Avoid too many return statements within this method.

codeclimate · 2022-12-17T14:28:53Z

src/Data/NodeResolver.php

-				$post = isset( $post->ID ) ? $this->validate_post( $post ) : null;
-				return isset( $post->ID ) ? $this->context->get_loader( 'post' )->load_deferred( $post->ID ) : null;
-			}
+			return ! empty( $queried_object->ID ) ? $this->context->get_loader( 'post' )->load_deferred( $queried_object->ID ) : null;


Avoid too many return statements within this method.

codeclimate · 2022-12-17T14:28:53Z

src/Data/NodeResolver.php

-			$post_type_object = get_post_type_object( $this->wp->query_vars['post_type'] );
+		// Resolve Terms.
+		if ( $queried_object instanceof \WP_Term ) {
+			return ! empty( $queried_object->term_id ) ? $this->context->get_loader( 'term' )->load_deferred( $queried_object->term_id ) : null;


Avoid too many return statements within this method.

codeclimate · 2022-12-17T14:28:54Z

src/Data/NodeResolver.php

-			return ! empty( $post_type_object ) ? $this->context->get_loader( 'post_type' )->load_deferred( $post_type_object->name ) : null;
+		// Resolve Post Types.
+		if ( $queried_object instanceof \WP_Post_Type ) {
+			return ! empty( $queried_object->name ) ? $this->context->get_loader( 'post_type' )->load_deferred( $queried_object->name ) : null;


Avoid too many return statements within this method.

codeclimate · 2022-12-17T14:28:54Z

src/Data/NodeResolver.php

-			}
+		// Resolve Users
+		if ( $queried_object instanceof \WP_User ) {
+			return ! empty( $queried_object->ID ) ? $this->context->get_loader( 'user' )->load_deferred( $queried_object->ID ) : null;


Avoid too many return statements within this method.

coveralls · 2022-12-17T15:45:31Z

Coverage increased (+3.4%) to 84.861% when pulling 47fe8ab on justlevine:feat/NodeResolver_resolve_uri-refactor into 6ae12df on wp-graphql:develop.

codeclimate · 2022-12-17T16:17:25Z

src/Data/NodeResolver.php

-		}
-
-		return $node;
+		return apply_filters( 'graphql_post_resolve_uri', $node, $uri, $queried_object, $query, $this->context, $this->wp, $extra_query_vars );


Avoid too many return statements within this method.

codeclimate · 2022-12-17T20:55:55Z

src/Type/ObjectType/RootQuery.php

@@ -217,7 +217,12 @@ public static function register_type() {
 							$idType = $args['idType'] ?? 'global_id';
 							switch ( $idType ) {
 								case 'uri':
-									return $context->node_resolver->resolve_uri( $args['id'] );
+									return $context->node_resolver->resolve_uri(


Avoid too many return statements within this method.

codeclimate · 2022-12-17T20:55:55Z

src/Type/ObjectType/RootQuery.php

@@ -527,7 +532,12 @@

 									break;
 								case 'uri':
-									return $context->node_resolver->resolve_uri( $args['id'] );
+									return $context->node_resolver->resolve_uri(


Avoid too many return statements within this method.

codeclimate · 2022-12-17T20:55:55Z

src/Type/ObjectType/RootQuery.php

@@ -584,7 +594,12 @@
 									$id = absint( $args['id'] );
 									break;
 								case 'uri':
-									return $context->node_resolver->resolve_uri( $args['id'] );
+									return $context->node_resolver->resolve_uri(


Avoid too many return statements within this method.

codeclimate · 2022-12-17T21:35:02Z

src/Data/NodeResolver.php

+	 *
+	 * @return \WP_Term|null
+	 */
+	public function validate_term( \WP_Term $term ) {


Function validate_term has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2022-12-17T21:55:41Z

Code Climate has analyzed commit 47fe8ab and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	10

View more on Code Climate.

justlevine · 2022-12-17T21:59:59Z

Verifying #1910 is resolved:

Install and Activate Woocommece
Add some dummy products.
Query nodeByUri for "\shop". This returns null, since by default products are not exposed to the schema

{
  nodeByUri(uri: "/shop") {
    __typename
    ... on ContentType {
      name
      contentNodes {
        nodes {
          __typename
          databaseId
        }
      }
    }
  }
}

Results:

Expose WC Products to the graphql schema:

add_filter( 'register_post_type_args',
  function( array $args, string $post_type ) {
    if ( 'product' === $post_type ) {
      $args['show_in_graphql'] = true;
      $args['graphql_single_name'] = 'product';
      $args['graphql_plural_name'] = 'products';
    }
    return $args;
  },
10, 2 );

Rerun the query. This will return a ContentType, since /shop is a product archive.
Result:
Attempt to fetch the query as a Page. This will return null, since /shop is a product archive.

{
  page(id: "/shop", idType: URI) {
    __typename
    databaseId
  }
}

Result:

justlevine added 27 commits December 16, 2022 16:39

tests: cleanup existing

0030e0f

tests: test nodeByUri for user

821f194

tests: testNodeByUri for post formats (wp-graphql#2910)

c147c15

tests: test nodeByUri for date archives (wp-graphql#2191)

3c25d92

tests: test nodeByUri for attachments ( wp-graphql#2178 )

ac9dd91

tests: tests posts and pages with(out) permalink prefix ( wp-graphql#…

6448576

…1639 )

tests: test nodeByUri with a post url with an anchor

9bd6ea1

tests: add more anchor test cases

24666c6

test: test NodeByUri posts with subpages

7435534

tests: test nodeByUri for post slug conflicts

4668811

chore: test nodeByUri for updated slugs

75523c3

tests: test nodeByUri for hierarchical terms

89852d7

tests: unregister test_hierarchical tax after test

eca51ce

test: fix hierarchical term links and test identical slugs

ebbe1e6

test: unregister tax before marking incomplete

3620059

tests: cleanup and organize NodeByUri tests

6b92385

tests: test NodeByUri for cpt archives

56abd3c

tests: test nodeByUri for default tax terms

dee1c53

tests: test nodeByUri for default Category

9f0f22c

tests: test nodeByUri for cpt with base url

548979a

tests: test nodeByUri for term with base url

fa10c04

tests: test nodeByUri for custom rewrite rule

6ae13e8

tests: set show_on_front when testing page_for_posts

a0a712d

tests: fail if nodeByUri is null instead of throwing error

459c8ec

test: (temp?) remove disabling pretty permalinks mid test

d78e5f1

This is a codeception quick we need to work around.

tests: show_on_front must be set with page_for_posts

9d3217c

feat: use WP_Query to resolve URIs

4f24018

codeclimate bot reviewed Dec 17, 2022

View reviewed changes

justlevine added 2 commits December 17, 2022 15:22

fix: validate posts before returning

9f51ede

test: create_initial_post_types when changing permastruc

e2de329

justlevine added 2 commits December 17, 2022 15:31

tests: unmark tests fixed by refactor

0ca1d04

tests: cleanup

bc868c6

justlevine mentioned this pull request Dec 17, 2022

🎯 Tracking: Resolving nodes by URI #2366

Closed

16 tasks

justlevine added Type: Enhancement New feature or request Architecture Status: In Progress labels Dec 17, 2022

dev: add lifecycle filters

f36de36

codeclimate bot reviewed Dec 17, 2022

View reviewed changes

justlevine marked this pull request as ready for review December 17, 2022 16:28

justlevine requested review from jasonbahl, renatonascalves and kidunot89 December 17, 2022 16:28

justlevine mentioned this pull request Dec 17, 2022

fix(resolver): check if name is an array #2642

Closed

fix: use nodeType to prevent resolving objects as wrong type

dbb88e5

codeclimate bot reviewed Dec 17, 2022

View reviewed changes

fix: categories/tags use an abbreviated query_var key

1775848

codeclimate bot reviewed Dec 17, 2022

View reviewed changes

dev: revert last, and use taxonomy on query_vars

47fe8ab

idflood mentioned this pull request Jan 20, 2023

GraphQL WordPress Plugin Performance Issue #1873

Closed

5 tasks

jasonbahl approved these changes Feb 15, 2023

View reviewed changes

jasonbahl merged commit 734d544 into wp-graphql:develop Feb 15, 2023

This was referenced Mar 2, 2023

Release/v1.14.0 #2746

Closed

release: v1.14.0 #2748

Merged

justlevine mentioned this pull request Mar 7, 2023

fix: handle 404s in NodeResolver.php #2752

Merged

justlevine deleted the feat/NodeResolver_resolve_uri-refactor branch February 10, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor NodeResolver::resolve_uri() to use WP_Query #2680

feat: Refactor NodeResolver::resolve_uri() to use WP_Query #2680

justlevine commented Dec 17, 2022 •

edited

Loading

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

coveralls commented Dec 17, 2022 •

edited

Loading

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot Dec 17, 2022

codeclimate bot commented Dec 17, 2022

justlevine commented Dec 17, 2022 •

edited

Loading

feat: Refactor NodeResolver::resolve_uri() to use WP_Query #2680

feat: Refactor NodeResolver::resolve_uri() to use WP_Query #2680

Conversation

justlevine commented Dec 17, 2022 • edited Loading

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

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

Any other comments?

Where has this been tested?

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

coveralls commented Dec 17, 2022 • edited Loading

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 17, 2022

Choose a reason for hiding this comment

codeclimate bot commented Dec 17, 2022

justlevine commented Dec 17, 2022 • edited Loading

justlevine commented Dec 17, 2022 •

edited

Loading

coveralls commented Dec 17, 2022 •

edited

Loading

justlevine commented Dec 17, 2022 •

edited

Loading