PHPCS Move strict_types to seperate lines

[emteknetnz]

Fix up PHPCS errors

This PR, was originally a fix for silverstripe/silverstripe-mfa#378, though have moved logic to https://github.com/silverstripe/silverstripe-mfa/pull/401/files instead

[robbieaverill]

I'm thinking we just remove the strict_types?

Drop the declaration to a new line and PHPCS will be happy. You can also run vendor/bin/phpcbf and provide the module name to fix.

[ScopeyNZ]

You're going to be gobbling up legitimate errors here. I think that the interface should deal with a 500 response, rather than just removing the error in this one place. Other errors thrown (resulting in 500s) are going to have the same problem as described in silverstripe/silverstripe-mfa#378.

[emteknetnz]

@ScopeyNZ

a) What are legitimate errors?
This is catching unhandled expections that are not turned into json (via Result::create()). On a dev environment I guess this isn't great because you get the exception output in the response, I guess getting 'The was an unknown error' back isn't so helpful there. On live though you just get a blank response and a 500 status code, hence the endless spinner.

b) What is "the interface"? (Sorry I'm not too familiar with this module)

[ScopeyNZ]

So the interface would be the thing that dispatches a request, and shows a spinner while it waits for the response. That should be updated to handle 500s. I don't know what the 500 error is here, but you're catching Exception, which is just one step above catching Throwable. The server throws a 500 because something's wrong. That's perfectly fine behaviour. Returning 200 OK - Everything is wrong is not really a good result.

[ScopeyNZ]

I was being a little terse there, it's actually returning a 401, but still "an unknown error occurred" with a 4xx response (client problem) is not really semantically correct. It should be a 5xx response.

[emteknetnz]

@ScopeyNZ OK I think I have a corresponding PR that does this for 429 on the mfa module - silverstripe/silverstripe-mfa#401 - so would it work if we close this PR and update the other PR to handle 500?

[ScopeyNZ]

Sound good to me.

In fact, you could probably just fill in the "default" case in that PR with an "unknown error" message.

[emteknetnz]

@ScopeyNZ OK have removed the 500 logic from this PR. I've kept this PR open to just fix up the PHPCS issues