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

Add Tax Query support to PostObject #1387

Closed
wants to merge 13 commits into from

Conversation

seagyn
Copy link

@seagyn seagyn commented Jul 14, 2020

What does this implement/fix? Explain your changes.

This implements the ability (previously in a plugin) to do a tax query on a post object. This is super useful for people which large sets of posts that need the ability to filter those down by taxonomy especially on sites that pull from WPGraphQL on runtime.

Does this close any currently open issues?

Closes #1384

Any relevant logs, error output, GraphiQL screenshots, etc? <- TODO

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

It's worth noting that the long term plan is to move to a more global filter plan which is being discussed in #1385.

Where has this been tested? <- TODO

Operating System:

WordPress Version:

@netlify
Copy link

netlify bot commented Jul 14, 2020

Deploy request for wpgraphql-docs pending review.

Review with commit ad0aca8

https://app.netlify.com/sites/wpgraphql-docs/deploys

@seagyn seagyn marked this pull request as draft July 14, 2020 07:05
@jasonbahl
Copy link
Collaborator

@seagyn I think we might need some way to make sure the tax query plugin doesn't cause errors by registering the same Types and Fields. 🤔

Like if WPGRAPHQL_VERSION is greater than x, do something to prevent execution of the tax query plugin 🤔

We can handle it to a degree with documentation, which we should for sure do as well, but some way to avoid the conflicts in code would be 👌 in my mind.

@seagyn
Copy link
Author

seagyn commented Jul 28, 2020

@seagyn I think we might need some way to make sure the tax query plugin doesn't cause errors by registering the same Types and Fields. 🤔

Like if WPGRAPHQL_VERSION is greater than x, do something to prevent execution of the tax query plugin 🤔

We can handle it to a degree with documentation, which we should for sure do as well, but some way to avoid the conflicts in code would be 👌 in my mind.

@jasonbahl because we're pre 1.0 is it perhaps not worth allowing this to break? I went down the rabbit whole of doing this and my thought pattern ended with "When would we feasibly remove this check?". There have been much larger breaking changes in the codebase.

If it's a hard requirement, we could possibly disable the plugin during the bootstrapping process. As soon as it initialises (the version with this code), it will check for the existence of the plugin and disable it? https://developer.wordpress.org/reference/functions/deactivate_plugins/ with this as a check: https://developer.wordpress.org/reference/functions/is_plugin_active/

Thoughts?

@jasonbahl
Copy link
Collaborator

@seagyn good points.

The work I'm doing for #1409 might alleviate my initial concerns also. If the plugin were active with the fields also in core WPGraphQL, helpful debug logs (not errors) would be output to let folks know about the duplicate fields.

@seagyn seagyn marked this pull request as ready for review July 28, 2020 22:48
@seagyn
Copy link
Author

seagyn commented Jul 28, 2020

@seagyn good points.

The work I'm doing for #1409 might alleviate my initial concerns also. If the plugin were active with the fields also in core WPGraphQL, helpful debug logs (not errors) would be output to let folks know about the duplicate fields.

OK great. I've added the tests. Let me know if this anything else I must do. Stuck on the coverage part and not sure if that's automated.

@seagyn
Copy link
Author

seagyn commented Sep 16, 2020

@jasonbahl finally fixed all the tests 🙈

Do you want me to explore any improvements to this?

@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #1387 into develop will decrease coverage by 0.27%.
The diff coverage is 36.84%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1387      +/-   ##
===========================================
- Coverage    62.36%   62.08%   -0.28%     
===========================================
  Files          186      190       +4     
  Lines        10542    10656     +114     
===========================================
+ Hits          6574     6616      +42     
- Misses        3968     4040      +72     
Impacted Files Coverage Δ
src/Type/Enum/TaxonomyQueryOperatorEnum.php 15.38% <15.38%> (ø)
src/Type/Enum/TaxonomyQueryFieldEnum.php 21.73% <21.73%> (ø)
src/Type/Input/TaxonomyQueryInput.php 28.57% <28.57%> (ø)
src/Type/Input/TaxonomyArrayInput.php 30.43% <30.43%> (ø)
src/Connection/PostObjects.php 62.68% <50.00%> (-0.15%) ⬇️
...c/Data/Connection/PostObjectConnectionResolver.php 64.11% <80.00%> (+2.11%) ⬆️
src/Registry/TypeRegistry.php 79.76% <100.00%> (+0.15%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb4e3b8...ad0aca8. Read the comment docs.

@rubas
Copy link

rubas commented Apr 1, 2021

@seagyn @jasonbahl Anything missing here?

@Zinkutal
Copy link

@jasonbahl, bringing this to your attention since version 1.5.9 plugin wp-graphql-tax-query no longer works.
See: wp-graphql/wp-graphql-tax-query#27
Thanks

@dgaidula
Copy link

@jasonbahl any word on this? I just came across the issue via: wp-graphql/wp-graphql-tax-query#27 Didn't want to lose the speed of 1.6+ so I wrote a patch for wp-graphql-tax that checks if type is registered first so I can register taxTypes that I needed along with mutations, etc. and still have the plugin do its work. Would rather have an official solution. thx.

@seagyn seagyn closed this May 24, 2022
@tomgreeEn
Copy link

Is this now obsolete @seagyn ?

@jasonbahl
Copy link
Collaborator

jasonbahl commented May 24, 2022

@tomgreeEn I've had some discussion with some folks (specifically my colleague at WPEngine, @ToughCrab24) on better standardizing the filters for connections.

We're hoping to have some progress in this area in the not too distant future, likely starting with some ideas posted in a new issue, then some iteration.

The idea is to get to a point where the filters on connections are more standardized.

We're looking at how other systems like Graph.Cool, Prisma, GraphCMS, SupaBase, Gatsby, etc handle their filters.

Right now, there's severe limitations from the Schema design and usability of our "connection where args" design and we want to address that a bit more formally

@tomgreeEn
Copy link

Understood thanks for the explanation!

@tomgreeEn
Copy link

Didn't want to lose the speed of 1.6+ so I wrote a patch for wp-graphql-tax that checks if type is registered first so I can register taxTypes that I needed along with mutations, etc. and still have the plugin do its work. Would rather have an official solution. thx.

@dgaidula, how did you patch wp-graphql-tax-query ? Any chance you could share your diff?

Or @jasonbahl is there a recommended way to run these kinds of queries while you formalize filters? Would really like to move off 1.5.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring taxQuery into Core
6 participants