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 Don't mark mocked response as an error #82

Merged

Conversation

GuySartorelli
Copy link
Member

This response type is only ever returned from the findgridfield action on the GridFieldQueuedExportButton (see GridFieldQueuedExportButton::handleAction()). The only way this action is accessed in normal operation is from the GenerateCSVJob::getGridField() method, which uses a mock request via Director::test() to get the response. More discussion about why this happens is in the parent issue.

In other words, the only way this response class is ever used, it cannot be a 500 error. It's always going to be a success - unless some exception is thrown, in which case this response won't be used anyway.

My best guess as to why this was set to 500 is in case some smart-alek decides to manually access the findgridfield action by typing in the appropriate URL to do so.... which is both a super weird edge case and also not something that explicitly needs a 500 response. A 200 response is, I'd argue, more appropriate even in that scenario, since the action has done what it's meant to do. There was no error.

tl;dr: 500 was probably never correct for this in the first place.

Issue

@emteknetnz
Copy link
Member

Tested locally, works good. The 500 in the constructor looks really weird and I cannot think of any reason to keep it, so happy to remove.

@emteknetnz emteknetnz merged commit 4549433 into silverstripe:2.8 Jun 14, 2023
6 checks passed
@emteknetnz emteknetnz deleted the pulls/2.8/not-error-response branch June 14, 2023 02:22
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

2 participants