Merge pull request #1861 from joemeyer76/fix/nntp-getxover-return-type

Fix NNTPService::getXOVER() TypeError on NNTP error responses
2026-09-02 19:28:55 +00:00 · 2026-07-04 09:28:08 +02:00
parent 09ad515969 b0fd5491a0
commit f0f17def8c
2 changed files with 69 additions and 1 deletions
@@ -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();
@@ -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'));
+    }
+}