Skip to content

Conversation

@KKSzymanowski
Copy link
Contributor

@KKSzymanowski KKSzymanowski commented Jan 19, 2017

This pull request prepares sentry-laravel for the upcoming Laravel 5.4 release.

There are two changes that affect this package, as I described here:

  1. Different handling of wildcard event - the Dispatcher::firing() method has been removed and instead the event name is passed as the first argument.
  2. The illuminate.log event has been changed to Illuminate\Log\Events\MessageLogged.

For the second change, I simply added another handler.
First change however was a bit more tricky. Because of changed event handling I was left with two options:

  1. Detect Laravel version and act appropriately
  2. Refactor wildcard event handlers to bespoke function for each event.

I chose the second one, simply because it provides cleaner and more easily extendable logic.
There are three events which need handling. For each of them there is an old and a new version, therefore there are total of 6 events with handlers attached:

protected static $eventHandlerMap = [
    'router.matched'                           => 'routerMatched', // Until Laravel 5.1
    'Illuminate\Routing\Events\RouteMatched'   => 'routeMatched',  // Since Laravel 5.2

    'illuminate.query'                         => 'query',         // Until Laravel 5.1
    'Illuminate\Database\Events\QueryExecuted' => 'queryExecuted', // Since Laravel 5.2

    'illuminate.log'                           => 'log',           // Until Laravel 5.3
    'Illuminate\Log\Events\MessageLogged'      => 'messageLogged', // Since Laravel 5.4
];

Because wildcard events essentially went through one method, there was no problem with catching all exceptions thrown by handlers. Now however, as each event has it's own method there would be try-catch block duplication all over the class. In order to avoid that, I don't register actual handler in the Dispatcher, instead I register a fictional method, a call to which is intercepted by __call method, handling all exceptions and passing the event payload down to the actual handler.

I have tested these changes in Laravel 5.1, 5.2, 5.3 and 5.4 with following bit of code:

\Log::info('test');
\App\User::get();
echo($nonexistentVariable);

An undefined variable error was caugth by Sentry and in the Issue summary there was the correct route as well as log entry and SQL query.

5.0 doesn't support PHP 7 and isn't supported by Laravel team, so I didn't run any tests there. Also Laravel event logic hasn't been changed between 5.0 and 5.1 so all code that works with 5.1 should also work with 5.0.

@dcramer
Copy link
Member

dcramer commented Jan 20, 2017

At a glance this looks really good. I'll try to do a deeper review tomorrow and get this merged in.

Thanks!

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.

2 participants