Suppressed PHP warning is send to Sentry

[bobvandevijver]

Environment

How do you use Sentry?
Self hosted 21.2.0

Which SDK and version?

bobvandevijver@webdev:/var/www/dev$ composer show | grep sentry
sentry/sdk                              3.1.0     This is a metapackage shipping sentry/sentry with a recommended HTTP client.
sentry/sentry                           3.2.0     A PHP SDK for Sentry (http://sentry.io)
sentry/sentry-symfony                   4.0.3     Symfony integration for Sentry (http://getsentry.com)

We're using PHP 7.3.27 on Debian 10. The issue started after upgrading to v4 of the sentry/sentry-symfony. This upgrade included:

Package	Old version	New version
sentry/sdk	2.2.0	3.1.0
sentry/sentry	2.5.2	3.2.0
sentry/sentry-symfony	3.5.3	4.0.3

Steps to Reproduce

Symfony tries to silence an include warning during the HTTP Kernel initialization, as the container cache is allowed to not exist. If that is the case, the container variables is reset and the application should continue without any notification whatsoever. The error reporting is restored directly after the "critical" part in the finally handler.

See the Kernel class implementation of Symfony, line 443 for the error level adjustment, line 469 for the line that generates the warning and the finally block on 480-482 that restores the error reporting.

This means that #1183 and #1087 are probably related to this particular issue.

For this particular case the line marked as broken code in #1183 was actually doing its job:

So, a small reproduction would be:

$errorLevel = error_reporting(\E_ALL ^ \E_WARNING);
try {
  include 'non-existing-file.php';
} finally {
  error_reporting($errorLevel);
}

Expected Result

The error should not be logged into Sentry, as it is suppressed using an explicit error_reporting call.

Actual Result

But is isn't...

[Jean85]

The PHP error reporting doesn't actually influence what Sentry reports, the SDK has a dedicated error_types option with the same bitmask usage to do that; and the silenced errors interception are meant for the @ operator.

I don't know if we should use the same interception for those cases too, but in any way those error could be reported anyway if you have capture_silenced_errors set to true.

As for the Symfony specific situation, is that happening during cache warmup? I didn't encounter this issue since I warm it up in the build and not in prod...

[bobvandevijver]

Using the error_types is no valid solution here, as the purpose is to temporarily block only the warning emitted here, and not the errors. See line 442 of the Kernel class:

// Silence E_WARNING to ignore "include" failures - don't use "@" to prevent silencing fatal errors

And I actually do not see any reference to the error_types option in the Sentry error handler, but that is probably checked elsewhere then.

capture_silenced_errors is not set, so it defaults to false, which is confirmed by the Sentry screenshot as an ErrorException is reported instead of a SilencedErrorException.

This indeed happens during the cache:clear command. It didn't happen with the previous Sentry version, so at least the behaviour changed since then.

The PHP error reporting doesn't actually influence what Sentry reports

But if this is the case, then why is the PHP error_reporting function used to determine whether it is a silenced error? Currently, on PHP 7 it only checks for the value 0, which means that only when all error reporting has been disabled, it is marked as silenced. I guess bringing back the error_reporting() & $level would already solve this issue as you're then actually checking whether the configured PHP bitmask marks the error passed to the error handler as being silenced or not...

[Jean85]

And I actually do not see any reference to the error_types option in the Sentry error handler, but that is probably checked elsewhere then.

Yes, that's checked afterwards, in the integration:

sentry-php/src/Integration/ErrorListenerIntegration.php

Lines 38 to 40 in f35ffc8

	if (!($client->getOptions()->getErrorTypes() & $exception->getSeverity())) {
	return;
	}

But if this is the case, then why is the PHP error_reporting function used to determine whether it is a silenced error? Currently, on PHP 7 it only checks for the value 0, which means that only when all error reporting has been disabled, it is marked as silenced. I guess bringing back the error_reporting() & $level would already solve this issue as your then actually checking whether the configured PHP bitmask marks the error passed to the error handler as being silenced or not...

That's possibly true. As you correctly pointed out, #1183 and #1087 changed this behavior, making those errors no longer silenced. IMO we should revert it, but I would like to ask @ste93cry & @stayallive for their opinion.

[stayallive]

Hi @bobvandevijver,

But if this is the case, then why is the PHP error_reporting function used to determine whether it is a silenced error?

The current definition of a silenced error is this in the Sentry SDK:

sentry-php/src/Exception/SilencedErrorException.php

Lines 7 to 10 in f35ffc8

	/**
	* Error that has been captured by the {@see ErrorHandler}, but that was silenced
	* with `@` in the source code.
	*/

When the Sentry client is initialised the value of error_reporting() is copied and determines what is being reported to Sentry regardless of what happens to the error_reporting() value afterwards.

There has been a serie of changes that led to this behaviour unfortunately but right now the error_types is set to a static value when the Sentry SDK is initialised.

We could change this to use error_types() whenever it's checking what errors to allow to be reported unless the option is set by the user in the code @Jean85 mentioned.

This would solve your issue and some concerns raised by my fixes of the silenced error detection code.

[Jean85]

@stayallive your proposed fix would only work if the error_types is not already used by the user. IMHO we should revert to the previous behaviour, which contradicts the comment linked by you: apart from @ silenced errors, errors below the error_reporting() limit were marked as silenced too by the Sentry SDK.

[EDIT] I discussed the issue with @stayallive, I agree that his proposed solution is the only way forward without breaking the documented behaviour; the only issue is a possible BC of the return type form Option::getErrorTypes, but he already suggested this as a solution:

    public function getErrorTypes(): int
    {
        return $this->options['error_types'] ?? error_reporting();
    }

[bobvandevijver]

Sounds like a valid solution to me! I just tested it in my environment with the changeset below, and at least for my described use case it works as I would expect.

diff --git a/src/Options.php b/src/Options.php
index 8b5c5bd..486b4fc 100644
--- a/src/Options.php
+++ b/src/Options.php
@@ -411,7 +411,7 @@ public function setTags(array $tags): void
      */
     public function getErrorTypes(): int
     {
-        return $this->options['error_types'];
+        return $this->options['error_types'] ?? error_reporting();
     }

     /**
@@ -716,7 +716,7 @@ private function configureOptions(OptionsResolver $resolver): void
                 return $event;
             },
             'tags' => [],
-            'error_types' => error_reporting(),
+            'error_types' => null,
             'max_breadcrumbs' => self::DEFAULT_MAX_BREADCRUMBS,
             'before_breadcrumb' => static function (Breadcrumb $breadcrumb): Breadcrumb {
                 return $breadcrumb;
@@ -748,7 +748,7 @@ private function configureOptions(OptionsResolver $resolver): void
         $resolver->setAllowedTypes('server_name', 'string');
         $resolver->setAllowedTypes('before_send', ['callable']);
         $resolver->setAllowedTypes('tags', 'string[]');
-        $resolver->setAllowedTypes('error_types', ['int']);
+        $resolver->setAllowedTypes('error_types', ['null', 'int']);
         $resolver->setAllowedTypes('max_breadcrumbs', 'int');
         $resolver->setAllowedTypes('before_breadcrumb', ['callable']);
         $resolver->setAllowedTypes('integrations', ['Sentry\\Integration\\IntegrationInterface[]', 'callable']);

[stayallive]

Cool. I'll brew up an PR to make this change. I think this is a good solution to keep as many options open as possible 👌