From 8c36229a6570fb31996830faf436177af0e97953 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sat, 12 Sep 2026 16:40:49 -0700 Subject: [PATCH 01/15] move the enabled check to the top --- Sources/Services/ErrorHandlerService.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 793c7cb37a..335bc71c67 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -211,6 +211,11 @@ public function log(string $error_message, string|bool $error_type = 'general', static $batch_size = 10; static $shutdown_registered = false; + // Check if error logging is actually on. + if (empty(Config::$modSettings['enableErrorLogging'])) { + return $error_message; + } + $error_call++; // Collect a backtrace @@ -228,11 +233,6 @@ public function log(string $error_message, string|bool $error_type = 'general', die('Error: loop detected. The database may have failed or crashed.'); } - // Check if error logging is actually on. - if (empty(Config::$modSettings['enableErrorLogging'])) { - return $error_message; - } - // Basically, htmlspecialchars it minus &. (for entities!) $error_message = strtr($error_message, ['<' => '<', '>' => '>', '"' => '"']); From ec39bb05147b9fc03419b5f8ae659c5a577ebbf9 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sat, 12 Sep 2026 16:47:54 -0700 Subject: [PATCH 02/15] check error loop before we gather backtrace --- Sources/Services/ErrorHandlerService.php | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 335bc71c67..afbe2422c6 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -218,6 +218,16 @@ public function log(string $error_message, string|bool $error_type = 'general', $error_call++; + // Are we in a loop? The count is how deep this call is: logging an + // error is allowed to produce one more, but a third means that + // whatever this depends on fails every time it is asked, and + // going round again would not end. + if ($error_call > 2) { + var_dump($backtrace); + + die('Error: loop detected. The database may have failed or crashed.'); + } + // Collect a backtrace if (!DebugUtils::isDebugEnabled()) { $backtrace = $backtrace ?? debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); @@ -226,13 +236,6 @@ public function log(string $error_message, string|bool $error_type = 'general', $backtrace = $backtrace ?? debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS & DEBUG_BACKTRACE_PROVIDE_OBJECT); } - // Are we in a loop? - if ($error_call > 2) { - var_dump($backtrace); - - die('Error: loop detected. The database may have failed or crashed.'); - } - // Basically, htmlspecialchars it minus &. (for entities!) $error_message = strtr($error_message, ['<' => '<', '>' => '>', '"' => '"']); From 4d055e6793dbd5cbd4234368d9f47c1dd4866a62 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sat, 12 Sep 2026 16:55:10 -0700 Subject: [PATCH 03/15] the error log doesn't parse arguments; don't gather them --- Sources/Services/ErrorHandlerService.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index afbe2422c6..0646cdb802 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -229,12 +229,7 @@ public function log(string $error_message, string|bool $error_type = 'general', } // Collect a backtrace - if (!DebugUtils::isDebugEnabled()) { - $backtrace = $backtrace ?? debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); - } else { - // This is how to keep the args but skip the objects. - $backtrace = $backtrace ?? debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS & DEBUG_BACKTRACE_PROVIDE_OBJECT); - } + $backtrace = $backtrace ?? debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); // Basically, htmlspecialchars it minus &. (for entities!) $error_message = strtr($error_message, ['<' => '<', '>' => '>', '"' => '"']); From 9cf7527dfe673a3670baa9772a7e8f61e837607c Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 01:50:10 -0700 Subject: [PATCH 04/15] show exception name in log --- Sources/Services/ErrorHandlerService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 0646cdb802..fcb722093c 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -179,7 +179,7 @@ public function call(int $error_level, string $error_string, string $file, int $ */ public function catch(\Throwable $e): void { - $message = Lang::txtExists($e->getMessage(), file: 'Errors') ? Lang::getTxt($e->getMessage(), file: 'Errors') : $e->getMessage(); + $message = $e::class . ': ' . (Lang::txtExists($e->getMessage(), file: 'Errors') ? Lang::getTxt($e->getMessage(), file: 'Errors') : $e->getMessage()); if (!empty(Config::$modSettings['enableErrorLogging'])) { $this->log($message, 'general', $e->getFile(), $e->getLine(), $e->getTrace()); From c189faa15bcb73e716381fd3660aaae64da27423 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 01:56:06 -0700 Subject: [PATCH 05/15] explicitly check the empty array --- Sources/Services/ErrorHandlerService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index fcb722093c..06c8d228ac 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -337,7 +337,7 @@ public function log(string $error_message, string|bool $error_type = 'general', // Register shutdown function to flush remaining batch. if (!$shutdown_registered) { register_shutdown_function(function () use (&$error_batch) { - if (!empty($error_batch)) { + if ($error_batch !== []) { $this->flushErrorBatch($error_batch); } }); From 6d281b28b528cb41e6fc1ce4b9e81c19f5d772b6 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 02:03:55 -0700 Subject: [PATCH 06/15] filter out any backtrace entries from either of the two classes --- Sources/Services/ErrorHandlerService.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 06c8d228ac..51104491ca 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -279,8 +279,12 @@ public function log(string $error_message, string|bool $error_type = 'general', // Make sure the category that was specified is a valid one $error_type = \in_array($error_type, $this->known_error_types) && $error_type !== true ? $error_type : 'general'; - // Leave out the call to this method. - array_splice($backtrace, 0, 1); + // Remove any ErrorHandler frames from the backtrace. + $backtrace = array_filter( + $backtrace, + // Intentionally not matching exact class names here. + static fn (array $trace): bool => !isset($trace['class']) || !str_contains($trace['class'], 'ErrorHandler'), + ); // Never log call arguments or bound objects. // From 544899e0529634511bd7e4864440723460ae92b2 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 02:08:07 -0700 Subject: [PATCH 07/15] this was probably an oversight --- Sources/Services/ErrorHandlerService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 51104491ca..365420db74 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -418,7 +418,7 @@ public function fatalLang(string $error, string|bool $log = 'general', array $sp } // Attempt to load the text string. - $error_message = Lang::getTxt($error, $sprintf, file: 'Errors'); + $error_message = Lang::getTxt($error, $sprintf, file: $file); // Send a custom header if we have a custom message. if (isset($_REQUEST['js']) || isset($_REQUEST['xml']) || isset($_REQUEST['ajax'])) { From 3de7ef252ab4cc3b34c04a80f80cae7ffe860aea Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 02:12:10 -0700 Subject: [PATCH 08/15] fix batched logging --- Sources/Services/ErrorHandlerService.php | 45 +++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 365420db74..fa4eef499c 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -785,17 +785,46 @@ protected function sendHttpStatus(int $code, string $message = ''): void * Flush batched errors to database in a single multi-row operation. * This is much faster than individual inserts, especially during high-error scenarios. * - * @param array $errors Array of error info arrays to flush + * @param array $errors Array of error info arrays to flush. */ private function flushErrorBatch(array $errors): void { - if (empty($errors)) { - return; - } + $columns = [ + 'id_member' => 'int', + 'log_time' => 'int', + 'ip' => 'inet', + 'url' => 'string', + 'message' => 'string', + 'session' => 'string', + 'error_type' => 'string', + 'file' => 'string', + 'line' => 'int', + 'backtrace' => 'string', + ]; - // Insert all batched errors in one query - foreach ($errors as $error_info) { - Db::$db->error_insert($error_info); - } + $data = []; + + foreach ($errors as $error_array) { + $data[] = [ + $error_array['id_member'], + $error_array['log_time'], + $error_array['ip'], + $error_array['url'], + $error_array['message'], + $error_array['session'], + $error_array['error_type'], + $error_array['file'], + $error_array['line'], + $error_array['backtrace'], + ]; + } + + Db::$db->insert( + 'insert', + '{db_prefix}log_errors', + $columns, + $data, + [], + ); } } From 6acafc20e96165d7b88b53eb2066f3443c4153aa Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 02:38:01 -0700 Subject: [PATCH 09/15] Apply suggestion from @live627 --- Sources/Services/ErrorHandlerService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index fa4eef499c..c0d431b9ca 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -283,7 +283,7 @@ public function log(string $error_message, string|bool $error_type = 'general', $backtrace = array_filter( $backtrace, // Intentionally not matching exact class names here. - static fn (array $trace): bool => !isset($trace['class']) || !str_contains($trace['class'], 'ErrorHandler'), + static fn(array $trace): bool => !isset($trace['class']) || !str_contains($trace['class'], 'ErrorHandler'), ); // Never log call arguments or bound objects. From ebf1a514aef64bff4e15f8668ecc4d2d58ccb640 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Sun, 13 Sep 2026 18:16:42 -0700 Subject: [PATCH 10/15] Exception names should be logged, not shown publicly --- Sources/Services/ErrorHandlerService.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index c0d431b9ca..a9ed44f680 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -179,10 +179,12 @@ public function call(int $error_level, string $error_string, string $file, int $ */ public function catch(\Throwable $e): void { - $message = $e::class . ': ' . (Lang::txtExists($e->getMessage(), file: 'Errors') ? Lang::getTxt($e->getMessage(), file: 'Errors') : $e->getMessage()); + $message = Lang::txtExists($e->getMessage(), file: 'Errors') + ? Lang::getTxt($e->getMessage(), file: 'Errors') + : $e->getMessage(); if (!empty(Config::$modSettings['enableErrorLogging'])) { - $this->log($message, 'general', $e->getFile(), $e->getLine(), $e->getTrace()); + $this->log($e::class . ': ' . $message, 'general', $e->getFile(), $e->getLine(), $e->getTrace()); } $this->fatal($message, false); From f26274187ed928bd738301eb3e97b9d38dd92993 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Mon, 14 Sep 2026 17:12:33 -0700 Subject: [PATCH 11/15] tests must flush any errors immediately --- Sources/Services/ErrorHandlerService.php | 8 ++++++-- tests/Integration/Installation.php | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index a9ed44f680..2e2b56cb95 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -57,6 +57,11 @@ class ErrorHandlerService \SQLite3Exception::class => 'cache', ]; + /** + * Maximum number of errors to collect before flushing the batch. + */ + public int $batch_size = 10; + /**************** * Public methods ****************/ @@ -210,7 +215,6 @@ public function log(string $error_message, string|bool $error_type = 'general', static $tried_hook = false; static $error_call = 0; static $error_batch = []; - static $batch_size = 10; static $shutdown_registered = false; // Check if error logging is actually on. @@ -335,7 +339,7 @@ public function log(string $error_message, string|bool $error_type = 'general', } // Flush batch when threshold reached. - if (\count($error_batch) >= $batch_size) { + if (\count($error_batch) >= $this->batch_size) { $this->flushErrorBatch($error_batch); $error_batch = []; } diff --git a/tests/Integration/Installation.php b/tests/Integration/Installation.php index 4505b2675f..8fcdb851dd 100644 --- a/tests/Integration/Installation.php +++ b/tests/Integration/Installation.php @@ -82,6 +82,9 @@ private static function connect(): string // index.php builds this before anything can ask for a service. Container::init(); + // Flush any errors immediately. + Container::getInstance()->get(\SMF\Services\ErrorHandlerService::class)->batch_size = 1; + try { // non_fatal, or a refused connection ends the process with SMF's own // database error page instead of letting us report it here. From 540d877f814dcb861e2ca3f7c8719de762c2fd0d Mon Sep 17 00:00:00 2001 From: John Rayes Date: Mon, 14 Sep 2026 17:14:41 -0700 Subject: [PATCH 12/15] fix batching again --- Sources/Services/ErrorHandlerService.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 2e2b56cb95..593efa85cf 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -811,6 +811,17 @@ private function flushErrorBatch(array $errors): void $data = []; foreach ($errors as $error_array) { + if (!isset($error_array['ip'])) { + $error_array = array_combine( + array_keys($columns), + $error_array, + ); + } + + if (filter_var($error_array['ip'], FILTER_VALIDATE_IP) === false) { + $error_array['ip'] = null; + } + $data[] = [ $error_array['id_member'], $error_array['log_time'], From 509b15b1ac43786d7c321438c98fa35cae25325c Mon Sep 17 00:00:00 2001 From: John Rayes Date: Mon, 14 Sep 2026 22:18:07 -0700 Subject: [PATCH 13/15] visibility promotion --- tests/Integration/IntegrationTestCase.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index b540e9158e..c5c631b18f 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -270,7 +270,7 @@ protected function rawSetting(string $variable): ?string * * @return int The id, or 0 when nothing has ever been logged. */ - private function lastErrorId(): int + protected function lastErrorId(): int { $request = Db::$db->query( 'SELECT COALESCE(MAX(id_error), 0) AS id_error From 0c553239c0f29516e6ba0d5bff08a30e3014d357 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Wed, 16 Sep 2026 00:59:07 -0700 Subject: [PATCH 14/15] keep backtrace a list --- Sources/Services/ErrorHandlerService.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Services/ErrorHandlerService.php b/Sources/Services/ErrorHandlerService.php index 593efa85cf..fb529e2f5c 100644 --- a/Sources/Services/ErrorHandlerService.php +++ b/Sources/Services/ErrorHandlerService.php @@ -286,11 +286,11 @@ public function log(string $error_message, string|bool $error_type = 'general', $error_type = \in_array($error_type, $this->known_error_types) && $error_type !== true ? $error_type : 'general'; // Remove any ErrorHandler frames from the backtrace. - $backtrace = array_filter( + $backtrace = array_values(array_filter( $backtrace, // Intentionally not matching exact class names here. static fn(array $trace): bool => !isset($trace['class']) || !str_contains($trace['class'], 'ErrorHandler'), - ); + )); // Never log call arguments or bound objects. // From 6f17ce75ef9e21cb7080fc5404cff017cc3d09e7 Mon Sep 17 00:00:00 2001 From: John Rayes Date: Wed, 16 Sep 2026 00:59:56 -0700 Subject: [PATCH 15/15] add test --- tests/Integration/ErrorHandlerServiceTest.php | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 tests/Integration/ErrorHandlerServiceTest.php diff --git a/tests/Integration/ErrorHandlerServiceTest.php b/tests/Integration/ErrorHandlerServiceTest.php new file mode 100644 index 0000000000..2fa5714f34 --- /dev/null +++ b/tests/Integration/ErrorHandlerServiceTest.php @@ -0,0 +1,140 @@ +batch_size = 2; + + $this->assertNoErrorsLogged(); + $last_error_id = $this->lastErrorId(); + + $service->log($marker . '-0', 'Test'); + + $num_errors = $this->queryRow( + 'SELECT message + FROM {db_prefix}log_errors + WHERE message LIKE {string:pattern} + AND id_error > {int:last_error_id}', + [ + 'pattern' => $marker . '-%', + 'last_error_id' => $last_error_id, + ], + ); + + $this->assertNull($num_errors); + + ErrorHandler::log($marker . '-1', 'Test'); + + $num_errors = $this->queryRow( + 'SELECT COUNT(*) + FROM {db_prefix}log_errors + WHERE message LIKE {string:pattern} + AND id_error > {int:last_error_id}', + [ + 'pattern' => $marker . '-%', + 'last_error_id' => $last_error_id, + ], + ); + + $this->assertEquals('2', current($num_errors)); + } + + /** + * Verifies that undefined errors use the undefined_vars error type. + */ + public function tesstUndefinedErrors(): void + { + $marker = 'Undefined variable: $my_special_error_' . bin2hex(random_bytes(8)); + Config::$modSettings['enableErrorLogging'] = '1'; + + $this->assertNoErrorsLogged(); + $last_error_id = $this->lastErrorId(); + + $error_reporting = error_reporting(); + error_reporting(E_USER_WARNING); + set_error_handler(ErrorHandler::call(...)); + trigger_error($marker, E_USER_WARNING); + restore_error_handler(); + error_reporting($error_reporting); + + $row = $this->queryRow( + 'SELECT error_type + FROM {db_prefix}log_errors + WHERE message = {string:message} + ORDER BY id_error DESC + LIMIT 1', + [ + 'message' => '%: ' . $marker, + ], + ); + + $this->assertNotNull($row); + $this->assertSame('undefined_vars', $row['error_type']); + } + +/** + * Verifies that undefined errors use the undefined_vars error type and log a backtrace. + */ +public function testUndefinedErrors(): void +{ + $marker = 'Undefined variable: $my_special_error_' . bin2hex(random_bytes(8)); + Config::$modSettings['enableErrorLogging'] = '1'; + + $this->assertNoErrorsLogged(); + $last_error_id = $this->lastErrorId(); + + $error_reporting = error_reporting(); + error_reporting(E_USER_WARNING); + set_error_handler(ErrorHandler::call(...)); + trigger_error($marker, E_USER_WARNING); + restore_error_handler(); + error_reporting($error_reporting); + + $row = $this->queryRow( + 'SELECT error_type, backtrace + FROM {db_prefix}log_errors + WHERE message = {string:message} + ORDER BY id_error DESC + LIMIT 1', + [ + 'message' => E_USER_WARNING . ': ' . $marker, + ], + ); + + $this->assertNotNull($row); + $this->assertSame('undefined_vars', $row['error_type']); + + $backtrace = json_decode($row['backtrace'], true); + + $this->assertIsArray($backtrace); + $this->assertIsList($backtrace); + $this->assertNotEmpty($backtrace); + + $this->assertArrayHasKey('file', $backtrace[0]); + $this->assertArrayHasKey('function', $backtrace[0]); + $this->assertSame(__FILE__, $backtrace[0]['file']); + $this->assertSame('trigger_error', $backtrace[0]['function']); +} +}