fixup! Refactor the ErrorHandler into a dynamic class

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
nextcloud · Jun 9, 2022 · 90687e4 · 90687e4
1 parent a41e99c
commit 90687e4
Show file tree

Hide file tree

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

diff --git a/lib/private/Log/ErrorHandler.php b/lib/private/Log/ErrorHandler.php
@@ -42,21 +42,8 @@
 class ErrorHandler {
 	private LoggerInterface $logger;
 
-	public function __construct(LoggerInterface $logger,
-								bool $isCli,
-								bool $debug) {
+	public function __construct(LoggerInterface $logger) {
 		$this->logger = $logger;
-
-		if ($debug) {
-			set_error_handler(Closure::fromCallable([$this, 'onAll']), E_ALL);
-			if ($isCli) {
-				set_exception_handler(Closure::fromCallable(['OC_Template', 'printExceptionErrorPage']));
-			}
-		} else {
-			set_error_handler(Closure::fromCallable([$this, 'onError']));
-		}
-		register_shutdown_function(Closure::fromCallable([$this, 'onShutdown']));
-		set_exception_handler(Closure::fromCallable([$this, 'onException']));
 	}
 
 	/**
@@ -69,7 +56,7 @@ private static function removePassword(string $msg): string {
 	/**
 	 * Fatal errors handler
 	 */
-	private function onShutdown(): void {
+	public function onShutdown(): void {
 		$error = error_get_last();
 		if ($error) {
 			$msg = $error['message'] . ' at ' . $error['file'] . '#' . $error['line'];
@@ -80,7 +67,7 @@ private function onShutdown(): void {
 	/**
 	 * Uncaught exception handler
 	 */
-	private function onException(Throwable $exception): void {
+	public function onException(Throwable $exception): void {
 		$class = get_class($exception);
 		$msg = $exception->getMessage();
 		$msg = "$class: $msg at " . $exception->getFile() . '#' . $exception->getLine();
@@ -90,7 +77,7 @@ private function onException(Throwable $exception): void {
 	/**
 	 * Recoverable errors handler
 	 */
-	private function onError(int $number, string $message, string $file, int $line): bool {
+	public function onError(int $number, string $message, string $file, int $line): bool {
 		if (!(error_reporting() & $number)) {
 			return true;
 		}
@@ -103,14 +90,14 @@ private function onError(int $number, string $message, string $file, int $line):
 	/**
 	 * Recoverable handler which catch all errors, warnings and notices
 	 */
-	private function onAll(int $number, string $message, string $file, int $line): bool {
+	public function onAll(int $number, string $message, string $file, int $line): bool {
 		$msg = $message . ' at ' . $file . '#' . $line;
 		$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,27 @@
 
 namespace Test;
 
-class ErrorHandlerTest extends \Test\TestCase {
+use Error;
+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 +70,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);
 	}
 }