Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: improve query handling in AbstractConnectionResolver #3125

Merged

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 {
Copy link

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.

src/Data/Connection/UserConnectionResolver.php Outdated Show resolved Hide resolved
src/Data/Connection/TermObjectConnectionResolver.php Outdated Show resolved Hide resolved
src/Data/Connection/MenuItemConnectionResolver.php Outdated Show resolved Hide resolved
src/Data/Connection/MenuItemConnectionResolver.php Outdated Show resolved Hide resolved
@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 New feature or request Status: In Review Needs to be reviewed by a project maintainer before merge Needs: Reviewer Response This needs the attention of a codeowner or maintainer. Component: Connections Issues related to 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 ensure 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.

Copy link

codeclimate bot commented May 8, 2024

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
29 of 30 checks passed
@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 Issues related to connections Needs: Reviewer Response This needs the attention of a codeowner or maintainer. Needs: Tests Tests should be added to ensure this works as expected scope: API Issues related to access functions, actions, and filters Status: In Review Needs to be reviewed by a project maintainer before merge Type: Enhancement New feature or request
Projects
None yet
3 participants