From 0e84a843a4e9c77df3a6191b1a6906059fb22fe5 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Mon, 21 Oct 2019 21:53:55 +0200 Subject: [PATCH 1/5] Add Fallback in case the logfile isn't accessible. - Fixes https://github.com/friendica/friendica/issues/7756#issuecomment-544227862 --- src/Factory/LoggerFactory.php | 34 ++++++++++++++++++++------------ src/Util/Logger/StreamLogger.php | 2 ++ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/Factory/LoggerFactory.php b/src/Factory/LoggerFactory.php index 55091a4879..b25d358387 100644 --- a/src/Factory/LoggerFactory.php +++ b/src/Factory/LoggerFactory.php @@ -53,9 +53,6 @@ class LoggerFactory * @param Profiler $profiler The profiler of the app * * @return LoggerInterface The PSR-3 compliant logger instance - * - * @throws \Exception - * @throws InternalServerErrorException */ public function create( Database $database, Configuration $config, Profiler $profiler) { @@ -84,12 +81,22 @@ class LoggerFactory // just add a stream in case it's either writable or not file if (!is_file($stream) || is_writable($stream)) { - static::addStreamHandler($logger, $stream, $loglevel); + try { + static::addStreamHandler($logger, $stream, $loglevel); + } catch (\Throwable $e) { + // No Logger .. + $logger = new VoidLogger(); + } } break; case 'syslog': - $logger = new SyslogLogger($this->channel, $introspection, $loglevel); + try { + $logger = new SyslogLogger($this->channel, $introspection, $loglevel); + } catch (\Throwable $e) { + // No logger ... + $logger = new VoidLogger(); + } break; case 'stream': @@ -97,7 +104,12 @@ class LoggerFactory $stream = $config->get('system', 'logfile'); // just add a stream in case it's either writable or not file if (!is_file($stream) || is_writable($stream)) { - $logger = new StreamLogger($this->channel, $stream, $introspection, $loglevel); + try { + $logger = new StreamLogger($this->channel, $stream, $introspection, $loglevel); + } catch (\Throwable $t) { + // No logger ... + $logger = new VoidLogger(); + } } else { $logger = new VoidLogger(); } @@ -210,11 +222,10 @@ class LoggerFactory case "3": return LogLevel::INFO; // legacy DATA - case "4": - return LogLevel::DEBUG; - // legacy ALL case "5": - return LogLevel::DEBUG; + // legacy ALL + case "4": + return LogLevel::DEBUG; // default if nothing set default: return $level; @@ -230,7 +241,6 @@ class LoggerFactory * * @return void * - * @throws InternalServerErrorException if the logger is incompatible to the logger factory * @throws \Exception in case of general failures */ public static function addStreamHandler($logger, $stream, $level = LogLevel::NOTICE) @@ -249,8 +259,6 @@ class LoggerFactory $fileHandler->setFormatter($formatter); $logger->pushHandler($fileHandler); - } else { - throw new InternalServerErrorException('Logger instance incompatible for MonologFactory'); } } diff --git a/src/Util/Logger/StreamLogger.php b/src/Util/Logger/StreamLogger.php index 3031461061..c9c245d63b 100644 --- a/src/Util/Logger/StreamLogger.php +++ b/src/Util/Logger/StreamLogger.php @@ -81,6 +81,8 @@ class StreamLogger extends AbstractLogger } else { throw new \InvalidArgumentException(sprintf('The level "%s" is not valid.', $level)); } + + $this->checkStream(); } public function close() From 6b2c28e2d72d618bd46b9264f9e348725d7b5f5f Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Tue, 22 Oct 2019 22:47:37 +0200 Subject: [PATCH 2/5] Add checks & realpath() usage - New util class "FileSystem" - Add check in admin summary too --- src/Factory/LoggerFactory.php | 13 +++-- src/Module/Admin/Summary.php | 19 +++++-- src/Util/FileSystem.php | 64 ++++++++++++++++++++++ src/Util/Logger/StreamLogger.php | 50 +++-------------- tests/src/Util/Logger/StreamLoggerTest.php | 43 ++++++++++++--- 5 files changed, 129 insertions(+), 60 deletions(-) create mode 100644 src/Util/FileSystem.php diff --git a/src/Factory/LoggerFactory.php b/src/Factory/LoggerFactory.php index b25d358387..d177faf7a9 100644 --- a/src/Factory/LoggerFactory.php +++ b/src/Factory/LoggerFactory.php @@ -6,6 +6,7 @@ use Friendica\Core\Config\Configuration; use Friendica\Core\Logger; use Friendica\Database\Database; use Friendica\Network\HTTPException\InternalServerErrorException; +use Friendica\Util\FileSystem; use Friendica\Util\Introspection; use Friendica\Util\Logger\Monolog\DevelopHandler; use Friendica\Util\Logger\Monolog\IntrospectionProcessor; @@ -51,10 +52,11 @@ class LoggerFactory * @param Database $database The Friendica Database instance * @param Configuration $config The config * @param Profiler $profiler The profiler of the app + * @param FileSystem $fileSystem FileSystem utils * * @return LoggerInterface The PSR-3 compliant logger instance */ - public function create( Database $database, Configuration $config, Profiler $profiler) + public function create(Database $database, Configuration $config, Profiler $profiler, FileSystem $fileSystem) { if (empty($config->get('system', 'debugging', false))) { $logger = new VoidLogger(); @@ -105,7 +107,7 @@ class LoggerFactory // just add a stream in case it's either writable or not file if (!is_file($stream) || is_writable($stream)) { try { - $logger = new StreamLogger($this->channel, $stream, $introspection, $loglevel); + $logger = new StreamLogger($this->channel, $stream, $introspection, $fileSystem, $loglevel); } catch (\Throwable $t) { // No logger ... $logger = new VoidLogger(); @@ -137,13 +139,14 @@ class LoggerFactory * * @param Configuration $config The config * @param Profiler $profiler The profiler of the app + * @param FileSystem $fileSystem FileSystem utils * * @return LoggerInterface The PSR-3 compliant logger instance * * @throws InternalServerErrorException * @throws \Exception */ - public static function createDev(Configuration $config, Profiler $profiler) + public static function createDev(Configuration $config, Profiler $profiler, FileSystem $fileSystem) { $debugging = $config->get('system', 'debugging'); $stream = $config->get('system', 'dlogfile'); @@ -183,7 +186,7 @@ class LoggerFactory case 'stream': default: - $logger = new StreamLogger(self::DEV_CHANNEL, $stream, $introspection, LogLevel::DEBUG); + $logger = new StreamLogger(self::DEV_CHANNEL, $stream, $introspection, $fileSystem, LogLevel::DEBUG); break; } @@ -225,7 +228,7 @@ class LoggerFactory case "5": // legacy ALL case "4": - return LogLevel::DEBUG; + return LogLevel::DEBUG; // default if nothing set default: return $level; diff --git a/src/Module/Admin/Summary.php b/src/Module/Admin/Summary.php index d0bb4347a1..e1952f294b 100644 --- a/src/Module/Admin/Summary.php +++ b/src/Module/Admin/Summary.php @@ -14,6 +14,7 @@ use Friendica\Model\Register; use Friendica\Module\BaseAdminModule; use Friendica\Util\ConfigFileLoader; use Friendica\Util\DateTimeFormat; +use Friendica\Util\FileSystem; use Friendica\Util\Network; class Summary extends BaseAdminModule @@ -76,11 +77,21 @@ class Summary extends BaseAdminModule // Check logfile permission if (Config::get('system', 'debugging')) { - $stream = Config::get('system', 'logfile'); + $file = Config::get('system', 'logfile'); - if (is_file($stream) && - !is_writeable($stream)) { - $warningtext[] = L10n::t('The logfile \'%s\' is not writable. No logging possible', $stream); + /** @var FileSystem $fileSystem */ + $fileSystem = self::getClass(FileSystem::class); + + try { + $stream = $fileSystem->createStream($file); + + if (is_file($stream) && + !is_writeable($stream)) { + $warningtext[] = L10n::t('The logfile \'%s\' is not writable. No logging possible', $stream); + } + + } catch (\Throwable $exception) { + $warningtext[] = L10n::t('The logfile \'%s\' is not usable. No logging possible (error: \'%s\')', $file, $exception->getMessage()); } $stream = Config::get('system', 'dlogfile'); diff --git a/src/Util/FileSystem.php b/src/Util/FileSystem.php new file mode 100644 index 0000000000..33e04788e6 --- /dev/null +++ b/src/Util/FileSystem.php @@ -0,0 +1,64 @@ +errorMessage, $dirname)); + } + + return $dirname; + } elseif (isset($dirname) && is_dir($dirname)) { + return $dirname; + } else { + return ''; + } + } + + public function createStream(string $url) + { + $directory = $this->createDir($url); + set_error_handler([$this, 'customErrorHandler']); + if (!empty($directory)) { + $url = $directory . DIRECTORY_SEPARATOR . pathinfo($url, PATHINFO_BASENAME); + } + + $stream = fopen($url, 'ab'); + restore_error_handler(); + + if (!is_resource($stream)) { + throw new \UnexpectedValueException(sprintf('The stream or file "%s" could not be opened: ' . $this->errorMessage, $url)); + } + + return $stream; + } + + private function customErrorHandler($code, $msg) + { + $this->errorMessage = preg_replace('{^(fopen|mkdir)\(.*?\): }', '', $msg); + } +} diff --git a/src/Util/Logger/StreamLogger.php b/src/Util/Logger/StreamLogger.php index c9c245d63b..600da14fc2 100644 --- a/src/Util/Logger/StreamLogger.php +++ b/src/Util/Logger/StreamLogger.php @@ -3,6 +3,7 @@ namespace Friendica\Util\Logger; use Friendica\Util\DateTimeFormat; +use Friendica\Util\FileSystem; use Friendica\Util\Introspection; use Psr\Log\LogLevel; @@ -35,11 +36,11 @@ class StreamLogger extends AbstractLogger */ private $pid; + /** - * An error message - * @var string + * @var FileSystem */ - private $errorMessage; + private $fileSystem; /** * Translates LogLevel log levels to integer values @@ -63,8 +64,10 @@ class StreamLogger extends AbstractLogger * * @throws \Exception */ - public function __construct($channel, $stream, Introspection $introspection, $level = LogLevel::DEBUG) + public function __construct($channel, $stream, Introspection $introspection, FileSystem $fileSystem, $level = LogLevel::DEBUG) { + $this->fileSystem = $fileSystem; + parent::__construct($channel, $introspection); if (is_resource($stream)) { @@ -157,43 +160,6 @@ class StreamLogger extends AbstractLogger throw new \LogicException('Missing stream URL.'); } - $this->createDir(); - set_error_handler([$this, 'customErrorHandler']); - $this->stream = fopen($this->url, 'ab'); - restore_error_handler(); - - if (!is_resource($this->stream)) { - $this->stream = null; - - throw new \UnexpectedValueException(sprintf('The stream or file "%s" could not be opened: ' . $this->errorMessage, $this->url)); - } - } - - private function createDir() - { - $dirname = null; - $pos = strpos($this->url, '://'); - if (!$pos) { - $dirname = dirname($this->url); - } - - if (substr($this->url, 0, 7) === 'file://') { - $dirname = dirname(substr($this->url, 7)); - } - - if (isset($dirname) && !is_dir($dirname)) { - set_error_handler([$this, 'customErrorHandler']); - $status = mkdir($dirname, 0777, true); - restore_error_handler(); - - if (!$status && !is_dir($dirname)) { - throw new \UnexpectedValueException(sprintf('Directory "%s" cannot get created: ' . $this->errorMessage, $dirname)); - } - } - } - - private function customErrorHandler($code, $msg) - { - $this->errorMessage = preg_replace('{^(fopen|mkdir)\(.*?\): }', '', $msg); + $this->stream = $this->fileSystem->createStream($this->url); } } diff --git a/tests/src/Util/Logger/StreamLoggerTest.php b/tests/src/Util/Logger/StreamLoggerTest.php index d42ba1d914..7dcb08ba6d 100644 --- a/tests/src/Util/Logger/StreamLoggerTest.php +++ b/tests/src/Util/Logger/StreamLoggerTest.php @@ -2,6 +2,7 @@ namespace Friendica\Test\src\Util\Logger; +use Friendica\Util\FileSystem; use Friendica\Test\Util\VFSTrait; use Friendica\Util\Logger\StreamLogger; use org\bovigo\vfs\vfsStream; @@ -22,11 +23,18 @@ class StreamLoggerTest extends AbstractLoggerTest */ private $logfile; + /** + * @var Filesystem + */ + private $fileSystem; + protected function setUp() { parent::setUp(); $this->setUpVfsDir(); + + $this->fileSystem = new Filesystem(); } /** @@ -37,7 +45,7 @@ class StreamLoggerTest extends AbstractLoggerTest $this->logfile = vfsStream::newFile('friendica.log') ->at($this->root); - $this->logger = new StreamLogger('test', $this->logfile->url(), $this->introspection, $level); + $this->logger = new StreamLogger('test', $this->logfile->url(), $this->introspection, $this->fileSystem, $level); return $this->logger; } @@ -60,7 +68,7 @@ class StreamLoggerTest extends AbstractLoggerTest $filehandler = fopen($logfile->url(), 'ab'); - $logger = new StreamLogger('test', $filehandler, $this->introspection); + $logger = new StreamLogger('test', $filehandler, $this->introspection, $this->fileSystem); $logger->emergency('working'); $text = $logfile->getContent(); @@ -76,7 +84,7 @@ class StreamLoggerTest extends AbstractLoggerTest $logfile = vfsStream::newFile('friendica.log') ->at($this->root); - $logger = new StreamLogger('test', $logfile->url(), $this->introspection); + $logger = new StreamLogger('test', $logfile->url(), $this->introspection, $this->fileSystem); $logger->emergency('working'); $logger->close(); // close doesn't affect @@ -94,7 +102,7 @@ class StreamLoggerTest extends AbstractLoggerTest */ public function testNoUrl() { - $logger = new StreamLogger('test', '', $this->introspection); + $logger = new StreamLogger('test', '', $this->introspection, $this->fileSystem); $logger->emergency('not working'); } @@ -109,7 +117,7 @@ class StreamLoggerTest extends AbstractLoggerTest $logfile = vfsStream::newFile('friendica.log') ->at($this->root)->chmod(0); - $logger = new StreamLogger('test', $logfile->url(), $this->introspection); + $logger = new StreamLogger('test', $logfile->url(), $this->introspection, $this->fileSystem); $logger->emergency('not working'); } @@ -123,7 +131,7 @@ class StreamLoggerTest extends AbstractLoggerTest { $this->markTestIncomplete('We need a platform independent way to set directory to readonly'); - $logger = new StreamLogger('test', '/$%/wrong/directory/file.txt', $this->introspection); + $logger = new StreamLogger('test', '/$%/wrong/directory/file.txt', $this->introspection, $this->fileSystem); $logger->emergency('not working'); } @@ -135,7 +143,7 @@ class StreamLoggerTest extends AbstractLoggerTest */ public function testWrongMinimumLevel() { - $logger = new StreamLogger('test', 'file.text', $this->introspection, 'NOPE'); + $logger = new StreamLogger('test', 'file.text', $this->introspection, $this->fileSystem, 'NOPE'); } /** @@ -148,7 +156,7 @@ class StreamLoggerTest extends AbstractLoggerTest $logfile = vfsStream::newFile('friendica.log') ->at($this->root); - $logger = new StreamLogger('test', $logfile->url(), $this->introspection); + $logger = new StreamLogger('test', $logfile->url(), $this->introspection, $this->fileSystem); $logger->log('NOPE', 'a test'); } @@ -160,6 +168,23 @@ class StreamLoggerTest extends AbstractLoggerTest */ public function testWrongFile() { - $logger = new StreamLogger('test', null, $this->introspection); + $logger = new StreamLogger('test', null, $this->introspection, $this->fileSystem); + } + + /** + * Test a relative path + */ + public function testRealPath() + { + $this->markTestSkipped('vfsStream isn\'t compatible with chdir, so not testable.'); + + $logfile = vfsStream::newFile('friendica.log') + ->at($this->root); + + chdir($this->root->getChild('logs')->url()); + + $logger = new StreamLogger('test', '../friendica.log' , $this->introspection, $this->fileSystem); + + $logger->info('Test'); } } From 4e32d46f97c8b342720ce919193a04eca10dc586 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Tue, 22 Oct 2019 22:48:54 +0200 Subject: [PATCH 3/5] switch case --- src/Factory/LoggerFactory.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Factory/LoggerFactory.php b/src/Factory/LoggerFactory.php index d177faf7a9..f21fe9b7f5 100644 --- a/src/Factory/LoggerFactory.php +++ b/src/Factory/LoggerFactory.php @@ -225,9 +225,9 @@ class LoggerFactory case "3": return LogLevel::INFO; // legacy DATA - case "5": - // legacy ALL case "4": + // legacy ALL + case "5": return LogLevel::DEBUG; // default if nothing set default: From 1fe9b789f3bceafa0cf44096d47b360d61e45276 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Tue, 22 Oct 2019 22:51:52 +0200 Subject: [PATCH 4/5] Add some PHP doc --- src/Util/FileSystem.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/Util/FileSystem.php b/src/Util/FileSystem.php index 33e04788e6..b3a0ae74de 100644 --- a/src/Util/FileSystem.php +++ b/src/Util/FileSystem.php @@ -2,6 +2,9 @@ namespace Friendica\Util; +/** + * Util class for filesystem manipulation + */ final class FileSystem { /** @@ -9,6 +12,13 @@ final class FileSystem */ private $errorMessage; + /** + * Creates a directory based on a file, which gets accessed + * + * @param string $file The file + * + * @return string The directory name (empty if no directory is found, like urls) + */ public function createDir(string $file) { $dirname = null; @@ -39,6 +49,13 @@ final class FileSystem } } + /** + * Creates a stream based on a URL (could be a local file or a real URL) + * + * @param string $url The file/url + * + * @return false|resource the open stream ressource + */ public function createStream(string $url) { $directory = $this->createDir($url); From 04a86dad753c8cea0df87016255a52660165bfb3 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Tue, 22 Oct 2019 22:52:40 +0200 Subject: [PATCH 5/5] remove superfluous line --- src/Util/Logger/StreamLogger.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Util/Logger/StreamLogger.php b/src/Util/Logger/StreamLogger.php index 600da14fc2..7485e750b8 100644 --- a/src/Util/Logger/StreamLogger.php +++ b/src/Util/Logger/StreamLogger.php @@ -36,7 +36,6 @@ class StreamLogger extends AbstractLogger */ private $pid; - /** * @var FileSystem */