Refactor the ErrorHandler into a dynamic class

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
nextcloud · Nov 2, 2022 · 052dcde · 052dcde
1 parent 77c6d24
commit 052dcde
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 64 deletions.
diff --git a/lib/base.php b/lib/base.php
@@ -662,9 +662,20 @@ public static function init() {
 
 		$config = \OC::$server->get(\OCP\IConfig::class);
 		if (!defined('PHPUNIT_RUN')) {
-			OC\Log\ErrorHandler::setLogger(\OC::$server->getLogger());
-			$debug = $config->getSystemValue('debug', false);
-			OC\Log\ErrorHandler::register($debug);
+			$errorHandler = new OC\Log\ErrorHandler(
+				\OCP\Server::get(\Psr\Log\LoggerInterface::class),
+			);
+			$exceptionHandler = [$errorHandler, 'onException'];
+			if ($config->getSystemValue('debug', false)) {
+				set_error_handler([$errorHandler, 'onAll'], E_ALL);
+				if (\OC::$CLI) {
+					$exceptionHandler = ['OC_Template', 'printExceptionErrorPage'];
+				}
+			} else {
+				set_error_handler([$errorHandler, 'onError']);
+			}
+			register_shutdown_function([$errorHandler, 'onShutdown']);
+			set_exception_handler($exceptionHandler);
 		}
 
 		/** @var \OC\AppFramework\Bootstrap\Coordinator $bootstrapCoordinator */

diff --git a/lib/private/Log/ErrorHandler.php b/lib/private/Log/ErrorHandler.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -27,80 +30,70 @@
  */
 namespace OC\Log;
 
+use Error;
 use OCP\ILogger;
+use Psr\Log\LoggerInterface;
+use Throwable;
 
 class ErrorHandler {
-	/** @var ILogger */
-	private static $logger;
+	private LoggerInterface $logger;
+
+	public function __construct(LoggerInterface $logger) {
+		$this->logger = $logger;
+	}
 
 	/**
-	 * remove password in URLs
-	 * @param string $msg
-	 * @return string
+	 * Remove password in URLs
 	 */
-	protected static function removePassword($msg) {
+	private static function removePassword(string $msg): string {
 		return preg_replace('#//(.*):(.*)@#', '//xxx:xxx@', $msg);
 	}
 
-	public static function register($debug = false) {
-		$handler = new ErrorHandler();
-
-		if ($debug) {
-			set_error_handler([$handler, 'onAll'], E_ALL);
-			if (\OC::$CLI) {
-				set_exception_handler(['OC_Template', 'printExceptionErrorPage']);
-			}
-		} else {
-			set_error_handler([$handler, 'onError']);
-		}
-		register_shutdown_function([$handler, 'onShutdown']);
-		set_exception_handler([$handler, 'onException']);
-	}
-
-	public static function setLogger(ILogger $logger) {
-		self::$logger = $logger;
-	}
-
-	//Fatal errors handler
-	public static function onShutdown() {
+	/**
+	 * Fatal errors handler
+	 */
+	public function onShutdown(): void {
 		$error = error_get_last();
-		if ($error && self::$logger) {
-			//ob_end_clean();
+		if ($error) {
 			$msg = $error['message'] . ' at ' . $error['file'] . '#' . $error['line'];
-			self::$logger->critical(self::removePassword($msg), ['app' => 'PHP']);
+			$this->logger->critical(self::removePassword($msg), ['app' => 'PHP']);
 		}
 	}
 
 	/**
-	 * 	Uncaught exception handler
-	 *
-	 * @param \Exception $exception
+	 * Uncaught exception handler
 	 */
-	public static function onException($exception) {
+	public function onException(Throwable $exception): void {
 		$class = get_class($exception);
 		$msg = $exception->getMessage();
 		$msg = "$class: $msg at " . $exception->getFile() . '#' . $exception->getLine();
-		self::$logger->critical(self::removePassword($msg), ['app' => 'PHP']);
+		$this->logger->critical(self::removePassword($msg), ['app' => 'PHP']);
 	}
 
-	//Recoverable errors handler
-	public static function onError($number, $message, $file, $line) {
+	/**
+	 * Recoverable errors handler
+	 */
+	public function onError(int $number, string $message, string $file, int $line): bool {
 		if (!(error_reporting() & $number)) {
-			return;
+			return true;
 		}
 		$msg = $message . ' at ' . $file . '#' . $line;
-		$e = new \Error(self::removePassword($msg));
-		self::$logger->logException($e, ['app' => 'PHP', 'level' => self::errnoToLogLevel($number)]);
+		$e = new Error(self::removePassword($msg));
+		$this->logger->log(self::errnoToLogLevel($number), $e->getMessage(), ['app' => 'PHP']);
+		return true;
 	}
 
-	//Recoverable handler which catch all errors, warnings and notices
-	public static function onAll($number, $message, $file, $line) {
+	/**
+	 * Recoverable handler which catch all errors, warnings and notices
+	 */
+	public function onAll(int $number, string $message, string $file, int $line): bool {
 		$msg = $message . ' at ' . $file . '#' . $line;
-		$e = new \Error(self::removePassword($msg));
-		self::$logger->logException($e, ['app' => 'PHP', 'level' => self::errnoToLogLevel($number)]);
+		$e = new Error(self::removePassword($msg));
+		$this->logger->log(self::errnoToLogLevel($number), $e->getMessage(), ['app' => 'PHP']);
+		return true;
 	}
 
-	public static function errnoToLogLevel(int $errno): int {
+	private static function errnoToLogLevel(int $errno): int {
 		switch ($errno) {
 			case E_USER_WARNING:
 				return ILogger::WARN;

diff --git a/tests/lib/ErrorHandlerTest.php b/tests/lib/ErrorHandlerTest.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * ownCloud
  *
@@ -22,7 +25,26 @@
 
 namespace Test;
 
-class ErrorHandlerTest extends \Test\TestCase {
+use OC\Log\ErrorHandler;
+use OCP\ILogger;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+
+class ErrorHandlerTest extends TestCase {
+
+	/** @var MockObject */
+	private LoggerInterface $logger;
+
+	private ErrorHandler $errorHandler;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->errorHandler = new ErrorHandler(
+			$this->logger
+		);
+	}
 
 	/**
 	 * provide username, password combinations for testRemovePassword
@@ -47,24 +69,19 @@ public function passwordProvider() {
 	 * @param string $username
 	 * @param string $password
 	 */
-	public function testRemovePassword($username, $password) {
+	public function testRemovePasswordFromError($username, $password) {
 		$url = 'http://'.$username.':'.$password.'@owncloud.org';
 		$expectedResult = 'http://xxx:xxx@owncloud.org';
-		$result = TestableErrorHandler::testRemovePassword($url);
+		$this->logger->expects(self::once())
+			->method('log')
+			->with(
+				ILogger::ERROR,
+				'Could not reach ' . $expectedResult . ' at file#4',
+				['app' => 'PHP'],
+			);
 
-		$this->assertEquals($expectedResult, $result);
-	}
-}
+		$result = $this->errorHandler->onError(E_USER_ERROR, 'Could not reach ' . $url, 'file', 4);
 
-/**
- * dummy class to access protected methods of \OC\Log\ErrorHandler
- */
-class TestableErrorHandler extends \OC\Log\ErrorHandler {
-
-	/**
-	 * @param string $msg
-	 */
-	public static function testRemovePassword($msg) {
-		return self::removePassword($msg);
+		self::assertTrue($result);
 	}
 }