[TASK] Handle configured error level only in error handler 87/59287/9
authorChristoph Lehmann <christoph.lehmann@networkteam.com>
Sun, 23 Dec 2018 14:13:33 +0000 (15:13 +0100)
committerAnja Leichsenring <aleichsenring@ab-softlab.de>
Fri, 5 Apr 2019 15:09:20 +0000 (17:09 +0200)
Currently there is no way to determine the error level of a previously
registered error handler.

The commit helps chaining error handlers and prevents unnecessary extra
work.

Resolves: #87281
Releases: master, 9.5
Change-Id: Ib6cc32eeb4714cae1dd16aa5382683c374615d38
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/59287
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Jigal van Hemert <jigal.van.hemert@typo3.org>
Tested-by: Mona Muzaffar <mona.muzaffar@gmx.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Jigal van Hemert <jigal.van.hemert@typo3.org>
Reviewed-by: Mona Muzaffar <mona.muzaffar@gmx.de>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
typo3/sysext/core/Classes/Error/ErrorHandler.php
typo3/sysext/core/Tests/Functional/Error/ErrorHandlerTest.php

index 16ad784..f8de654 100644 (file)
@@ -40,6 +40,13 @@ class ErrorHandler implements ErrorHandlerInterface, LoggerAwareInterface
     protected $exceptionalErrors = 0;
 
     /**
+     * Error levels which should be handled.
+     *
+     * @var int
+     */
+    protected $errorHandlerErrors = 0;
+
+    /**
      * Whether to write a flash message in case of an error
      *
      * @var bool
@@ -55,8 +62,8 @@ class ErrorHandler implements ErrorHandlerInterface, LoggerAwareInterface
     {
         $excludedErrors = E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR;
         // reduces error types to those a custom error handler can process
-        $errorHandlerErrors = $errorHandlerErrors & ~$excludedErrors;
-        set_error_handler([$this, 'handleError'], $errorHandlerErrors);
+        $this->errorHandlerErrors = $errorHandlerErrors & ~$excludedErrors;
+        set_error_handler([$this, 'handleError'], $this->errorHandlerErrors);
     }
 
     /**
@@ -96,8 +103,9 @@ class ErrorHandler implements ErrorHandlerInterface, LoggerAwareInterface
      */
     public function handleError($errorLevel, $errorMessage, $errorFile, $errorLine)
     {
-        // Don't do anything if error_reporting is disabled by an @ sign
-        if (error_reporting() === 0) {
+        // Don't do anything if error_reporting is disabled by an @ sign or $errorLevel is something we won't handle
+        $shouldHandleErrorLevel = (bool)($this->errorHandlerErrors & $errorLevel);
+        if (error_reporting() === 0 || !$shouldHandleErrorLevel) {
             return true;
         }
         $errorLevels = [
index 05c1475..a9e56f7 100644 (file)
@@ -15,6 +15,12 @@ namespace TYPO3\CMS\Core\Tests\Functional\Error;
  * The TYPO3 project - inspiring people to share!
  */
 
+use PHPUnit\Framework\MockObject\MockObject;
+use TYPO3\CMS\Core\Error\ErrorHandler;
+use TYPO3\CMS\Core\Log\Logger;
+use TYPO3\CMS\Core\Log\LogManager;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\TestingFramework\Core\AccessibleObjectInterface;
 use TYPO3\TestingFramework\Core\Functional\FunctionalTestCase;
 
 /**
@@ -42,4 +48,83 @@ class ErrorHandlerTest extends FunctionalTestCase
         );
         $this->assertTrue(true);
     }
+
+    /**
+     * This test checks the following:
+     *
+     * Normally the core error handler is registered with an error level other than E_ALL to not handle E_NOTICE errors
+     * for instance.
+     *
+     * As PHP allows to stack error handlers with different error levels it is possible to register an error handler
+     * with an E_ALL error level. As that custom handler does not know the error level of it's previously registered
+     * handler, it has no choice but to forward all occurring errors to the previous handler by calling
+     * \TYPO3\CMS\Core\Error\ErrorHandler::handleError via call_user_func. This leads to
+     * \TYPO3\CMS\Core\Error\ErrorHandler::handleError handling errors it was not registered for. Thus, there needs to
+     * be a check if \TYPO3\CMS\Core\Error\ErrorHandler should handle the incoming error.
+     *
+     * @test
+     * @group not-sqlite
+     * @group not-mssql
+     */
+    public function handleErrorOnlyHandlesRegisteredErrorLevels(): void
+    {
+        // Make sure the core error handler does not return due to error_reporting being 0
+        static::assertNotSame(0, error_reporting());
+
+        // Make sure the core error handler does not return true due to a deprecation error
+        $logManagerMock = $this->createMock(LogManager::class);
+        $logManagerMock->expects($this->never())->method('getLogger')->with('TYPO3.CMS.deprecations');
+        GeneralUtility::setSingletonInstance(LogManager::class, $logManagerMock);
+
+        /** @var Logger|MockObject $logger */
+        $logger = $this->getMockBuilder(Logger::class)
+            ->disableOriginalConstructor()
+            ->setMethods(['log'])
+            ->getMock();
+
+        // Make sure the assigned logger does not log
+        $logger->expects($this->never())->method('log');
+
+        /** @var ErrorHandler|AccessibleObjectInterface $coreErrorHandler */
+        $coreErrorHandler = new ErrorHandler(
+            E_ALL & ~(E_STRICT | E_NOTICE | E_COMPILE_WARNING | E_COMPILE_ERROR | E_CORE_WARNING | E_CORE_ERROR | E_PARSE | E_ERROR)
+        );
+        $coreErrorHandler->setLogger($logger);
+
+        $customErrorHandler = new class {
+            protected $existingHandler;
+
+            public function setExistingHandler($existingHandler)
+            {
+                $this->existingHandler = $existingHandler;
+            }
+
+            /**
+             * @param $code
+             * @param $message
+             * @param string $file
+             * @param int $line
+             * @param array $context
+             * @return bool|mixed
+             */
+            public function handleError($code, $message, $file = '', $line = 0, $context = [])
+            {
+                // process errors
+                if ($this->existingHandler !== null) {
+                    return call_user_func($this->existingHandler, $code, $message, $file, $line, $context);
+                }
+
+                return false;
+            }
+        };
+
+        $existingHandler = set_error_handler([$customErrorHandler, 'handleError'], E_ALL);
+        $customErrorHandler->setExistingHandler($existingHandler);
+
+        static::assertTrue($customErrorHandler->handleError(E_NOTICE, 'Notice error message', __FILE__, __LINE__));
+        // This assertion is the base assertion but as \TYPO3\CMS\Core\Error\ErrorHandler::handleError has a few return
+        // points that return true, the expectation on dependency objects are in place. We want to be sure that the
+        // first return point is used by checking that the method does not log anything, which happens before later
+        // return points that return true.
+    }
 }