Implement new error handling stack

[franzliedke]

Refs #1641

Changes proposed in this pull request:

This separates the error registry (mapping exception types to status
codes) from actual handling (the middleware) as well as error formatting
(Whoops, pretty error pages or JSON-API?) and reporting (log? Sentry?).

The components can be reused in different places (e.g. the API client
and the error handler middleware both need the registry to understand
all the exceptions Flarum knows how to handle), while still allowing to
change only the parts that need to change (the API stack always uses the
JSON-API formatter, and the forum stack switches between Whoops and
pretty error pages based on debug mode).

Finally, this paves the way for some planned features and extensibility:

• A console error handler can build on top of the registry.
• Extensions can register new exceptions and how to handle them.
• Extensions can change how we report exceptions (e.g. Sentry).
• We can build more pretty error pages, even different ones for
  exceptions having the same status code.

Reviewers should focus on:

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run php vendor/bin/phpunit).

[dsevillamartin]

Looks good. I feel like we might want to support multiple Reporters, instead of just one though (unless I completely misread the code, an alias is used to set the reporter).

I'll report more whenever I attempt #1528 on top of this and see how well fof/sentry works with these new changes.

[franzliedke]

Good point.

My original idea was that extensions (or rather, an extender) could extend the binding with a composite reporter. Something like this:

class CompositeReporter implements Reporter
{
  public function __construct(array $reporters)
  {
    $this->reporters = $reporters;
  }
  public function report(HandledError $error)
  {
    foreach ($this->reporters as $reporter) {
      $reporter->report($error);
    }
  }
}

$container->extend(Reporter::class, function ($original) {
  return new CompositeReporter([
    $original,
    new SentryReporter,
  ]);
});

That class would have had to be provided by core, though.

Then I realized this can be drastically simplified using container tagging, so I implemented that. 😀

[franzliedke]

Okay, this breaks some minor things such as error messages when logging in with incorrect credentials.

The reason for that is our inconsistent use / generation of status codes. I will fix that in a follow-up PR, though, to make it easier to review, and explain my thinking in detail. (Already half-finished.)

[franzliedke]

TL;DR I consider this complete.

[dsevillamartin]

Ok, I reworked fof/sentry with these new changes, and it does seem better / easier.

I do have to ask, is there any reason for making ViewRenderer@determineView and ViewRenderer@getMessage private ? I created SentryRenderer which extends ViewRenderer, as all it needs to do is add some JavaScript to the view, not rework the whole logic, and I don't see why I'd need to copy/paste those two methods when I could just use them if they were protected.

Something weird happened, not sure if I did something wrong or what. The Request passed to Formatter@format doesn't seem to be the same request that is received by middleware. I had to use middleware to bind the request to the app, and use that instead of the one passed to the method. $request->getAttribute('actor') was null, so I had to use app('sentry.request')->getAttribute('actor').

You can see the code @ https://github.com/FriendsOfFlarum/sentry/tree/error-handling-refactoring. It's not great, and I should probably create a Sentry class instead of binding stuff to the application, but... 🤷‍♂

[tobyzerner]

@franzliedke gave me a personal tour of this PR and I approve :D

[franzliedke]

I do have to ask, is there any reason for making ViewRenderer@determineView and ViewRenderer@getMessage private ?

I just make them private by default. Inheriting from these classes means "your" classes can break when I change the internals of "my" classes, and I don't really consider that to be public API (i.e. free from BC breaks).

(That said, there are still many protected methods all over Flarum, because that used to be my / other people's default. I changed my mind over time.)

As for your use-case: since you're really just re-using the entire implementation of the ViewRenderer, you could decorate it:

$this->app->extend(ViewRenderer::class, function (ViewRenderer $renderer) {
    return new SentryRenderer($renderer);
});

Your SentryRenderer would simply call the existing renderer and then inject the JavaScript snippet directly into the PSR-7 response it receives from the ViewRenderer.

Something weird happened, not sure if I did something wrong or what. The Request passed to Formatter@format doesn't seem to be the same request that is received by middleware.

This is by far the harder problem. I congratulate you on your creative solution, though 😉.

The "problem" here is that PSR-7 requests and responses are immutable. The ErrorHandler middleware basically passes on the request it received from the outside world to inner middleware. One of these middlewares extracts cookies, headers etc. from the response to determine the actor, which is then stored as a "request attribute" on the request. Once you add a new request attribute, you're creating a new request instance, though - which means the request instance known by the ErrorHandler is unchanged. This was a design decision when PSR-7 was created.

I think it's unfortunate that we should have discovered this problem earlier: your previous solution only worked because you added your error middleware to the end of the stack. I would argue an error handler should be the first middleware in the stack, though.

Toby and I had an idea how to fix this, but it needs some further thought. For now, can I ask you to temporarily disable the user feature (or use your workaround) in your Sentry extension, and create an issue about accessing the request's actor in an error handler? That would be the right place for my proposal.

[dsevillamartin]

Thank you for your response @franzliedke, and glad I was able to discover the bug now.

Decorating is a good idea, and not something I had thought of, so thanks for that.

As those were my only concerns, 👍. I'll go ahead an create the issue for the bug as well.

[luceos]

Any particular reason to name the contract Formatter and its implementations Renderers? Can we amend this to make their relation easier and possibly move the implementations to a subfolder?

[franzliedke]

Thanks for the comment. I pushed some small updates to this:

• Documentation for most of the classes involved
• Renderers are now formatters, the interface is now called HttpFormatter
• Some other small API changes (in Reporter and in HandledError)

@datitisev Heads-up. These changes will break your Sentry extension. I am happy with this now, though; so there shouldn't be any further API changes hopefully.