Skip to content

chore: refactor AbstractConnectionResolver::$args handling#2521

Merged
jasonbahl merged 2 commits intowp-graphql:developfrom
justlevine:feat/add_graphql_connection_args_filter_to_resolver
Sep 19, 2022
Merged

chore: refactor AbstractConnectionResolver::$args handling#2521
jasonbahl merged 2 commits intowp-graphql:developfrom
justlevine:feat/add_graphql_connection_args_filter_to_resolver

Conversation

@justlevine
Copy link
Copy Markdown
Collaborator

What does this implement/fix? Explain your changes.

This PR deprecates the AbstractConnectionResolver::getArgs() in favor of AbstractConnectionResolver::get_args(), which is passed through the graphql_connection_args WP filter on instantiation.

This PR replaces part of #2340 , and lays the foundation to create the individual PRs necessary to close #998 in a piecemeal approach, by allowing child ConnectionResolver classes and 3rd-party code to manipulate connection arg values. before they are used in the resolver.

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?

  • As the functionality will be tested in future PRs, I opted to avoid the redundant work of adding unit tests.

Where has this been tested?

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

WordPress Version: 6.0.2

@justlevine justlevine added type: enhancement Improvements to existing functionality component: connections Relating to GraphQL Connections scope: api Issues related to access functions, actions, and filters status: in review Awaiting review before merging or closing labels Sep 17, 2022
@qlty-cloud-legacy
Copy link
Copy Markdown

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

View more on Code Climate.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 17, 2022

Coverage Status

Coverage decreased (-0.005%) to 81.797% when pulling 128e7c3 on justlevine:feat/add_graphql_connection_args_filter_to_resolver into a2098ea on wp-graphql:develop.

*
* @since @todo
*/
$this->args = apply_filters( 'graphql_connection_args', $this->get_args(), $this );
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think I'm ok with this filter being added, but it might be a bit naiive as it only targets connections, and not any type of field.

I was thinking it would make sense to allow filtering args on any field, not just connections.

Like described here: #965 (comment)

I think it would be nice to be able to filter args before they're used in resolution, but also do something with the args after resolution but before the response is returned.

Anyway, I think we can approach the bigger picture separately. This works for connections and has immediate benefits for working with connections.

*
* Deprecated in favor of $this->get_args();
*
* @deprecated @todo
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can update the deprecated version/message when release is prepped. 👍🏻

@jasonbahl jasonbahl merged commit 9998de6 into wp-graphql:develop Sep 19, 2022
@justlevine justlevine deleted the feat/add_graphql_connection_args_filter_to_resolver branch September 19, 2022 16:49
@jasonbahl jasonbahl mentioned this pull request Sep 19, 2022
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 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.

Update Resolvers for all ID inputs to behave consistently

3 participants