search

dbtlr / php-airbrake

A PHP 5.3 library for sending errors to the Airbrake.io service.
http://airbrake.io
MIT License
121 stars 44 forks source link

Backtraces sometimes misses the location of an error #37

Closed leonszkliniarz closed 9 years ago

leonszkliniarz commented 9 years ago

This only seems to affect PHP errors and does not seem to effect exceptions. When you get a notification, the first line of the stack trace seems to be missing entirely. So code like this:

function MyFunc()
{
  //Several hundred lines of legacy code
  $foo = $bar;
  //Several hundred more lines of legacy code
}
MyFunc();

You'll know that there was an undefined variable error that happened in MyFunc but not where in MyFunc it happened. This isn't so much fun when you have to work with legacy code.

A cursory glance suggests the problem is due to this line here: https://github.com/dbtlr/php-airbrake/blob/master/src/Airbrake/EventHandler.php#L169. This seems to be a fix for Bug #21.

leonszkliniarz commented 9 years ago

I'm just trying to write a test case for this and all of my test cases behave as expected. There's some weird edge-case thing happening here. I'll update this ticket when I find out what that is.

leonszkliniarz commented 9 years ago

Apparently I was just stupid when writing the test cases and once I got a working test case I couldn't NOT write one.

Test cases are available at https://github.com/llamadigital/php-airbrake/blob/master/tests/StackTraceTest.php. I've also done a little bit of refactoring of the Tests to have a shared TestCase parent class between the Test classes which work with the EventHandler class and made the Notice object visible on the test Client object.

There's two problems at work here (hence the two test cases). The one I spotted with php errors and a similar issue with exceptions. The php errors case is exactly because of the array_shift pointed out in the bug report. The issue with exceptions is handily summarised by this comment on php.net: http://php.net/manual/en/exception.gettrace.php#107563.

It may be a good idea to add a configuration value to opt-in for the fixes for these issues if they would in any way inconvenience people.

dbtlr commented 9 years ago

I just merged some testing updates into master.

I'll take a look at this issue a little later and see if I can replicate.

If you do manage to replicate it, please feel free to create a pull request. :)