FIX: Don't attempt to correct sort when passed as an argument (closes #571)

[kinglozzer]

Description

Proposed fix for #571. Doesn’t actually solve the underlying issue, but prevents the warnings being emitted by SortTrait.

Manual testing steps

N/A

Issues

• GraphQL errors when passing sort parameters as separate variables #571

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[marwan38]

Bump. This issue is completely breaking one of my queries

[GuySartorelli]

What are the circumstances that result in the value not being an instance of that class?
Depending on what those circumstances are, we might want to throw an exception (or at the very least log a warning) if someone tries to perform a sort that we explicitly don't respect here.

[kinglozzer]

$arg->value is only ever an instance of ObjectValueNode when the sort order is baked into the query, so the continue statement is reached when sort is passed as a variable instead.

[kinglozzer]

@GuySartorelli a little more context: there are two ways to pass a sort order: (1) baked directly in to the query itself, or (2) as an argument.

For (1), #563 was merged to ensure that the order that you provide sort fields is respected. A regression from that PR is that when you use approach (2), a warning is thrown.

For (2), #563 had no effect anyway. This PR just ensures that when using this approach, the warning isn’t thrown and everything behaves as it did before #563 was merged.

Ideally, we’d expand #563 to cover both approaches to sorting. However that isn’t possible, because the underlying GraphQL library discards the order that sort fields are passed in when parsing the query. That isn’t a bug with the library, it’s actually the intended behaviour as it respects the GraphQL spec. There‘s some more info in #573 about that, but that’s a much bigger job and would have to be targeted at a major release.

[GuySartorelli]

Thank you for that extra context. I think in that case I'm okay with this - but a warning needs to be logged so developers can get some feedback that their query isn't working as intended.

[kinglozzer]

a warning needs to be logged so developers can get some feedback that their query isn't working as intended

I’m not really sure how to tackle that, the warning is the problem... we could possibly use a custom logger and call $logger->info() but unless the logger is set up to actually record that somewhere, nobody will ever see it.

[GuySartorelli]

We should log a warning using the Psr\Log\LoggerInterface.errorhandler singleton documented in https://docs.silverstripe.org/en/5/developer_guides/debugging/error_handling/#logging-and-error-handling

unless the logger is set up to actually record that somewhere, nobody will ever see it.

Yup, that's true of all logging. Better to give the option than nothing though.

[kinglozzer]

I’ve finally got around to this. Using LoggerInterface::class . '.errorhandler' results in the warning being output in the HTTP response, which breaks everything, so I’ve copied the approach from the old file migration task: https://github.com/silverstripe/silverstripe-assets/blob/1.13/_config/migration.yml. Essentially it’s a logger instance that doesn’t include an output handler so nothing appears in the HTTP output, but the warning is still logged to the error log

[GuySartorelli]

LGTM, thanks for doing this.