Avoid constructing Error objects if we are going to skip notification

bugsnag · Jul 1, 2015 · 142f0f8 · 142f0f8
1 parent aa6ab28
commit 142f0f8
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 60 deletions.
diff --git a/src/Bugsnag/Client.php b/src/Bugsnag/Client.php
@@ -414,22 +414,24 @@ public function notifyError($name, $message, array $metaData = null, $severity =
     // Exception handler callback, should only be called internally by PHP's set_exception_handler
     public function exceptionHandler($exception)
     {
+        if(!$this->config->autoNotify) {
+            return;
+        }
+
         $error = Bugsnag_Error::fromPHPException($this->config, $this->diagnostics, $exception);
         $error->setSeverity("error");
-
-        if (!$error->shouldIgnore() && $this->config->autoNotify) {
-            $this->notify($error);
-        }
+        $this->notify($error);
     }
 
     // Exception handler callback, should only be called internally by PHP's set_error_handler
     public function errorHandler($errno, $errstr, $errfile = '', $errline = 0)
     {
-        $error = Bugsnag_Error::fromPHPError($this->config, $this->diagnostics, $errno, $errstr, $errfile, $errline);
-
-        if (!$error->shouldIgnore() && $this->config->autoNotify) {
-            $this->notify($error);
+        if(!$this->config->autoNotify || $this->config->shouldIgnoreErrorCode($errno)) {
+            return;
         }
+
+        $error = Bugsnag_Error::fromPHPError($this->config, $this->diagnostics, $errno, $errstr, $errfile, $errline);
+        $this->notify($error);
     }
 
     // Shutdown handler callback, called when the PHP process has finished running
@@ -440,13 +442,10 @@ public function shutdownHandler()
         $lastError = error_get_last();
 
         // Check if a fatal error caused this shutdown
-        if (!is_null($lastError) && Bugsnag_ErrorTypes::isFatal($lastError['type'])) {
+        if (!is_null($lastError) && Bugsnag_ErrorTypes::isFatal($lastError['type']) && $this->config->autoNotify && !$this->config->shouldIgnoreErrorCode($lastError['type'])) {
             $error = Bugsnag_Error::fromPHPError($this->config, $this->diagnostics, $lastError['type'], $lastError['message'], $lastError['file'], $lastError['line'], true);
             $error->setSeverity("error");
-
-            if (!$error->shouldIgnore() && $this->config->autoNotify) {
-                $this->notify($error);
-            }
+            $this->notify($error);
         }
 
         // Flush any buffered errors

diff --git a/src/Bugsnag/Configuration.php b/src/Bugsnag/Configuration.php
@@ -60,6 +60,17 @@ public function shouldNotify()
         return is_null($this->notifyReleaseStages) || (is_array($this->notifyReleaseStages) && in_array($this->releaseStage, $this->notifyReleaseStages));
     }
 
+    public function shouldIgnoreErrorCode($code)
+    {
+        if (isset($this->errorReportingLevel)) {
+            return !($this->errorReportingLevel & $code);
+        } else {
+            return !(error_reporting() & $code);
+        }
+
+        return false;
+    }
+
     public function setProjectRoot($projectRoot)
     {
         $this->projectRoot = $projectRoot;

diff --git a/src/Bugsnag/Error.php b/src/Bugsnag/Error.php
@@ -16,7 +16,6 @@ class Bugsnag_Error
     public $metaData = array();
     public $config;
     public $diagnostics;
-    public $code;
     public $previous;
     public $groupingHash;
 
@@ -74,21 +73,14 @@ public function setGroupingHash($groupingHash)
 
         return $this;
     }
-    
+
     public function setStacktrace($stacktrace)
     {
         $this->stacktrace = $stacktrace;
 
         return $this;
     }
 
-    public function setCode($code)
-    {
-        $this->code = $code;
-
-        return $this;
-    }
-
     public function setSeverity($severity)
     {
         if (!is_null($severity)) {
@@ -132,8 +124,7 @@ public function setPHPError($code, $message, $file, $line, $fatal = false)
         $this->setName(Bugsnag_ErrorTypes::getName($code))
              ->setMessage($message)
              ->setSeverity(Bugsnag_ErrorTypes::getSeverity($code))
-             ->setStacktrace($stacktrace)
-             ->setCode($code);
+             ->setStacktrace($stacktrace);
 
         return $this;
     }
@@ -156,20 +147,6 @@ public function setPrevious($exception)
         return $this;
     }
 
-    public function shouldIgnore()
-    {
-        // Check if we should ignore errors of this type
-        if (isset($this->code)) {
-            if (isset($this->config->errorReportingLevel)) {
-                return !($this->config->errorReportingLevel & $this->code);
-            } else {
-                return !(error_reporting() & $this->code);
-            }
-        }
-
-        return false;
-    }
-
     public function toArray()
     {
         $errorArray = array(
@@ -182,11 +159,11 @@ public function toArray()
             'exceptions' => $this->exceptionArray(),
             'metaData' => $this->cleanupObj($this->metaData),
         );
-        
+
         if (isset($this->groupingHash)) {
         	$errorArray['groupingHash'] = $this->groupingHash;
         }
-        
+
         return $errorArray;
     }
 

diff --git a/tests/Bugsnag/ConfigurationTest.php b/tests/Bugsnag/ConfigurationTest.php
@@ -56,4 +56,18 @@ public function testNotifier()
         $this->assertEquals($this->config->notifier['name'], "Bugsnag PHP (Official)");
         $this->assertEquals($this->config->notifier['url'], "https://bugsnag.com");
     }
+
+    public function testShouldIgnore()
+    {
+        $this->config->errorReportingLevel = E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED;
+
+        $this->assertTrue($this->config->shouldIgnoreErrorCode(E_NOTICE));
+    }
+
+    public function testShouldNotIgnore()
+    {
+        $this->config->errorReportingLevel = E_ALL;
+
+        $this->assertfalse($this->config->shouldIgnoreErrorCode(E_NOTICE));
+    }
 }
diff --git a/tests/Bugsnag/ErrorTest.php b/tests/Bugsnag/ErrorTest.php
@@ -33,24 +33,6 @@ public function testMetaDataMerging()
         $this->assertEquals($errorArray['metaData']["Testing"]["localArray"], "yo");
     }
 
-    public function testShouldIgnore()
-    {
-        $this->config->errorReportingLevel = E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED;
-
-        $this->error->setPHPError(E_NOTICE, "Broken", "file", 123);
-
-        $this->assertTrue($this->error->shouldIgnore());
-    }
-
-    public function testShouldNotIgnore()
-    {
-        $this->config->errorReportingLevel = E_ALL;
-
-        $this->error->setPHPError(E_NOTICE, "Broken", "file", 123);
-
-        $this->assertfalse($this->error->shouldIgnore());
-    }
-
     public function testFiltering()
     {
         $this->error->setMetaData(array("Testing" => array("password" => "123456")));
@@ -129,15 +111,15 @@ public function testPreviousException()
             $this->assertEquals($errorArray['exceptions'][1]['message'], 'secondly');
         }
     }
-    
+
     public function testErrorGroupingHash()
     {
         $this->error->setGroupingHash('herp#derp');
 
         $errorArray = $this->error->toArray();
         $this->assertEquals($errorArray['groupingHash'], 'herp#derp');
     }
-    
+
     public function testErrorGroupingHashNotSet()
     {
         $errorArray = $this->error->toArray();