Skip to content

Conversation

@justlevine
Copy link
Collaborator

@justlevine justlevine commented May 4, 2024

What does this implement/fix? Explain your changes.

This PR improves query handling in AbstractConnectionResolver by making the following changes:

  • Implement the new 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 new graphql_connection_query_class` filter before being stored on the class property.
    • If ::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->queryClass is set, but the Resolver doesn't use ::query_class().
  • Implement the new ::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 instantiate AbstractConnectionResolver::$query only once.
  • ::get_query() calls the new AbstractConnectionResolver::query() method, which if not overloaded by a child class, instantiates a new ::$query_class().
  • A new graphql_connection_pre_get_query filter has been added to allow extenders to short-circuit the ::query() logic without extending the class.

Less importantly,

  • AbstractConnectionResolver now 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 @extends doc-block comment on the Connection Resolver class, instead of needing to overload the typehints for all the relevant properties and methods.
  • Usage of generic Exceptions have been replaced with an InvariantException when 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

  • Add tests using $context->queryClass
  • Manually test against WPGraphQL for Woocommerce for possible regressions.

Does 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::$query or ::get_query_method()

image

Any other comments?

Backwards compatibility is ensured by reinstantiating ::$query and manually applying the graphql_connection_query filter in AbstractConnectionResolver::execute_and_get_ids(). Similarly, direct calls to ::$query in 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

@justlevine
Copy link
Collaborator Author

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).

@justlevine justlevine force-pushed the feat/refactor-query-handling branch from 6c05e8b to b8dd6a6 Compare May 4, 2024 17:37
*
* @throws \GraphQL\Error\InvariantViolation If the query class is invalid.
*/
protected function validate_query_class(): void {

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.

@coveralls
Copy link

coveralls commented May 4, 2024

Coverage Status

coverage: 84.359%. remained the same
when pulling 0a75ffa on justlevine:feat/refactor-query-handling
into 6c7cb70 on wp-graphql:develop.

@justlevine justlevine added type: enhancement Improvements to existing functionality status: in review Awaiting review before merging or closing needs: reviewer response This needs the attention of a codeowner or maintainer component: connections Relating to GraphQL Connections scope: api Issues related to access functions, actions, and filters labels May 4, 2024
@justlevine justlevine changed the title refactor: improve query handling in AbstractConnectionResolver [WIP] refactor: improve query handling in AbstractConnectionResolver May 4, 2024
@justlevine justlevine added the needs: tests Tests should be added to be sure this works as expected label May 4, 2024
@justlevine justlevine marked this pull request as ready for review May 4, 2024 22:19
@justlevine
Copy link
Collaborator Author

Tests suites passing for wp-graphql-headless-login, wp-graphql-gravity-forms, wp-graphql-facetwp, and wp-graphql-rank-math.

  • @kidunot89 can you use this branch against the wp-graphql-woocommerce test suite? I've never been able to get the test suite to run locally...

jasonbahl
jasonbahl previously approved these changes May 8, 2024
@justlevine justlevine dismissed jasonbahl’s stale review May 8, 2024 17:27

The merge-base changed after approval.

@qlty-cloud-legacy
Copy link

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

Here's the issue category breakdown:

Category Count
Complexity 3
Duplication 2

View more on Code Climate.

@jasonbahl jasonbahl merged commit 97af181 into wp-graphql:develop May 8, 2024
@justlevine justlevine deleted the feat/refactor-query-handling branch May 8, 2024 19:23
@jasonbahl jasonbahl mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: connections Relating to GraphQL Connections needs: reviewer response This needs the attention of a codeowner or maintainer needs: tests Tests should be added to be sure this works as expected scope: api Issues related to access functions, actions, and filters status: in review Awaiting review before merging or closing type: enhancement Improvements to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Acquiring the WP_Query instance within a wp-graphql filter causes post hooks to fire again Overriding/subclassing the default object connection types

3 participants