Analysis results are not available for those commits

View more on Code Climate.

coverage: 84.209% (-0.004%) from 84.213%
when pulling 59e1f4d on fix/set_query_arg_override
into 3a96572 on develop.

Code LGTM, but I'm not entirely sold we want to deprecate/add these in 1.x, when they'll prob need to be changed again after a breaking release, and a safer/more robust pattern is for extenders to overload the GraphQL args over the WPQuery args.

Will respond/elaborate in more detail hopefully by EOD tomorrow

Okay breaking the argument down:

Background

1. As of v1.27.1, AbstractConnectionResolver::set_query_arg() exists as a way to "manually" set an underlying $query_args key-value, used by AbstractConnectionResolver::$query_class (e.g. WP_Query, WP_Term_Query, etc.

2. The justifiable use case for ::set_query_arg() is when extenders need to quickly overload resolution behavior for a $query_args key-value that _is not already exposed as/mapped from a GraphQL connection arg. (e.g. RootQuery.myPostsFilteredByMyCustomField is a WP_Query for posts matching my_custom_field meta value).

3. However, if there is an GraphQL connection arg, we should almost always use that instead of manipulating the $query_args key-value directly. For example when getting comments on a specific Post, we should:

   'resolve' => static function ( Post $post, $args, $context, $info ) {
     if ( $post->isRevision ) {
       $id = $post->parentDatabaseId;
     } else {
       $id = $post->ID;
     }
   
   +  // Set the Post ID on the GraphQL args.
   +  $args['contentId'] = absint( $id );
   
     $resolver = new CommentConnectionResolver( $post, $args, $context, $info );
   
   -  return $resolver->set_query_arg( 'post_id', absint( $id ) )->get_connection();
   +  return $resolver->get_connection();

   • This provides lifecycle conformity and reliability when reusing and extending Connection Resolvers and is a huge DX benefit. Extenders can target the same behavior regardless of where the resolver class is consumed; the args are the same at each hook (i.e. a "lifecycle breakpoint"), and extending child connection resolvers remains DRY.
   • The reason we currently arent doing this approach was the lifecycle flakiness of the AbstractConnectionResolver class. It was safer to simply have a cudgel on our AbstractConnectionResolver class than it was to target the implementation inconsistences (different/missing hooks, inconsistent mapping patterns, retriggering logic (e.g. rerunning the query) when trying to interact with class parameters). However, due the series of improvements to AbstractConnectionResolver, these are no longer concerns.
   • Once we are able to introduce breaking changes into AbstractConnectionResolver, there will be even more benefits to overloading the connection $args instead of using set_query_arg(), include performance bumps (e.g. by initializing class variables on demand instead of in the constructor).

In the context of #3066

4. The issue the author was attempting to address was our use of ::set_query_arg() leading to the buggy behavior described in point 3 (above)
   https://github.com/wp-graphql/wp-graphql/blob/release/v1.27.1/src/Registry/Utils/TermObject.php#L233C7-L243C9
   Specifically, by using ::set_query_arg() for tax_query we are overloading any values set for tax_query in the PostObjectConnectionResolver lifecycle. That means any plugin that wants to also set tax_query (e.g. wp-graphql-tax-query)` needs to overload our overload.
5. The author addressed this by changing the behavior of ::set_query_arg() so instead of setting the arg, it adds the arg to any previously set args. This is bad for several reasons:
   • It's a breaking change. If I was currently using ::set_query_arg() as intended to overload an arg (for legacy reasons described in 3), this is now broken. E.g. a trashedPosts field using $post_object_connection_resolver->set_query_arg( 'post_status', 'trashed' ), now returns all published and trashed posts.
   • It's an unwanted change. For the above reason, this removes the ability to overload query vars altogether, without resorting to a an add_filter(), breaking the resolve callback pattern.

In the context of this PR

6. We should never encourage the use of overloading $query_args directly vs passing it as GraphQL $args to the constructor for the reasons mentioned in point 3 (above). There should always be an explicit and specific reason for using ::set_query_arg() (or whatever it's called - see below).
7. As such, it seems counterproductive to add a special ::add_query_arg(), when we can accomplish that by using ::get_query_args() and then array_merge()ing what we want back into ::set_qeury_arg(). It's only 2-3 lines of code, and in this case the friction is good.
8. Worse, since the array_merge() pattern is implicit to overloading the _GraphQL $args in the constructor, we now have two paths to accomplish what seems to be the same thing, but actually has major edge cases that need to be accounted for (since regardless of whether$query_args are appended or overloaded, it happens only after the connection resolver has been instantiated).
9. Additionally, as efforts to migrate away from ::set_query_arg() take place (either due to post-breaking change improvements mentioned above and in the Proposed Steps below), developers who adopt these functions will likely need to almost immediately begin migrating away from them in 2.x).

Proposed Steps

10. What's remaining is whether to rename ::set_query_arg() to ::overwrite_query_arg(), or really the question "why do people keep thinking a function called `set_query_arg()" would append a value?
11. I believe that's due to our own use of ::set_query_arg() whether due to the legacy reasons noted in point 3 above or as a cudgel when it would be assumed that our intent is to add (e.g. the bug with wp-graphql-tax-query).
12. Taken together: I think the best step to address the confusion is to simply fix/replace all our uses of ::set_query_arg() which we need to do anyway (and is the last thing on my list before writing up the "migration guide" for the connecton resolver lifecycle improvements).

• Whenever possible, overload the GraphQL $args passed to the constructor instead of directly manipulating the underlying $query_args key-value pair.
• Review our patterns, and see where it makes more sense to array_merge() to a possible existing value instead of overloading it.

I'm on no sleep, so hope this is coherent 🤞

closing in favor of #3162