Closed kinglozzer closed 3 months ago
Bump. This issue is completely breaking one of my queries
@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.
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.
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.
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.
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
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
571
Pull request checklist