getsentry / sentry-php

The official PHP SDK for Sentry (sentry.io)
https://sentry.io
MIT License
1.83k stars 450 forks source link

previousErrorHandler number of arguments #891

Closed htuscher closed 4 years ago

htuscher commented 4 years ago
app/vendor/sentry/sentry/src/ErrorHandler.php on line 386 and exactly 5 expected in /app/vendor/shopware/shopware/engine/Shopware/Plugins/Default/Core/ErrorHandler/Bootstrap.php on line 183

Originally caused by the following error:

The Sentry\State\Hub::getCurrent() method is deprecated since version 2.2 and will be removed in 3.0. Use SentrySdk::getCurrentHub() instead.

The previousErrorHandler registered accepts 5 arguments. The fifth argument is the $errcontext according to https://www.php.net/manual/de/function.set-error-handler.php.

I've fixed the deprecated call for now, but the errorHandler should in general deal with the fifth optional argument.

Jean85 commented 4 years ago

I'm not sure if I understood the source of this error... The fifth parameter is optional AND deprecated as of PHP 7.2, so we cannot use it. Not having it in the function signature shouldn't be an issue.

htuscher commented 4 years ago

The application I'm in has an already registered error_handler. It's a closure with exactly 5 arguments. Sentry calls the previousErrorHandler with only 4 which is the problem. I know the 5th argument is deprecated but obviously it must be present if the closure requires it.

Jean85 commented 4 years ago

To my knowledge, PHP doesn't complain about invoking a function with more arguments, see https://3v4l.org/jf5va

So, you're probably doing something additional on your side to trigger this issue.

ste93cry commented 4 years ago

@Jean85 it's indeed a bug, the problem here is that the previous error handler expects the 5th argument to be present while we pass only 4 and PHP complains (correctly). @hhoechtl would you be so kind to submit the patch? Also, out of curiosity, does the deprecation generates from your application or from inside our library itself?

htuscher commented 4 years ago

I don't have a patch, I just fixed all errors in my application and that deprecated Sentry SDK call, so I don't have a problem atm.

But I found this: https://www.php.net/manual/de/migration71.incompatible.php

ste93cry commented 4 years ago

I don't have a patch

What I meant was if you could be so kind to submit a PR to fix the problem in our library (the fact that we don't pass the 5th argument). The problem you had will came back as soon as Sentry logs an error since it will again call the previous error handler without the $errcontext :wink:

htuscher commented 4 years ago

I don't think https://github.com/getsentry/sentry-php/commit/72c95d1b8c5ae7e1141d589990aa62780259ad88 is enough, the problem would be

if (null !== $this->previousErrorHandler) {
            return false !== \call_user_func($this->previousErrorHandler, $level, $message, $file, $line);
        }
ste93cry commented 4 years ago

Closing as it has been solved in #892