bugsnag / bugsnag-symfony

BugSnag notifier for the Symfony PHP framework. Monitor and report errors in your Symfony apps.
https://docs.bugsnag.com/platforms/php/symfony
MIT License
43 stars 21 forks source link

Symfony 5 patch #99

Closed Cawllec closed 4 years ago

Cawllec commented 4 years ago

Goal

Changes

The order that the GetResponseForExceptionEvent and ExceptionEvent are checked have been swapped. This is because for within Symfony 4.3 & 4.4, ExceptionEvent is an extension of GetResponseForExceptionEvent and therefore would trigger either condition. By ensuring ExceptionEvent comes first, we ensure the correct (non-deprecated) method, getThrowable is called when available.

However, within Symfony 4.3, event though the ExceptionEvent exists, neither it nor its base class, GetResponseForExceptionEvent, implement the getThrowable method. This is why the method_exists check is present, to avoid attempting to call this method.

In addition, to ensure continuity with the InvalidArgumentException that would be thrown by adding type hints, this method will throw an InvalidArgumentException if the input is neither an ExceptionEvent with the getThrowable method, or a GetResponseForExceptionEvent.

Tests

Unit testing the changes proves to be difficult for a number of reasons.

  1. There is seemingly no easy way to ensure a test only runs against a specific version of Symfony

  2. In Symfony 5 the ExceptionEvent class is marked as final, meaning it cannot be mocked via mockery. It may be possible to create an ExceptionEvent, but because that requires an HttpKernel object it may be difficult to ensure all of the elements are in place and mockable to create a working configuration.

It would be feasible to create maze runner e2e tests in order to verify the change, however this would be a significant addition and may take time.

Cawllec commented 4 years ago

It looks like StyleCI has decided to add a bunch of changes to the example config. This shouldn't effect the content of the release.