feat: Allow global/db IDs in connection where args#2340
feat: Allow global/db IDs in connection where args#2340justlevine wants to merge 7 commits intowp-graphql:developfrom
Conversation
| * | ||
| * @return array | ||
| */ | ||
| public function get_args() { |
There was a problem hiding this comment.
Function get_args has a Cognitive Complexity of 18 (exceeds 5 allowed). Consider refactoring.
There was a problem hiding this comment.
There's definitely a way to simplify this - open to suggestions. My focus was providing a working example of implementation.
|
@jasonbahl let me know if this approach looks good to you, and if so whether I should move |
| * | ||
| * @return array | ||
| */ | ||
| public function get_args() { |
There was a problem hiding this comment.
Function get_args has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.
|
Related: #1779 |
1399f17 to
49e39d5
Compare
|
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. |
49e39d5 to
2c1e3fb
Compare
| ], | ||
| 'description' => __( 'Specify IDs NOT to retrieve. If this is used in the same query as "in", it will be ignored', 'wp-graphql' ), | ||
| 'parseValue' => function ( $value ) { | ||
| return Utils::parse_input_ids( $value ); |
There was a problem hiding this comment.
Avoid too many return statements within this method.
| ], | ||
| 'description' => __( 'Find objects connected to author(s) in the array of author\'s userIds', 'wp-graphql' ), | ||
| 'parseValue' => function ( $value ) { | ||
| return Utils::parse_input_ids( $value ); |
There was a problem hiding this comment.
Avoid too many return statements within this method.
| ], | ||
| 'description' => __( 'Find objects NOT connected to author(s) in the array of author\'s userIds', 'wp-graphql' ), | ||
| 'parseValue' => function ( $value ) { | ||
| return Utils::parse_input_ids( $value ); |
There was a problem hiding this comment.
Avoid too many return statements within this method.
| ], | ||
| 'description' => __( 'Array of category IDs, used to display objects from one category OR another', 'wp-graphql' ), | ||
| 'parseValue' => function ( $value ) { | ||
| return Utils::parse_input_ids( $value ); |
There was a problem hiding this comment.
Avoid too many return statements within this method.
| ], | ||
| 'description' => __( 'Array of category IDs, used to display objects from one category OR another', 'wp-graphql' ), | ||
| 'parseValue' => function ( $value ) { | ||
| return Utils::parse_input_ids( $value ); |
There was a problem hiding this comment.
Avoid too many return statements within this method.
|
Code Climate has analyzed commit e4683a0 and detected 13 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
Updates:
@jasonbahl : With your approval, I will close this in favor of a series of PRs: first the new filter and method in |
What does this implement/fix? Explain your changes.
This PR allows GraphQL connection where args with a type
IDto accept either global IDs or database IDs.The method proposed is non-breaking, works uniformly across all connections, and is easily extendable both by custom connection resolvers and with WP filters.
It follows the pattern used by
AbstractConnectionResolver::get_query_args().As with other PRs addressing #998 , this can be implemented piece-meal for each connection, but only once the primary change to
AbstractConnectionResolverhas been changed. For this reason, I've also includedCommentsConnectionResolverin the PR, and can either break that to a separate PR or add the other updated resolvers to this one. 🤷How it works:
AbstractConnectionResolver::$argsis now passed through thegraphql_connection_argsfilter.AbstractConnectionResolver::getArgs()has been deprecated in favor ofAbstractConnectionResolver::get_args(), which provides a way for resolvers to modify the connection args before they are passed toAbstractConnectionResolver::get_query_args().CommentConnectionResolver::get_args()to map all inputs of typeIDto their database ID.Alternatives considered:
parseValuesconfig callable.The theory is that using
parseValueson the input config, would allow us to improve separation of concerns, at the cost of significant code duplication. However, I either misunderstood the GraphQL-PHP docs, or this isnt currently supported by WPGraphQL. (You can see this approach implemented - and failing! - forData\Connection\PostObjectshere ).Does this close any currently open issues?
#998
Any other comments?
TheNow working.graphql_connection_argsfilter only passes the$argsparameter, as the class instance is not in a place yet where its public methods can be called consistently.Tests are going to be a huge
PITAopportunity to improve coverall scores, since most connections dont currently have many of these where args tested. Could use help implementing these.the version number on the deprecations and new filters will need to be updated before release
Where has this been tested?
Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.0.19)
WordPress Version: 6.0.1