From bca6abf4abaa35e290d518de3aea0a73caedffb1 Mon Sep 17 00:00:00 2001 From: Philipp Date: Mon, 17 Jul 2023 01:06:11 +0200 Subject: [PATCH] Fix logger classes and tests --- src/Core/Logger/Factory/StreamLogger.php | 6 +-- src/Core/Logger/Util/FileSystem.php | 10 ++-- .../src/Core/Addon/Model/AddonLoaderTest.php | 6 +-- .../Core/Hooks/Util/HookFileManagerTest.php | 24 +-------- tests/src/Core/Logger/StreamLoggerTest.php | 54 +++++-------------- 5 files changed, 28 insertions(+), 72 deletions(-) diff --git a/src/Core/Logger/Factory/StreamLogger.php b/src/Core/Logger/Factory/StreamLogger.php index 6da4b8cd88..caad78e3b4 100644 --- a/src/Core/Logger/Factory/StreamLogger.php +++ b/src/Core/Logger/Factory/StreamLogger.php @@ -25,10 +25,10 @@ use Friendica\Core\Config\Capability\IManageConfigValues; use Friendica\Core\Logger\Capabilities\LogChannel; use Friendica\Core\Logger\Exception\LoggerArgumentException; use Friendica\Core\Logger\Exception\LoggerException; +use Friendica\Core\Logger\Exception\LogLevelException; use Friendica\Core\Logger\Type\StreamLogger as StreamLoggerClass; use Friendica\Core\Logger\Util\FileSystem; use Psr\Log\LoggerInterface; -use Psr\Log\LogLevel; use Psr\Log\NullLogger; /** @@ -55,7 +55,7 @@ class StreamLogger extends AbstractLoggerTypeFactory $fileSystem = new FileSystem(); $logfile = $logfile ?? $config->get('system', 'logfile'); - if ((@file_exists($logfile) && !@is_writable($logfile)) && !@is_writable(basename($logfile))) { + if (!@file_exists($logfile) || !@is_writable($logfile)) { throw new LoggerArgumentException(sprintf('%s is not a valid logfile', $logfile)); } @@ -64,7 +64,7 @@ class StreamLogger extends AbstractLoggerTypeFactory if (array_key_exists($loglevel, StreamLoggerClass::levelToInt)) { $loglevel = StreamLoggerClass::levelToInt[$loglevel]; } else { - $loglevel = StreamLoggerClass::levelToInt[LogLevel::NOTICE]; + throw new LogLevelException(sprintf('The level "%s" is not valid.', $loglevel)); } $stream = $fileSystem->createStream($logfile); diff --git a/src/Core/Logger/Util/FileSystem.php b/src/Core/Logger/Util/FileSystem.php index 3793ac4a9d..41bb423b4d 100644 --- a/src/Core/Logger/Util/FileSystem.php +++ b/src/Core/Logger/Util/FileSystem.php @@ -21,6 +21,8 @@ namespace Friendica\Core\Logger\Util; +use Friendica\Core\Logger\Exception\LoggerUnusableException; + /** * Util class for filesystem manipulation for Logger classes */ @@ -37,6 +39,8 @@ class FileSystem * @param string $file The file * * @return string The directory name (empty if no directory is found, like urls) + * + * @throws LoggerUnusableException */ public function createDir(string $file): string { @@ -57,7 +61,7 @@ class FileSystem restore_error_handler(); if (!$status && !is_dir($dirname)) { - throw new \UnexpectedValueException(sprintf('Directory "%s" cannot get created: ' . $this->errorMessage, $dirname)); + throw new LoggerUnusableException(sprintf('Directory "%s" cannot get created: ' . $this->errorMessage, $dirname)); } return $dirname; @@ -75,7 +79,7 @@ class FileSystem * * @return resource the open stream resource * - * @throws \UnexpectedValueException + * @throws LoggerUnusableException */ public function createStream(string $url) { @@ -89,7 +93,7 @@ class FileSystem restore_error_handler(); if (!is_resource($stream)) { - throw new \UnexpectedValueException(sprintf('The stream or file "%s" could not be opened: ' . $this->errorMessage, $url)); + throw new LoggerUnusableException(sprintf('The stream or file "%s" could not be opened: ' . $this->errorMessage, $url)); } return $stream; diff --git a/tests/src/Core/Addon/Model/AddonLoaderTest.php b/tests/src/Core/Addon/Model/AddonLoaderTest.php index 3e9a325b14..189110fded 100644 --- a/tests/src/Core/Addon/Model/AddonLoaderTest.php +++ b/tests/src/Core/Addon/Model/AddonLoaderTest.php @@ -72,7 +72,7 @@ EOF; public function dataHooks(): array { return [ - 'normal' => [ + 'normal' => [ 'structure' => $this->structure, 'enabled' => $this->addons, 'files' => [ @@ -86,7 +86,7 @@ EOF; ], ], ], - 'double' => [ + 'double' => [ 'structure' => $this->structure, 'enabled' => $this->addons, 'files' => [ @@ -101,7 +101,7 @@ EOF; ], ], ], - 'wrongName' => [ + 'wrongName' => [ 'structure' => $this->structure, 'enabled' => $this->addons, 'files' => [ diff --git a/tests/src/Core/Hooks/Util/HookFileManagerTest.php b/tests/src/Core/Hooks/Util/HookFileManagerTest.php index 216a9832f1..f718e8a92f 100644 --- a/tests/src/Core/Hooks/Util/HookFileManagerTest.php +++ b/tests/src/Core/Hooks/Util/HookFileManagerTest.php @@ -55,20 +55,12 @@ return [ \Psr\Log\NullLogger::class => [''], ], ], - \Friendica\Core\Hooks\Capabilities\BehavioralHookType::DECORATOR => [ - \Psr\Log\LoggerInterface::class => [ - \Psr\Log\NullLogger::class, - ], - ], ]; EOF, 'addonsArray' => [], 'assertStrategies' => [ [LoggerInterface::class, NullLogger::class, ''], ], - 'assertDecorators' => [ - [LoggerInterface::class, NullLogger::class], - ], ], 'normalWithString' => [ 'content' => << '', ], ], - \Friendica\Core\Hooks\Capabilities\BehavioralHookType::DECORATOR => [ - \Psr\Log\LoggerInterface::class => \Psr\Log\NullLogger::class, - ], ]; EOF, 'addonsArray' => [], 'assertStrategies' => [ [LoggerInterface::class, NullLogger::class, ''], ], - 'assertDecorators' => [ - [LoggerInterface::class, NullLogger::class], - ], ], 'withAddons' => [ 'content' => << [], ], 'withAddonsWithString' => [ 'content' => << [], ], // This should work because unique name convention is part of the instance manager logic, not of the file-infrastructure layer 'withAddonsDoubleNamed' => [ @@ -167,7 +151,6 @@ EOF, [LoggerInterface::class, NullLogger::class, ''], [LoggerInterface::class, NullLogger::class, ''], ], - 'assertDecorators' => [], ], 'withWrongContentButAddons' => [ 'content' => << [ [LoggerInterface::class, NullLogger::class, ''], ], - 'assertDecorators' => [], ], ]; } @@ -199,7 +181,7 @@ EOF, /** * @dataProvider dataHooks */ - public function testSetupHooks(string $content, array $addonsArray, array $assertStrategies, array $assertDecorators) + public function testSetupHooks(string $content, array $addonsArray, array $assertStrategies) { vfsStream::newFile('static/hooks.config.php') ->withContent($content) @@ -215,10 +197,6 @@ EOF, $instanceManager->shouldReceive('registerStrategy')->withArgs($assertStrategy)->once(); } - foreach ($assertDecorators as $assertDecorator) { - $instanceManager->shouldReceive('registerDecorator')->withArgs($assertDecorator)->once(); - } - $hookFileManager->setupHooks($instanceManager); self::expectNotToPerformAssertions(); diff --git a/tests/src/Core/Logger/StreamLoggerTest.php b/tests/src/Core/Logger/StreamLoggerTest.php index c8fac4939d..a1e0af5bbb 100644 --- a/tests/src/Core/Logger/StreamLoggerTest.php +++ b/tests/src/Core/Logger/StreamLoggerTest.php @@ -22,9 +22,7 @@ namespace Friendica\Test\src\Core\Logger; use Friendica\Core\Logger\Exception\LoggerArgumentException; -use Friendica\Core\Logger\Exception\LoggerException; use Friendica\Core\Logger\Exception\LogLevelException; -use Friendica\Core\Logger\Util\FileSystem; use Friendica\Test\Util\VFSTrait; use Friendica\Core\Logger\Type\StreamLogger; use org\bovigo\vfs\vfsStream; @@ -40,33 +38,26 @@ class StreamLoggerTest extends AbstractLoggerTest */ private $logfile; - /** - * @var \Friendica\Core\Logger\Util\Filesystem - */ - private $fileSystem; - protected function setUp(): void { parent::setUp(); $this->setUpVfsDir(); - - $this->fileSystem = new FileSystem(); } /** * {@@inheritdoc} */ - protected function getInstance($level = LogLevel::DEBUG) + protected function getInstance($level = LogLevel::DEBUG, $logfile = 'friendica.log') { - $this->logfile = vfsStream::newFile('friendica.log') + $this->logfile = vfsStream::newFile($logfile) ->at($this->root); $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn($this->logfile->url())->once(); + $this->config->shouldReceive('get')->with('system', 'loglevel')->andReturn($level)->once(); - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem, $level); - - return $logger; + $loggerFactory = new \Friendica\Core\Logger\Factory\StreamLogger($this->introspection, 'test'); + return $loggerFactory->create($this->config); } /** @@ -83,11 +74,12 @@ class StreamLoggerTest extends AbstractLoggerTest public function testNoUrl() { $this->expectException(LoggerArgumentException::class); - $this->expectExceptionMessage("Missing stream URL."); + $this->expectExceptionMessage(' is not a valid logfile'); $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn('')->once(); - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem); + $loggerFactory = new \Friendica\Core\Logger\Factory\StreamLogger($this->introspection, 'test'); + $logger = $loggerFactory->create($this->config); $logger->emergency('not working'); } @@ -104,7 +96,8 @@ class StreamLoggerTest extends AbstractLoggerTest $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn($logfile->url())->once(); - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem); + $loggerFactory = new \Friendica\Core\Logger\Factory\StreamLogger($this->introspection, 'test'); + $logger = $loggerFactory->create($this->config); $logger->emergency('not working'); } @@ -119,7 +112,8 @@ class StreamLoggerTest extends AbstractLoggerTest static::markTestIncomplete('We need a platform independent way to set directory to readonly'); - $logger = new StreamLogger('test', '/$%/wrong/directory/file.txt', $this->introspection, $this->fileSystem); + $loggerFactory = new \Friendica\Core\Logger\Factory\StreamLogger($this->introspection, 'test'); + $logger = $loggerFactory->create($this->config); $logger->emergency('not working'); } @@ -132,9 +126,7 @@ class StreamLoggerTest extends AbstractLoggerTest $this->expectException(LogLevelException::class); $this->expectExceptionMessageMatches("/The level \".*\" is not valid./"); - $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn('file.text')->once(); - - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem, 'NOPE'); + $logger = $this->getInstance('NOPE'); } /** @@ -145,29 +137,11 @@ class StreamLoggerTest extends AbstractLoggerTest $this->expectException(LogLevelException::class); $this->expectExceptionMessageMatches("/The level \".*\" is not valid./"); - $logfile = vfsStream::newFile('friendica.log') - ->at($this->root); - - $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn($logfile->url())->once(); - - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem); + $logger = $this->getInstance('NOPE'); $logger->log('NOPE', 'a test'); } - /** - * Test when the file is null - */ - public function testWrongFile() - { - $this->expectException(LoggerArgumentException::class); - $this->expectExceptionMessage("A stream must either be a resource or a string."); - - $this->config->shouldReceive('get')->with('system', 'logfile')->andReturn(null)->once(); - - $logger = new StreamLogger('test', $this->config, $this->introspection, $this->fileSystem); - } - /** * Test a relative path * @doesNotPerformAssertions