Connehito / cake-sentry

CakePHP plugin integration for Sentry
MIT License
34 stars 26 forks source link

Cake 4.x updates #29

Closed dorxy closed 4 years ago

dorxy commented 4 years ago

I've ensured that all tests still pass, but have not yet tested the changes in a real setting, will do so with this fork in the upcoming days.

Might be a good starting point to use for releasing an upgraded version of the plugin, have already edited the README to explicitly use a ^3.0 version for composer require.

BGehrels commented 4 years ago

Hey @dorxy, thank you for your work!

It works fine for me so far, apart from one thing: skipLog is not working as it should: The Exceptions will still end up in sentry.

One thing i found here is that Connehito\CakeSentry\Error\Middleware\ErrorHandlerMiddleware tries to override the logException($request, $exception) method of Cake\Error\Middleware\ErrorHandlerMiddleware, but the later does not have this method any more since 4.x

dorxy commented 4 years ago

Hey @dorxy, thank you for your work!

It works fine for me so far, apart from one thing: skipLog is not working as it should: The Exceptions will still end up in sentry.

One thing i found here is that Connehito\CakeSentry\Error\Middleware\ErrorHandlerMiddleware tries to override the logException($request, $exception) method of Cake\Error\Middleware\ErrorHandlerMiddleware, but the later does not have this method any more since 4.x

Seems like the right approach would be to configure the plugin Error handlers to the existing middleware in ones application. This would eliminate the use for another middleware in the plugin. If any regular collaborators can confirm this, I can remove the middleware and update the docs with replacing the default

->add(new ErrorHandlerMiddleware(Configure::read('Error', [])))

with

->add(new ErrorHandlerMiddleware(new ErrorHandler())

in Application.php

and adding a public logException method to the SentryErrorHandlerTrait

I must say that there are so many ErrorHandler/Rendering/Logging constructions in CakePHP that I'm not sure what the best strategy would be. It might even be going to the lowest level and catching exceptions using a logging wrapper? this would then automatically log any exceptions going through CakePHP's BaseErrorHandler::logException (from which both ErrorHandler and ConsoleErrorHandler extend), and automatically skip any exceptions configured in its skipLog

I'm thinking this way a lot of classes can be removed and all we have to do is extend ErrorLogger and ensure it's configured on the handlers, adding instructions in the README.

dorxy commented 4 years ago

I've also noticed, I don't know why they did this, that plugin bootstrapping is now done after application bootstrap. So if you load this plugin the cake way, bootstrap.php is loaded after application bootstrapping and if any errors occur during that process, they probably won't be logged in Sentry either...

o0h commented 4 years ago

Merged #34 as alternate to this PR.Close this.