Allow passing the event hint to the captureException method and pass it down to the before_send callback

[jarstelfox]

I have a use case where we have extended Exception to have some extra data. Ex:

class ExtraDataException {
   private $extraData = [];
   public function addExtraData($data) {
      $extraData[] = $data;
   }
   public function getExtraData(): array {
      return $this->extraData;
   }
}

This is ideologically different from how sentry works. Namely, the behaviour is to wrap with a scope to be able to $scope->setExtra('data', $data).

I am not opposed to this at all. In fact, I like it better than our current approach.

However, I have issues being able to shim in the functionality I need at a global level, as I lose context of the exception right here.

With this change I can do something like:

'before_send' => function (Event $event): Event {
   $data = array_map(function(ExceptionDataBag $e) {
      return $e->getThrowable()->getExtraData();
   }, $event->getExceptions());
   $event->setExtra($data);
   return $event;
}

The main reason for this pull is to allow us to shim our existing code.

[ste93cry]

Sorry for not answering your question, I forgot it (shame on me). I'm not sure I understand why you cannot set the data on the scope but you can on the event hint. As a side note, it looks like in the JS SDK you can set the captureContext key which is defined as Scope | Partial<ScopeContext> | ((scope: Scope) => Scope). Would it be enough for you to solve your issue in a more general way without having a specific field for the exception?

[jarstelfox]

As a side note, it looks like in the JS SDK you can set the captureContext key which is defined as Scope | Partial<ScopeContext> | ((scope: Scope) => Scope). Would it be enough for you to solve your issue in a more general way without having a specific field for the exception?

That would work! For reference, how I solved this for us was a custom integration.

class DebugExceptionIntegration extends AbstractErrorListenerIntegration {
   /**
    * {@inheritdoc}
    */
   protected function captureException(HubInterface $hub, Throwable $exception): void {
      $hub->withScope(function (Scope $scope) use ($hub, $exception): void {
         $scope->addEventProcessor(\Closure::fromCallable([$this, 'addExceptionMechanismToEvent']));

         if ($exception instanceof DebugException && $exception->hasDebugInfo()) {
            $scope->setExtra('DebugInfo', $exception->getDebugInfoStr());
         }

         $hub->captureException($exception);
      });
   }

Some background of why I opened this PR

Our previous error handler expected to get the exception at a similar time as before_send. When working with sentry I looked as the callback but noticed it is not passed the hint (which has the exception). I also noticed that the ExcpetionBag does not save the full exception and thought I may be missing something. After some digging, I was fairly certain that I did not have access to the exception during the on before_send function. Hence the pull.

It's possible I am still missing something and the $event does have access to the hint. If I missed It, I suspect other people would as well.

tl;dr

Any way to access the actual exception object just before reporting would be great!

[ste93cry]

That would work! For reference, how I solved this for us was a custom integration.

That was the solution I was going to suggest you as soon as I realized that what I wrote before did not make sense because I confused the ExceptionDataBag object with the EventHint object. They are indeed two distinct objects with two distinct purposes: while the first is the data object model of the API payload for the Exception interface, the latter is an object that should be passed along to carry information to the event processors and to the before_send callback

I was fairly certain that I did not have access to the exception during the on before_send function

And you were right. The key point is that we forgot to pass the hint to the callback like it's done in the JS SDK. If we did this, we would then be able fill the EventHint::$exception property and pass the object to the captureException method which in turn would carry that information over up to the before_send callback. If you are willing to reopen this PR and change it to make it work as expected I would gladly accept it.

[jarstelfox]

If you are willing to reopen this PR and change it to make it work as expected I would gladly accept it.

Will do!

[ste93cry]

There will be a few things to take into account, more specifically the fact that the captureException method of both the HubInterface and ClientInterface interfaces cannot be changed without breaking BC, so you will have to do some magic with func_get_args() to handle the new argument. Also, this PR should target the develop branch because it will be released in the next minor version. Thank you very much in advance for the patience and the willingness of still helping out even if you didn't get an answer at first

[jarstelfox]

@ste93cry I seem to be hitting a weird CI issue with xdebug + code coverage. As you can see, the code changes would have no effect on these. However, I am failing CI on:

Use of undefined constant XDEBUG_CC_UNUSED - assumed 'XDEBUG_CC_UNUSED' (this will throw an Error in a future version of PHP)

Would you mind helping me out here?

[ste93cry]

I believe it's because Xdebug 3 is not compatible with PHPUnit 7.5, but PHP 7.3.25 on Travis uses it. The issue was introduced with php-build/php-build@ac92f4e as a consequence of php-build/php-build#650

[jarstelfox]

@ste93cry Thanks for the links. I was looking around for some breadcrumbs as to what happened. I saw your fix was merged. Would you mind re triggering a build on this pull? I don't see the option in Travis, I assume I don't have enough permission.

[ste93cry]

I saw your fix was merged

The fix to revert updating Xdebug 3 on PHP versions lower than 8 has been discarded, I think you are confusing such fix with the one in PHP-Parser. At this point, since the update of PHPUnit is already done in the PR to support PHP8, my suggestion is to wait until it gets merged. Please also remember that these changes are not sufficient yet as you should add the hint to the HubInterface::captureException() method too

[ste93cry]

develop branch should again be green, if you don't mind rebasing and making the last changes required to get this PR working I will be happy to review it once more

[ste93cry]

@jarstelfox are you still interested in completing the work? Otherwise, would you mind if I do it for you?

[jarstelfox]

@ste93cry feel free to take it up. Sorry between work and the holidays, I forgot about this

[ste93cry]

Even if the CI is broken (until I merge master into develop to port the switch from Travis to GA), I expect everything to work, so if you want to give it a shot to see if it helps you with your original issue then it would be 🆒

Side note: I rebased on the wrong branch, in the tentative of fixing my mistake I lost your initial commit. I will anyway ensure to give you the proper credits for your contribution in the commit once it gets merged

[Jean85]

Sorry for making you backtrack again, but I fear that @ste93cry's suggestion was wrong: adding an argument to the interfaces is a breaking change and you correctly, but nearly (or all?) the implementing classes are final, so adding the argument there without func_get_arg() should be fine.

Do you agree @ste93cry?

[ste93cry]

Do you agree @ste93cry?

Yes, I wasn't aware that adding an argument with a default value even though the signature is different from the interface was fine. I now changed the implementation to get rid of all the calls to func_get_arg 👍

[stayallive]

Looks great, thanks all, good addition 👍