Filter API inconsistencies

[smthomas]

In the graphql_map_input_fields_to_wp_query filter, the $post_type is the last parameter. This is inconsistent with the other graphql_map_input_fields_* filters. For example in graphql_map_input_fields_to_get_terms the $taxonomy parameter is the third parameter (while the $context & $info parameters are the last two in all of the graphql_map_input_fields_* filters). For consistency sake, it may make sense to update this filter.

wp_query filter:

wp-graphql/src/Data/Connection/PostObjectConnectionResolver.php

Line 431 in 560bd4a

$query_args = apply_filters( 'graphql_map_input_fields_to_wp_query', $query_args, $where_args, $this->source, $this->args, $this->context, $this->info, $this->post_type );

get_terms filter:

wp-graphql/src/Data/Connection/TermObjectConnectionResolver.php

Line 225 in 560bd4a

$query_args = apply_filters( 'graphql_map_input_fields_to_get_terms', $query_args, $where_args, $this->taxonomy, $this->source, $this->args, $this->context, $this->info );

[justlevine]

Related: #331

[stale]

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.

[stale]

This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue.

[justlevine]

Removing not stale, now that stalebot is behaving.

While filter consistency is still an issue, the specific inconsistencies will need to be reviewed before we can figure out an implementation plan (and what can be addressed via breaking/nonbreaking changes).

[tihom4537]

I am willing to contribute on this issue . I went through the codebase and I think I can fix it. I am bit confused if this issue is on hold or still active . I can start working on it once my doubt is cleared .

[justlevine]

I am willing to contribute on this issue . I went through the codebase and I think I can fix it. I am bit confused if this issue is on hold or still active . I can start working on it once my doubt is cleared .

Hey @tihom4537 , thanks so much for your offer to contribute 💪!

The issue is still valid along with the rest of #331. The reason the status is on hold is because:

• this would be a breaking change to implement
• the change is "low impact" (devs have access to the same params, it's just the order that should be rearranged for DX consistency)
• There's a lot of improvements that need to be made to the Connection Resolver (exploring in refactor!: [WIP] Exploring a ConnectionResolver 2.0  #2749), which will anyways invalidate anything we do here.

Let me know if you have any further questions about this ticket or any others you might want to pick up instead. (Also feel free to ask in the #maintainers channel on Slack).

[tihom4537]

I have generated a pull request. Do consider it for the review .

[justlevine]

Hi @tihom4537

The effort is appreciated. However, as I mentioned above, this is currently on hold. It likely won't be merged until we start working on v2.0, and that's assuming no other changes invalidate it.