From ea3ed561039aeda5cd47a2a174cdd137c352f580 Mon Sep 17 00:00:00 2001 From: Megan Schanz Date: Mon, 6 Jul 2026 09:13:57 -0400 Subject: [PATCH 1/3] Logger improvements WIP --- module/VuFind/src/VuFind/Log/Logger.php | 90 +++++++++++++++---- .../src/VuFind/Log/LoggerAwareTrait.php | 9 +- .../src/VuFind/View/Helper/Root/SearchBox.php | 3 + .../src/VuFindTest/Log/LoggerTest.php | 47 ++++++++++ 4 files changed, 130 insertions(+), 19 deletions(-) diff --git a/module/VuFind/src/VuFind/Log/Logger.php b/module/VuFind/src/VuFind/Log/Logger.php index 51e085bfbc60..b63aac6f8225 100644 --- a/module/VuFind/src/VuFind/Log/Logger.php +++ b/module/VuFind/src/VuFind/Log/Logger.php @@ -268,13 +268,55 @@ public function log($level, string|\Stringable $message, array $context = []): v ? self::LEVEL_MAP[$level] : $level; - if (is_array($message)) { - $context['vufind_log_details'] = $message; - $mainMonologMessage = 'Exception/Detailed log. See context for levels.'; - } else { - $mainMonologMessage = $message; + // If the details is set in context, fill in any missing parts + $context = $this->fillInMissingDetails($context); + + $this->monologLogger->log($monologLevel, $message, $context); + } + + /** + * If there are 'details' in the context that are missing any of the key keys + * (for each verbosity), use data from the next lower index, if that index is missing, + * instead use the next higher index. Leave blank as a last resort. + * + * @param mixed[] $context Additional context data + * + * @return mixed[] + */ + protected function fillInMissingDetails(array $context = []): array + { + if (!array_key_exists('details', $context) || !is_array($context['details'])){ + return $context; + } + $details = $context['details']; + $levels = [1, 2, 3, 4, 5]; + $filledDetails = []; + + foreach ($levels as $level) { + // This level has data, no need to look further + if (isset($details[$level]) && $details[$level] !== '') { + $filledDetails[$level] = $details[$level]; + continue; + } + + // Try prior index (Backfill) + if (isset($details[$level - 1]) && $details[$level - 1] !== '') { + $filledDetails[$level] = $details[$level - 1]; + continue; + } + + // Try next index (Frontfill) + if (isset($details[$level + 1]) && $details[$level + 1] !== '') { + $filledDetails[$level] = $details[$level + 1]; + continue; + } + + // Leave blank as last resort + $filledDetails[$level] = ''; } - $this->monologLogger->log($monologLevel, $mainMonologMessage, $context); + + $context['details'] = $filledDetails; + return $context; } /** @@ -299,10 +341,34 @@ public function debugNeeded($newState = null) * * @param \Exception $error Exception to log * @param \Laminas\Stdlib\Parameters $server Server metadata + * @param mixed $level Optional log level. Will determine from the + * exception if not provided. (e.g., 'err', 'warn') * * @return void */ - public function logException($error, $server) + public function logException($error, $server, $level = null) + { + $details = $this->getDetailsFromException($error, $server); + $this->log( + $level ?? $this->getSeverityFromException($error), + $details[1] ?? 'Exception/Detailed log. See context for levels.', + [ + 'details' => $details, + ] + ); + } + + /** + * Extract the error data from the exception object and server data, + * if provided, and convert it into an array with keys for each of + * the 5 verbosity levels. + * + * @param \Exception $error Exception to log + * @param \Laminas\Stdlib\Parameters $server Server metadata + * + * @return array + */ + protected function getDetailsFromException($error, $server): array { // We need to build a variety of pieces so we can supply // information at five different verbosity levels: @@ -353,21 +419,13 @@ public function logException($error, $server) } } - $errorDetails = [ + return [ 1 => $baseError, 2 => $baseError . $basicServer, 3 => $baseError . $basicServer . $basicBacktrace, 4 => $baseError . $detailedServer . $basicBacktrace, 5 => $baseError . $detailedServer . $detailedBacktrace, ]; - - $this->log( - $this->getSeverityFromException($error), - $baseError, - [ - 'details' => $errorDetails, - ] - ); } /** diff --git a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php index b479833e4a86..d7a13e75797a 100644 --- a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php +++ b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php @@ -87,14 +87,17 @@ protected function logError($msg, array $context = [], $prependClass = true) /** * Log an exception. * - * @param \Exception $exception Exception to log + * @param \Exception $exception Exception to log + * @param \Laminas\Stdlib\Parameters $server Optional server metadata + * @param mixed $level Optional log level. Will determine from the + * exception if not provided. (e.g., 'err', 'warn') * * @return void */ - public function logException(\Exception $exception): void + public function logException(\Exception $exception, $server = null, $level = null): void { if ($this->logger instanceof ExtendedLoggerInterface) { - $this->logger->logException($exception, new \Laminas\Stdlib\Parameters()); + $this->logger->logException($exception, $server ?? new \Laminas\Stdlib\Parameters(), $level); } } diff --git a/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php b/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php index e830a8e55242..28a4dfef4f69 100644 --- a/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php +++ b/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php @@ -165,6 +165,9 @@ public function autocompleteFormattingRulesJson($activeSearchClass): string $rules["VuFind:$target|$key"] = $val; } } catch (\Exception $e) { + // TODO START + $this->logException(); + // TODO END // Log a warning and ignore when we can't add the autocomplete rules for // any of the handlers $baseMsg = "Could not determine autocomplete formatting rules for {$target}."; diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php index db191452c28c..471253ef8370 100644 --- a/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php +++ b/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php @@ -136,4 +136,51 @@ public function testLogException() $logger->logException($e, $fakeServer); } } + + /** + * Test fillInMissingDetails(). + * + * @return void + */ + public function testFillInMissingDetails() + { + $mockIpReader = $this->createMock(\VuFind\Net\UserIpReader::class); + $logger = $this->getMockBuilder(\VuFind\Log\Logger::class) + ->setConstructorArgs([$mockIpReader, new \Monolog\Logger('test')]) + ->onlyMethods(['log']) + ->getMock(); + + // Test 1: Gap in the middle (Index 3 missing) + $context = [ + 'details' => [ + 1 => 'low', + 2 => 'med', + // 3 is missing + 4 => 'high', + 5 => 'ultra' + ] + ]; + + $method = new \ReflectionMethod($logger, 'fillInMissingDetails'); + $method->setAccessible(true); + + $result = $method->invoke($logger, $context); + + $this->assertEquals('med', $result['details'][3], 'Index 3 should backfill from Index 2'); + + // Test 2: Missing start (Index 1 missing) + $context2 = ['details' => [2 => 'value']]; + $result2 = $method->invoke($logger, $context2); + $this->assertEquals('value', $result2['details'][1], 'Index 1 should frontfill from Index 2'); + + // Test 3: Empty string in index + $context3 = ['details' => [1 => 'data', 2 => '', 3 => 'more data']]; + $result3 = $method->invoke($logger, $context3); + $this->assertEquals('data', $result3['details'][2], 'Empty string should be treated as missing'); + + // Test 4: No index to backfill or frontfill from + $context4 = ['details' => [1 => 'data']]; + $result4 = $method->invoke($logger, $context4); + $this->assertEquals('', $result4['details'][3], 'Index 3-5 should be empty string since no near indexes to fill from'); + } } From 25af9f1004fde398a8a351e0c1e93570d25228b3 Mon Sep 17 00:00:00 2001 From: Megan Schanz Date: Tue, 1 Sep 2026 14:25:20 -0400 Subject: [PATCH 2/3] Logger improvements --- .../VuFind/Controller/CombinedController.php | 17 +-------- module/VuFind/src/VuFind/Log/Logger.php | 9 +++-- .../src/VuFind/Log/LoggerAwareTrait.php | 4 ++ .../src/VuFind/View/Helper/Root/SearchBox.php | 37 ++----------------- .../VuFind/View/Helper/Root/SearchTabs.php | 17 +-------- .../src/VuFindTest/Log/LoggerTest.php | 10 +++-- 6 files changed, 24 insertions(+), 70 deletions(-) diff --git a/module/VuFind/src/VuFind/Controller/CombinedController.php b/module/VuFind/src/VuFind/Controller/CombinedController.php index 6bccb980bc9f..b47de4b78faa 100644 --- a/module/VuFind/src/VuFind/Controller/CombinedController.php +++ b/module/VuFind/src/VuFind/Controller/CombinedController.php @@ -176,21 +176,8 @@ public function resultsAction() } catch (\Exception $e) { // Prevent errors from any of the combined search results // from raising up to the user interface and instead just skip them - $baseMsg = "Failed get combined options for {$searchClassId}."; - $shortDetails = $e->getMessage(); - $fullDetails = (string)$e; - $this->logError( - $baseMsg, - [ - 'details' => [ - 1 => "$baseMsg $shortDetails", - 2 => "$baseMsg $shortDetails", - 3 => "$baseMsg $shortDetails", - 4 => "$baseMsg $fullDetails", - 5 => "$baseMsg $fullDetails", - ], - ] - ); + $this->logError("Failed get combined options for {$searchClassId}."); + $this->logException($e); continue; } $this->adjustQueryForSettings( diff --git a/module/VuFind/src/VuFind/Log/Logger.php b/module/VuFind/src/VuFind/Log/Logger.php index b63aac6f8225..7eee7d7d7b9f 100644 --- a/module/VuFind/src/VuFind/Log/Logger.php +++ b/module/VuFind/src/VuFind/Log/Logger.php @@ -34,6 +34,7 @@ use Psr\Log\LogLevel; use VuFind\Net\UserIpReader; +use function array_key_exists; use function in_array; use function is_array; use function is_bool; @@ -279,13 +280,13 @@ public function log($level, string|\Stringable $message, array $context = []): v * (for each verbosity), use data from the next lower index, if that index is missing, * instead use the next higher index. Leave blank as a last resort. * - * @param mixed[] $context Additional context data + * @param mixed[] $context Additional context data * * @return mixed[] */ protected function fillInMissingDetails(array $context = []): array { - if (!array_key_exists('details', $context) || !is_array($context['details'])){ + if (!array_key_exists('details', $context) || !is_array($context['details'])) { return $context; } $details = $context['details']; @@ -363,8 +364,8 @@ public function logException($error, $server, $level = null) * if provided, and convert it into an array with keys for each of * the 5 verbosity levels. * - * @param \Exception $error Exception to log - * @param \Laminas\Stdlib\Parameters $server Server metadata + * @param \Exception $error Exception to log + * @param \Laminas\Stdlib\Parameters $server Server metadata * * @return array */ diff --git a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php index d7a13e75797a..3845eb1598a1 100644 --- a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php +++ b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php @@ -34,6 +34,7 @@ use Psr\Log\LogLevel; use function get_class; +use function is_array; /** * Implementation of PSR-3 \Psr\Log\LoggerAwareTrait with some additional convenience methods. @@ -97,6 +98,9 @@ protected function logError($msg, array $context = [], $prependClass = true) public function logException(\Exception $exception, $server = null, $level = null): void { if ($this->logger instanceof ExtendedLoggerInterface) { + if (is_array($server)) { + $server = new \Laminas\Stdlib\Parameters($server); + } $this->logger->logException($exception, $server ?? new \Laminas\Stdlib\Parameters(), $level); } } diff --git a/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php b/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php index 28a4dfef4f69..066566587f33 100644 --- a/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php +++ b/module/VuFind/src/VuFind/View/Helper/Root/SearchBox.php @@ -165,26 +165,10 @@ public function autocompleteFormattingRulesJson($activeSearchClass): string $rules["VuFind:$target|$key"] = $val; } } catch (\Exception $e) { - // TODO START - $this->logException(); - // TODO END // Log a warning and ignore when we can't add the autocomplete rules for // any of the handlers - $baseMsg = "Could not determine autocomplete formatting rules for {$target}."; - $shortDetails = $e->getMessage(); - $fullDetails = (string)$e; - $this->logWarning( - $baseMsg, - [ - 'details' => [ - 1 => "$baseMsg $shortDetails", - 2 => "$baseMsg $shortDetails", - 3 => "$baseMsg $shortDetails", - 4 => "$baseMsg $fullDetails", - 5 => "$baseMsg $fullDetails", - ], - ] - ); + $this->logWarning("Could not determine autocomplete formatting rules for {$target}."); + $this->logException($e, level: 'warn'); } } } @@ -523,21 +507,8 @@ protected function getCombinedHandlers($activeSearchClass, $activeHandler, array } catch (\Exception $e) { // If we can't get the options or basic handlers for the search // target, then log it and don't add it to the search box - $baseMsg = "Missing required data for {$target}. Could not add to search box."; - $shortDetails = $e->getMessage(); - $fullDetails = (string)$e; - $this->logError( - $baseMsg, - [ - 'details' => [ - 1 => "$baseMsg $shortDetails", - 2 => "$baseMsg $shortDetails", - 3 => "$baseMsg $shortDetails", - 4 => "$baseMsg $fullDetails", - 5 => "$baseMsg $fullDetails", - ], - ] - ); + $this->logError("Missing required data for {$target}. Could not add to search box."); + $this->logException($e); continue; } if (empty($basic)) { diff --git a/module/VuFind/src/VuFind/View/Helper/Root/SearchTabs.php b/module/VuFind/src/VuFind/View/Helper/Root/SearchTabs.php index 64259d09df98..f446b44545ed 100644 --- a/module/VuFind/src/VuFind/View/Helper/Root/SearchTabs.php +++ b/module/VuFind/src/VuFind/View/Helper/Root/SearchTabs.php @@ -168,21 +168,8 @@ public function getTabConfig( } } catch (\Exception $e) { // Log the error and just don't add tabs that we couldn't get the data for - $baseMsg = "Could not add tab for {$key}."; - $shortDetails = $e->getMessage(); - $fullDetails = (string)$e; - $this->logError( - $baseMsg, - [ - 'details' => [ - 1 => "$baseMsg $shortDetails", - 2 => "$baseMsg $shortDetails", - 3 => "$baseMsg $shortDetails", - 4 => "$baseMsg $fullDetails", - 5 => "$baseMsg $fullDetails", - ], - ] - ); + $this->logError("Could not add tab for {$key}."); + $this->logException($e); continue; } $tab = [ diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php index 471253ef8370..efe550bb6e55 100644 --- a/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php +++ b/module/VuFind/tests/unit-tests/src/VuFindTest/Log/LoggerTest.php @@ -157,8 +157,8 @@ public function testFillInMissingDetails() 2 => 'med', // 3 is missing 4 => 'high', - 5 => 'ultra' - ] + 5 => 'ultra', + ], ]; $method = new \ReflectionMethod($logger, 'fillInMissingDetails'); @@ -181,6 +181,10 @@ public function testFillInMissingDetails() // Test 4: No index to backfill or frontfill from $context4 = ['details' => [1 => 'data']]; $result4 = $method->invoke($logger, $context4); - $this->assertEquals('', $result4['details'][3], 'Index 3-5 should be empty string since no near indexes to fill from'); + $this->assertEquals( + '', + $result4['details'][3], + 'Index 3-5 should be empty string since no near indexes to fill from' + ); } } From 72f7bdbc8faec7ac3f927c31829bcdae54f3c372 Mon Sep 17 00:00:00 2001 From: Megan Schanz Date: Tue, 8 Sep 2026 10:02:18 -0400 Subject: [PATCH 3/3] Add changes based on feedback --- module/VuFind/src/VuFind/Log/Logger.php | 13 +++++-------- module/VuFind/src/VuFind/Log/LoggerAwareTrait.php | 6 +++--- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/module/VuFind/src/VuFind/Log/Logger.php b/module/VuFind/src/VuFind/Log/Logger.php index 7eee7d7d7b9f..a109c4c9eb48 100644 --- a/module/VuFind/src/VuFind/Log/Logger.php +++ b/module/VuFind/src/VuFind/Log/Logger.php @@ -34,7 +34,6 @@ use Psr\Log\LogLevel; use VuFind\Net\UserIpReader; -use function array_key_exists; use function in_array; use function is_array; use function is_bool; @@ -286,7 +285,7 @@ public function log($level, string|\Stringable $message, array $context = []): v */ protected function fillInMissingDetails(array $context = []): array { - if (!array_key_exists('details', $context) || !is_array($context['details'])) { + if (!is_array($context['details'] ?? null)) { return $context; } $details = $context['details']; @@ -295,19 +294,19 @@ protected function fillInMissingDetails(array $context = []): array foreach ($levels as $level) { // This level has data, no need to look further - if (isset($details[$level]) && $details[$level] !== '') { + if (($details[$level] ?? '') !== '') { $filledDetails[$level] = $details[$level]; continue; } // Try prior index (Backfill) - if (isset($details[$level - 1]) && $details[$level - 1] !== '') { + if (($details[$level - 1] ?? '') !== '') { $filledDetails[$level] = $details[$level - 1]; continue; } // Try next index (Frontfill) - if (isset($details[$level + 1]) && $details[$level + 1] !== '') { + if (($details[$level + 1] ?? '') !== '') { $filledDetails[$level] = $details[$level + 1]; continue; } @@ -353,9 +352,7 @@ public function logException($error, $server, $level = null) $this->log( $level ?? $this->getSeverityFromException($error), $details[1] ?? 'Exception/Detailed log. See context for levels.', - [ - 'details' => $details, - ] + compact('details') ); } diff --git a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php index 3845eb1598a1..ddd7633e7d0c 100644 --- a/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php +++ b/module/VuFind/src/VuFind/Log/LoggerAwareTrait.php @@ -88,9 +88,9 @@ protected function logError($msg, array $context = [], $prependClass = true) /** * Log an exception. * - * @param \Exception $exception Exception to log - * @param \Laminas\Stdlib\Parameters $server Optional server metadata - * @param mixed $level Optional log level. Will determine from the + * @param \Exception $exception Exception to log + * @param \Laminas\Stdlib\Parameters|array|null $server Optional server metadata + * @param mixed $level Optional log level. Will determine from the * exception if not provided. (e.g., 'err', 'warn') * * @return void