From b0fd5491a0260caa8bcf65e31793318cc206cc90 Mon Sep 17 00:00:00 2001
From: joemeyer76 <joemeyer76@hotmail.com>
Date: Fri, 3 Jul 2026 20:21:36 -0400
Subject: [PATCH] Fix NNTPService::getXOVER() TypeError on NNTP error responses

getXOVER() declared its return type as `array|string|NNTPService`, which
does not include DariusIII\NetNntp\Error. The underlying NNTP client
legitimately returns an Error object whenever the server responds with
an error to an XOVER command (e.g. a group with no matching articles,
or a range past the group's high-water mark) -- both call sites in
BinariesService already guard for exactly this case via
NNTPService::isError($result). Because the declared return type
excluded Error, PHP raised a TypeError before either caller ever got a
chance to run that check:

    App\Services\NNTP\NNTPService::getXOVER(): Return value must be of
    type App\Services\NNTP\NNTPService|array|string, DariusIII\NetNntp\Error
    returned

In practice this crashed the first XOVER call that hit any error
response, which made historical backfill (`update:backfill` /
`multiprocessing:backfill`) unusable beyond the very first successful
chunk for a group -- backfill by its nature keeps requesting older and
older ranges until it walks off the group's actual history, at which
point the server error becomes inevitable.

Every sibling method on this class that the NNTP client can answer with
an Error object (doConnect, doQuit, getOverview, getGroups, getMessages,
getMessagesByMessageID) already declares `mixed` for this same reason.
This change brings getXOVER() in line with that existing convention
rather than introducing a new pattern.

Added a regression test that uses reflection to assert getXOVER()'s
return type permits DariusIII\NetNntp\Error (or is unrestricted via
`mixed`), plus a sanity check that NNTPService::isError() correctly
identifies Error instances. Verified the test fails against the old
`array|string|NNTPService` signature and passes against `mixed`.

Manually verified against a live NNTP server: `update:backfill` on a
real group ran 15+ chunks past the point where it previously crashed on
the very first error response, with no exceptions.
---
 app/Services/NNTP/NNTPService.php             |  2 +-
 .../NNTPServiceGetXoverReturnTypeTest.php     | 68 +++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 tests/Unit/Services/NNTPServiceGetXoverReturnTypeTest.php

diff --git a/app/Services/NNTP/NNTPService.php b/app/Services/NNTP/NNTPService.php
index 6e8729909..ad53c90b8 100644
--- a/app/Services/NNTP/NNTPService.php
+++ b/app/Services/NNTP/NNTPService.php
@@ -470,7 +470,7 @@ class NNTPService extends NntpClient
      *
      * @throws \Exception
      */
-    public function getXOVER(string $range): array|string|NNTPService
+    public function getXOVER(string $range): mixed
     {
         // Check if we are still connected.
         $connected = $this->_checkConnection();
diff --git a/tests/Unit/Services/NNTPServiceGetXoverReturnTypeTest.php b/tests/Unit/Services/NNTPServiceGetXoverReturnTypeTest.php
new file mode 100644
index 000000000..e1e510786
--- /dev/null
+++ b/tests/Unit/Services/NNTPServiceGetXoverReturnTypeTest.php
@@ -0,0 +1,68 @@
+<?php
+
+namespace Tests\Unit\Services;
+
+use App\Services\NNTP\NNTPService;
+use DariusIII\NetNntp\Error as NntpError;
+use ReflectionMethod;
+use ReflectionNamedType;
+use Tests\TestCase;
+
+final class NNTPServiceGetXoverReturnTypeTest extends TestCase
+{
+    /**
+     * Regression test for a production TypeError:
+     *
+     *   App\Services\NNTP\NNTPService::getXOVER(): Return value must be of type
+     *   App\Services\NNTP\NNTPService|array|string, DariusIII\NetNntp\Error returned
+     *
+     * getXOVER() previously declared its return type as `array|string|NNTPService`,
+     * which does not include DariusIII\NetNntp\Error. But the underlying NNTP client
+     * legitimately returns an Error object whenever the server responds with an error
+     * (e.g. to an XOVER command for a group with no matching articles), and every
+     * caller already checks for this via NNTPService::isError($result) -- see
+     * app/Services/Binaries/BinariesService.php. Because the declared return type
+     * didn't permit Error, PHP threw a TypeError before the caller ever got a chance
+     * to run that check, crashing the very first backfill/binaries pull that hit a
+     * group boundary or empty range.
+     *
+     * All sibling methods on this class (doConnect, doQuit, getOverview, getGroups,
+     * getMessages, getMessagesByMessageID) already use `mixed` for exactly this
+     * reason. This test locks getXOVER() to the same convention so the narrow union
+     * can't silently creep back in.
+     */
+    public function test_get_xover_return_type_permits_nntp_error(): void
+    {
+        $method = new ReflectionMethod(NNTPService::class, 'getXOVER');
+        $returnType = $method->getReturnType();
+
+        $this->assertNotNull($returnType, 'getXOVER() must declare a return type.');
+
+        $typeNames = $returnType instanceof ReflectionNamedType
+            ? [$returnType->getName()]
+            : array_map(
+                fn ($type) => $type->getName(),
+                method_exists($returnType, 'getTypes') ? $returnType->getTypes() : []
+            );
+
+        $isUnrestricted = in_array('mixed', $typeNames, true);
+        $permitsNntpError = in_array(NntpError::class, $typeNames, true) || in_array(\Error::class, $typeNames, true);
+
+        $this->assertTrue(
+            $isUnrestricted || $permitsNntpError,
+            'getXOVER() return type ('.(string) $returnType.') excludes '.NntpError::class.
+            ', which the NNTP client legitimately returns on server error responses. '.
+            'Every caller already guards this with NNTPService::isError(), so the '.
+            'declared type must not reject it -- use `mixed`, matching every other '.
+            'method on this class that can return an Error.'
+        );
+    }
+
+    public function test_is_error_recognizes_nntp_error_instances(): void
+    {
+        $error = new NntpError('400 no such group', 400);
+
+        $this->assertTrue(NNTPService::isError($error));
+        $this->assertFalse(NNTPService::isError('some overview data'));
+    }
+}