Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1000: Respect error_reporting #1001

Closed
wants to merge 1 commit into from
Closed

Fix #1000: Respect error_reporting #1001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/ErrorHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,10 +363,10 @@ public function addExceptionHandlerListener(callable $listener): void
*/
private function handleError(int $level, string $message, string $file, int $line, ?array $errcontext = []): bool
{
if (0 === error_reporting()) {
$errorAsException = new SilencedErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);
} else {
if ($level & error_reporting()) {
$errorAsException = new \ErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);
} else {
$errorAsException = new SilencedErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);
Comment on lines +368 to +369
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something different. A SilencedErrorException is something like @foo(), related to the @ silencing operator.

Copy link
Author

@PatchRanger PatchRanger Apr 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it was designed as related to @. I see that Sentry has its own reporting level. My point is that the case of intentionally silenced by tweaking error_reporting warnings should be handled the same way.

Let's review how it should work in such particular case:

  • I have error_reporting=E_ALL.
  • My Sentry error reporting is set to WARNING: I want to see in my Sentry dashboard everything that equals or above WARNING.
  • Symfony decides to workaround missed cache issue by tweaking error_reporting level. The idea is to intentionally "skip" particular warning. I don't want to see it in my Sentry dashboard, it was silenced - just by another way than @, see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L431 .

In my view tweaking error_reporting level should be considered as another way of intentionally silencing @.

This is in fact a BC, we cannot accept it as a patch.

Ok, I understand. I am going to rebase it to another branch once we reconcile the most appropriate way of dealing with this issue.

Copy link
Author

@PatchRanger PatchRanger Apr 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying this change would mean intersecting the two settings.

You made me think, thank you :-)
I think there is no intersection:

  • Sentry Options::getErrorTypes() controls whether to report or not. It uses the same constants and defaults to E_ALL - but is totally independent of error_reporting setting: it is not controlled by tweaking error_reporting.
  • Globally defined error_reporting impacts how to report the error: either silenced or usual way. It doesn't matter how error_reporting was tweaked: either by @ or manually - does it?
  • There is another option (setCaptureSilencedErrors), which makes to report even silenced errors.

In my view it works almost the same way as we already have: just the case of 0 === error_reporting() was expanded to $level & error_reporting(). Isn't it?:)

}

$backtrace = $this->cleanBacktraceFromErrorHandlerFrames($errorAsException->getTrace(), $errorAsException->getFile(), $errorAsException->getLine());
Expand Down
7 changes: 7 additions & 0 deletions tests/phpt/error_handler_respects_error_reporting.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,15 @@ $client->getOptions()->setCaptureSilencedErrors(false);
echo 'Triggering silenced error' . PHP_EOL;

@$b++;

$errorReporting = error_reporting(E_ALL & ~E_WARNING);
include 'foo.bar';
echo 'Triggering silenced by error_reporting error' . PHP_EOL;
error_reporting($errorReporting);

?>
--EXPECT--
Triggering silenced error
Transport called
Triggering silenced error
Triggering silenced by error_reporting error