fix: taking care about the case of empty order by

[mohjak]

What does this implement/fix? Explain your changes.

This fix resolves the issue when the 'ORDER BY' is missing in cursor pagination.

Does this close any currently open issues?

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

Reproduce Link

I have installed WPGraphQL Version 1.21.0 in addition to this version of an offset pagination plugin https://github.com/alaa-alshamy/wp-graphql-offset-pagination/releases/tag/v0.2.0.2.
I have the following categories for example as parent, child

When I run the following GraphQL query to count total of children of the targeted parent that id is 38.

query terms {
  categories(where: {
    parent: 38
  }) {
    pageInfo {
      offsetPagination {
        total
      }
    }
    nodes {
      name
      databaseId
      slug      
    }
  }
}

Then the total is returning null as you can see in the image below:

After Applying the fix of this pull request the issue resolves:

Any other comments?

I believe the following testing environment should help you to reproduce the issue, this is hosted in local by flywheel:
https://probable-tendency.localsite.io/wp-admin/admin.php?page=graphiql-ide
Basic Authentication:
Username: pattern
Password: unusual

Admin Panel:
Username: admin
Password: password

Where has this been tested?

Operating System: …
Ubuntu 22.04.4 LTS

WordPress Version: …
6.4.3

[justlevine]

Thanks so much for this commit, @mohjak 🙌

Can you provide an example of something that currently triggers this error - both for review purposes and to help us write the necessary unit tests?

(For example, I see that graphql_wp_query_cursor_pagination_stability() already enforces a string type, though graphql_wp_user_query_cursor_pagination_stability() only typehints it. Meanwhile, graphql_wp_term_query_cursor_pagination_support()'s $pieces has no typings so it definitely needs that guard. )

[justlevine]

(tests are failing because this is based on master and not develop where the Codecption version is pinned. We can rebase now or wait for #3061 to ship)

[mohjak]

You are welcome @justlevine, I have updated the pull request info. Please let me know if there are anything still missing.

[mohjak]

@justlevine We are not enforcing string type, but we're trying to resolve if there is no value in $orderby parameter then a MySQL syntax error will appear.

in the old code:

return "{$orderby}, {$key} {$order}";

So, If the $orderby is empty then the result will be for example ', date DESC'. Which is a MySQL syntax error.

[justlevine]

@justlevine We are not enforcing string type, but we're trying to resolve if there is no value in $orderby parameter then a MySQL syntax error will appear.

Yes understood.

Since the latter two methods don't enforce a param type, those concatenations could produce a fatal error if they're not being type guarded upstream to make sure they're only getting a string.

Was just giving an example of something that having a test case helps us scope when reviewing. The updated PR description looks like it now has the info we need 👌

[jasonbahl]

@mohjak I was able to reproduce the issue you were seeing where offsetPagination.total returns 0 before this PR and returns 1 after the PR.

Before merging, we'll need a test case that ensures this fix doesn't have a future regression.

I'm working on writing a test case now.

[jasonbahl]

@mohjak I couldn't push up my test to your PR branch so I opened this PR: mohjak#1

But I think I'll just merge your PR then follow up with the test in a separate PR to core WPGraphQL

[jasonbahl]

Approving. I added a test here: mohjak#1

And will merge that test in a follow-up PR.