Caught exceptions are send to sentry

[amenk]

Bug: What is the current behavior?

When opening the page with clear layout cache, lots of exceptions are sent to sentry, which are actually caught be Magento core already.

Bug: What is the expected behavior?

Not sending these exceptions to Magento.

Bug: What is the proposed solution?

Filter them out?

Additional

In general I am also wondering why these exceptions are raised at all, but this is a different story for StackOverflow.

• Magento 2.4.5-p1
• Module 3.3.0

[indykoning]

These are errors that are not actually caught by Magento until the very last moment, at a point where it's not possible to hook into it anymore.
Then depending on the context it hides this error.
If it were in the terminal it would still error out and break the application.

There's a large chance to miss important errors if we were to ignore the errors that would be hidden in the frontend or otherwise solved (by for instance adding a notification)

Normally you can filter exceptions with these settings: https://github.com/justbetter/magento2-sentry#:~:text=%27log_level%27%20%3D%3E%20%5CMonolog%5CLogger%3A%3AWARNING%2C%0A%20%20%20%20%27errorexception_reporting%27%20%3D%3E%20E_ALL%2C%0A%20%20%20%20%27ignore_exceptions%27%20%3D%3E%20%5B%5D%2C
But since it's a very general ErrorException that's not smart.

I am thinking about adding wrappers for the filtering functions https://docs.sentry.io/platforms/php/configuration/filtering/
so Magentos plugin system can be used to change and filter them.

But right now you can already add advanced filtering in the following way
#89 (comment)

[amenk]

These are errors that are not actually caught by Magento until the very last moment, at a point where it's not possible to hook
into it anymore.

I am confused 😕 ? There is a catch in line 996 and no throw - so I believe it should be caught?

[amenk]

@indykoning I think that exception is caught - do you know the reasons why it still pops up? Is it because it's originally a warning?

[indykoning]

The catch statement does not catch every throwable sadly so things could still pass through, throwable is the base of any exception which is what we catch so that could be a cause.
Or some Magento magic is obscuring the true stacktrace, they have an error handler that transforms any runtime error or warning into an exception which could be a probable cause of it passing through the first catch as it's a warning but not through Sentry as it's an exception by that point.

Sadly i'm not able to reproduce these errors in my projects

[amenk]

Happy to help debugging.

It looks like the warning is converted to an exception in

vendor/magento/framework/App/ErrorHandler.php

which seems to be logged by sentry before it's converted to an exception and being caught.

[amenk]

I would have a workaround for that specific problem - adding the NOWARNING and NOERROR flags in https://github.com/magento/magento2/blob/b9101b158a9acb163a31433b2c20fe13cad5f7fe/lib/internal/Magento/Framework/View/Model/Layout/Merge.php#L564

        return simplexml_load_string($xmlString, \Magento\Framework\View\Layout\Element::class,  LIBXML_NOWARNING | LIBXML_NOERROR);

I could submit a patch to Magento, but it is possible that this happens also on other places.

The question is now to not report such errors which are caught by Magento Error handler.

Probably it is reproducible by changing this line to

        return simplexml_load_string('brokenXMLStringHere', \Magento\Framework\View\Layout\Element::class);

[amenk]

It's sent to sentry here:

vendor/sentry/sentry/src/ErrorHandler.php:302

I am reproducing this by inserting this code:

vendor/magento/framework/App/Bootstrap.php:267

                $response = $application->launch();

                # test code here, after launch:
                try {
                    simplexml_load_string('thisisanerror', \Magento\Framework\View\Layout\Element::class);
                } catch (\Exception $e) {
                    echo $e->getMessage();
                }

                die(__METHOD__);

Error handler is registered here:

# vendor/sentry/sentry/src/ErrorHandler.php:143
        $errorHandlerCallback = \Closure::fromCallable([self::$handlerInstance, 'handleError']);

        self::$handlerInstance->isErrorHandlerRegistered = true;
        self::$handlerInstance->previousErrorHandler = set_error_handler($errorHandlerCallback);

So this is sentry core code. I would be better from my point of view if the sentry error handler runs after custom framework (Magento) error handlers to see what was caught already and to avoid false positives?

[amenk]

I did some experiments with registering the Magento handler after Sentry's, by patching:

\Magento\Framework\App\Bootstrap::run

    public function run(AppInterface $application)
    {
        try {
            try {
                \Magento\Framework\Profiler::start('magento');
                $this->assertMaintenance();
                $this->assertInstalled();



                $response = $application->launch();
                $this->initErrorHandler();

                foobar();
                
                try {
                    simplexml_load_string('thisisanerror', \Magento\Framework\View\Layout\Element::class);
                } catch (\Exception $e) {
                    echo $e->getMessage();
                }

While this reporting the caught exception of SimpleXML ist would also avoid reporting the non existing foobar() method because this is also actually a caught exception and Magento uses exit(1) in \Magento\Framework\App\Bootstrap::terminate