Skip to content

chore: refactor NodeResolver#2342

Closed
justlevine wants to merge 2 commits intowp-graphql:developfrom
justlevine:feat/refactor-resolve_uri
Closed

chore: refactor NodeResolver#2342
justlevine wants to merge 2 commits intowp-graphql:developfrom
justlevine:feat/refactor-resolve_uri

Conversation

@justlevine
Copy link
Copy Markdown
Collaborator

@justlevine justlevine commented Apr 11, 2022

What does this implement/fix? Explain your changes.

Early work on refactoring NodeResolver to make it easier to work on going forward and address the assortment of reported bugs.

In a nutshell:

  • the code replicating WP::parse_request() has been moved from resolve_uri() to parse_request()
  • resolve_uri() now uses various resolve_{type}() methods to keep the logic dry.
  • visible post types and taxonomies are now saved as instance params to prevent unnecessary db hits.

Does this close any currently open issues?

Any other comments?

  • There are several parts of the code (marked by @todo) that I need to better understand before marking this ready for review.
  • We should do an audit of various possible queries to ensure everything that currently resolves correctly still works. While tests are passing and the codecov for the relevant parts is the same, my gut tells me there's more use cases that need wpunit tests.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.0.15)

WordPress Version: 5.9.3

@justlevine justlevine added component: query Relating to GraphQL Queries needs: tests Tests should be added to be sure this works as expected lang: php Pull requests that update PHP code scope: code quality Refactoring, linting, and enforcing coding standards labels Apr 11, 2022
} else {
$resolved = $this->resolve_taxonomy_term();
if ( false !== $resolved ) {
return $resolved;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

$post_type = $this->wp->query_vars['post_type'];
}

return $this->resolve_post_by_slug( $this->wp->query_vars['name'], $post_type );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.


return $this->resolve_post_by_slug( $this->wp->query_vars['name'], $post_type );
} elseif ( isset( $this->wp->query_vars['cat'] ) ) {
return $this->resolve_term_by_id( $this->wp->query_vars['cat'], 'category' );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

} elseif ( isset( $this->wp->query_vars['cat'] ) ) {
return $this->resolve_term_by_id( $this->wp->query_vars['cat'], 'category' );
} elseif ( isset( $this->wp->query_vars['tag'] ) ) {
return $this->resolve_term_by_slug( $this->wp->query_vars['tag'], 'post_tag' );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 11, 2022

Coverage Status

Coverage increased (+0.04%) to 79.911% when pulling ad69342 on justlevine:feat/refactor-resolve_uri into 2054b01 on wp-graphql:develop.

}

// If we're still here, the query wants a post type object.
return $this->resolve_post_type( $this->wp->query_vars['post_type'] );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

// Search all post types if none is set.
$post_type = isset( $this->wp->query_vars['post_type'] ) ? $this->wp->query_vars['post_type'] : $this->get_allowed_post_types();

return $this->resolve_post_by_slug( $this->wp->query_vars['pagename'], $post_type );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

// }
// return $post;
} elseif ( isset( $this->wp->query_vars['author_name'] ) ) {
return $this->resolve_user_by_slug( $this->wp->query_vars['author_name'] );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

// If the query is asking for a Page nodeType with the uri, try and resolve it.
if ( isset( $this->wp->query_vars['nodeType'] ) && 'Page' === $this->wp->query_vars['nodeType'] && isset( $this->wp->query_vars['uri'] ) ) {
$post_type = $this->wp->query_vars['post_type'];
return $this->resolve_post_by_slug( $this->wp->query_vars['uri'], $post_type );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

}

$page_on_front = get_option( 'page_on_front', 0 );
return $this->resolve_post_by_id( absint( $page_on_front ) );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

if ( ! $term instanceof \WP_Term ) {
return null;
}
return $this->resolve_term_by_id( $term->term_id, 'category' );
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

*
* @return string|null The parsed uri.
*/
public function parse_request( string $uri, $extra_query_vars = '' ) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parse_request has a Cognitive Complexity of 112 (exceeds 5 allowed). Consider refactoring.

@justlevine justlevine force-pushed the feat/refactor-resolve_uri branch from 271d0c1 to ad69342 Compare May 17, 2022 23:41
@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit ad69342 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 12

View more on Code Climate.

@stale
Copy link
Copy Markdown

stale bot commented Aug 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 2, 2022
@stale
Copy link
Copy Markdown

stale bot commented Sep 1, 2022

This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue.

@stale
Copy link
Copy Markdown

stale bot commented Dec 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale? May need to be revalidated due to prolonged inactivity label Dec 1, 2022
@justlevine justlevine closed this Dec 7, 2022
@justlevine justlevine deleted the feat/refactor-resolve_uri branch February 10, 2024 19:38
@justlevine justlevine restored the feat/refactor-resolve_uri branch February 10, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: query Relating to GraphQL Queries lang: php Pull requests that update PHP code needs: tests Tests should be added to be sure this works as expected scope: code quality Refactoring, linting, and enforcing coding standards stale? May need to be revalidated due to prolonged inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants