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

fix: PostObjectCursor's support for meta fields improved #2874

Merged

Conversation

kidunot89
Copy link
Member

What does this implement/fix? Explain your changes.

  • Fixes support for sorting post connection results by multiple meta.

Does this close any currently open issues?

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

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

Any other comments?

Where has this been tested?

Operating System:

WordPress Version:

@kidunot89 kidunot89 requested a review from jasonbahl July 27, 2023 23:08
@kidunot89 kidunot89 assigned kidunot89 and unassigned jasonbahl and justlevine Jul 27, 2023
@coveralls
Copy link

coveralls commented Jul 27, 2023

Coverage Status

coverage: 84.981% (+0.006%) from 84.975% when pulling 509c5e6 on kidunot89:fix/post-object-cursor-meta-where-fix into ee2f5e8 on wp-graphql:develop.

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change itself looks good to me (although I'm worried about a perf hit on non-custom meta). Wondering if maybe this deserves it's own test class instead of shoehorning into the existing post cursor test.

See comments for details.

src/Data/Cursor/PostObjectCursor.php Show resolved Hide resolved
tests/wpunit/PostObjectCursorTest.php Outdated Show resolved Hide resolved
tests/wpunit/PostObjectCursorTest.php Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Jul 31, 2023

Code Climate has analyzed commit 509c5e6 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@jasonbahl
Copy link
Collaborator

There's still some issues re: meta queries and pagination that this doesn't fully solve, for example, if multiple posts have the same meta value and one is the endCursor the next page will skip the other items with the same value.

This helps unblock a certain set of users and we can circle back on the remaining bugs later.

@jasonbahl jasonbahl merged commit 771192f into wp-graphql:develop Aug 3, 2023
32 of 33 checks passed
@justlevine
Copy link
Collaborator

There's still some issues re: meta queries and pagination that this doesn't fully solve, for example, if multiple posts have the same meta value and one is the endCursor the next page will skip the other items with the same value.

This helps unblock a certain set of users and we can circle back on the remaining bugs later.

Is there an existing issue for this? That's a complex edge case, wanna make sure we dont lose it.

@jasonbahl jasonbahl mentioned this pull request Aug 3, 2023
@kidunot89
Copy link
Member Author

kidunot89 commented Aug 3, 2023

@justlevine There is, using certain meta query structure when ordering by a meta field don't get processed correctly by the PostObjectCursor and the same in the TermObjectCursor class too. This PR fixes the issue for WP_Query instances with a meta_query field with at least two entries that aren't relation.

@justlevine
Copy link
Collaborator

Okay, my misunderstanding - the remaining edge cases that are unresolved are in #2644, not in addition to that GH issue. 👍

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.

None yet

4 participants