Skip to content

fix: stop overwrite tax query filter if it exists #3066

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.

Sign up for GitHub

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

Jump to bottom
Merged
jasonbahl merged 4 commits into from
Jul 1, 2024
Merged

fix: stop overwrite tax query filter if it exists #3066

jasonbahl merged 4 commits into from
Jul 1, 2024

Conversation

@mohjak
Copy link

@mohjak mohjak commented Mar 4, 2024 •
edited
Loading

What does this implement/fix? Explain your changes.

The change is aggregating the taxonomy filters. This is related to wp-graphql-taxquery plugin.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Suppose the following scenario:

Given I installed a fresh copy of a WordPress v6.4.3 and WP GraphQL v1.22.0 and WPGraphQL Tax Query
v0.2.0
And I have created a category with slug 'cat1'
And a tag with slug 'tag1'
And I have created a post that relates only to 'cat1'
When I run the following query:

query categoriesSearch {
  categories(where: {slug: "cat1"}) {
    edges {
      node {
        name
        ... on Category {
          posts(
            first: 1
            where: {status: PUBLISH, taxQuery: {relation: AND, taxArray: [{field: NAME, operator: EXISTS, taxonomy: TAG}]}}
          ) {
            nodes {
              title
              status
              tags {
                nodes {
                  name
                }
              }
            }
          }
        }
      }
    }
  }
}

Then the following result returns:

{
  "data": {
    "categories": {
      "edges": [
        {
          "node": {
            "name": "Cat1",
            "posts": {
              "nodes": [
                {
                  "title": "Post Cat1",
                  "status": "publish",
                  "tags": {
                    "nodes": []
                  }
                }
              ]
            }
          }
        }
      ]
    }
  }
}

But, the expected result is to return nothing in posts.

Any other comments?

…

Where has this been tested?

Operating System: …
Ubuntu 22.04.4 LTS

WordPress Version: …
6.4.3

@justlevine
Copy link
Collaborator

justlevine commented Mar 4, 2024

Hey @mohjak thanks for opening this PR!

This would be a breaking change to implement, and the same outcome can already be achieved with get_query_arg() and array_merge().

I didn't really follow what issue you're trying to solve with this change, but perhaps it makes more sense to do in your custom code with one of the connection resolver hooks, or if it's a bug with the tax query extensions you referenced then a PR to that repo.

( ref: #3036 (comment) )

@justlevine justlevine added type: enhancement Improvements to existing functionality needs: discussion Requires a discussion to proceed compat: breaking change This is a breaking change to existing functionality needs: info More information is needed to resolve this issue needs: reviewer response This needs the attention of a codeowner or maintainer labels Mar 4, 2024
@mohjak
Copy link
Author

mohjak commented Mar 5, 2024

Hi @justlevine,

What I am trying to resolve is the following set_query_arg function:

wp-graphql/src/Registry/Utils/TermObject.php

Line 234 in 9d02149

$resolver->set_query_arg(

Which is expecting that there is no tax_query in the query args that coming from graphql input, but actually it can have a tax_query like my above scenario. So, it overwrites the tax_query with the value mentioned in that referenced line of code.

@justlevine
Copy link
Collaborator

justlevine commented Mar 6, 2024

Hi @justlevine,

What I am trying to resolve is the following set_query_arg function:

wp-graphql/src/Registry/Utils/TermObject.php

Line 234 in 9d02149

$resolver->set_query_arg(

Which is expecting that there is no tax_query in the query args that coming from graphql input, but actually it can have a tax_query like my above scenario. So, it overwrites the tax_query with the value mentioned in that referenced line of code.

If anything, seems like it would be the resolve you referenced that needs to be "fixed", to check if tax_query is set, and if so merge it and pass the results to the $resolver->set_query_arg().

@mohjak can you foresee any issues if we took that (nonbreaking) approach?

@jasonbahl jasonbahl self-requested a review March 15, 2024 17:00
jasonbahl
jasonbahl reviewed Jun 27, 2024
View reviewed changes
@coveralls
Copy link

coveralls commented Jun 27, 2024

Coverage Status

coverage: 84.295% (+0.005%) from 84.29%
when pulling e28d834 on mohjak:fix/overwrite-taxquery-issue
into f884793 on wp-graphql:master.

@coveralls
Copy link

coveralls commented Jun 27, 2024

Coverage Status

coverage: 84.295% (+0.005%) from 84.29%
when pulling e28d834 on mohjak:fix/overwrite-taxquery-issue
into f884793 on wp-graphql:master.

@jasonbahl
Copy link
Collaborator

jasonbahl commented Jul 1, 2024

Apologies for the long-overdue review of this PR.

I was able to walk through the scenario with the WPGraphQL Tax Query plugin active and reproduce the issue you outlined and I have confirmed that this PR does fix it.

I wrote up a test based on the scenario that fails without the changes in this PR applied and passes with the changes applied.

@coveralls
Copy link

coveralls commented Jul 1, 2024

Coverage Status

coverage: 84.295% (+0.005%) from 84.29%
when pulling c37b437 on mohjak:fix/overwrite-taxquery-issue
into f884793 on wp-graphql:master.

@jasonbahl jasonbahl merged commit e27e1b9 into wp-graphql:master Jul 1, 2024
This was referenced Jul 2, 2024
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonbahl jasonbahl jasonbahl approved these changes

Assignees

@mohjak mohjak

Labels

compat: breaking change This is a breaking change to existing functionality needs: discussion Requires a discussion to proceed needs: info More information is needed to resolve this issue needs: reviewer response This needs the attention of a codeowner or maintainer type: enhancement Improvements to existing functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mohjak @justlevine @coveralls @jasonbahl