refactor: more robust download handling and improved tests

Author: thorsten

phpmyfaq/src/phpMyFAQ/Setup/Upgrade.php

@@ -34,6 +34,7 @@
 use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
 use ZipArchive;
 
 class Upgrade extends AbstractSetup
@@ -48,14 +49,19 @@ class Upgrade extends AbstractSetup
 
     private bool $isNightly;
 
+    private HttpClientInterface $httpClient;
+
     public function __construct(
         protected System $system,
         private readonly Configuration $configuration,
+        ?HttpClientInterface $httpClient = null,
     ) {
         parent::__construct($this->system);
 
         $this->isNightly =
             $this->configuration->get(item: 'upgrade.releaseEnvironment') === ReleaseType::NIGHTLY->value;
+
+        $this->httpClient = $httpClient ?? HttpClient::create(['timeout' => 60]);
     }
 
     /**
@@ -125,25 +131,40 @@ public function downloadPackage(string $version): string
     {
         $url = $this->getDownloadHost() . $this->getPath() . $this->getFilename($version);
 
-        $httpClient = HttpClient::create(['timeout' => 60]);
+        $attempts = 3;
+        $lastExceptionMessage = null;
 
-        try {
-            $response = $httpClient->request('GET', $url);
+        for ($i = 0; $i < $attempts; $i++) {
+            try {
+                $response = $this->httpClient->request('GET', $url);
 
-            if ($response->getStatusCode() !== 200) {
-                throw new Exception('Cannot download package (HTTP Status: ' . $response->getStatusCode() . ').');
-            }
+                if ($response->getStatusCode() !== 200) {
+                    throw new Exception('Cannot download package (HTTP Status: ' . $response->getStatusCode() . ').');
+                }
 
-            $package = $response->getContent();
+                $package = $response->getContent();
 
-            file_put_contents($this->upgradeDirectory . DIRECTORY_SEPARATOR . $this->getFilename($version), $package);
+                $targetPath = $this->upgradeDirectory . DIRECTORY_SEPARATOR . $this->getFilename($version);
+                file_put_contents($targetPath, $package);
 
-            return $this->upgradeDirectory . DIRECTORY_SEPARATOR . $this->getFilename($version);
-        } catch (
-            TransportExceptionInterface|ClientExceptionInterface|RedirectionExceptionInterface|ServerExceptionInterface $e
-        ) {
-            throw new Exception($e->getMessage());
+                return $targetPath;
+            } catch (
+                TransportExceptionInterface|ClientExceptionInterface|RedirectionExceptionInterface|ServerExceptionInterface $exception
+            ) {
+                $lastExceptionMessage = $exception->getMessage();
+
+                // After the last attempt, throw the exception outward
+                if ($i === ($attempts - 1)) {
+                    throw new Exception('Download failed after ' . $attempts . ' attempts: ' . $lastExceptionMessage);
+                }
+
+                // Short sleep to mitigate transient network issues
+                usleep(250000); // 250ms
+            }
         }
+
+        // Should not be reached, but for safety
+        throw new Exception('Download failed: ' . ($lastExceptionMessage ?? 'unknown error'));
     }
 
     /**
@@ -155,8 +176,7 @@ public function downloadPackage(string $version): string
      */
     public function verifyPackage(string $path, string $version): bool
     {
-        $httpClient = HttpClient::create(['timeout' => 30]);
-        $response = $httpClient->request('GET', DownloadHostType::PHPMYFAQ->value . 'info/' . $version);
+        $response = $this->httpClient->request('GET', DownloadHostType::PHPMYFAQ->value . 'info/' . $version);
 
         try {
             $responseContent = json_decode($response->getContent(), true, 512, JSON_THROW_ON_ERROR);
@@ -267,8 +287,9 @@ public function installPackage(callable $progressCallback): bool
         $currentFile = 0;
 
         foreach ($sourceDirIterator as $item) {
-            $source = $item->getPathName();
-            $destination = $destinationDir . DIRECTORY_SEPARATOR . $sourceDirIterator->getSubPathName();
+            $source = $item->getRealPath();
+            $relativePath = str_replace($sourceDir, '', $source);
+            $destination = $destinationDir . DIRECTORY_SEPARATOR . $relativePath;
 
             if ($item->isDir()) {
                 if (!is_dir($destination)) {

tests/phpMyFAQ/Setup/UpgradeTest.php

@@ -7,42 +7,67 @@
 use phpMyFAQ\Database\Sqlite3;
 use phpMyFAQ\Enums\DownloadHostType;
 use phpMyFAQ\System;
+use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
+use Symfony\Contracts\HttpClient\ResponseInterface;
 
 class UpgradeTest extends TestCase
 {
     private Upgrade $upgrade;
+    private HttpClientInterface|MockObject $httpClientMock;
+
     protected function setUp(): void
     {
         parent::setUp();
 
         $dbHandle = new Sqlite3();
         $dbHandle->connect(PMF_TEST_DIR . '/test.db', '', '');
         $configuration = new Configuration($dbHandle);
-        $this->upgrade = new Upgrade(new System(), $configuration);
+
+        $this->httpClientMock = $this->createMock(HttpClientInterface::class);
+        $this->upgrade = new Upgrade(new System(), $configuration, $this->httpClientMock);
         $this->upgrade->setUpgradeDirectory(PMF_CONTENT_DIR . '/upgrades');
     }
 
     /**
      * @throws Exception
      */
-    public function testDownloadPackage(): void
+    public function testDownloadPackageSuccessful(): void
+    {
+        $response = $this->createMock(ResponseInterface::class);
+        $response->method('getStatusCode')->willReturn(200);
+        $response->method('getContent')->willReturn('zip-binary-content');
+
+        $this->httpClientMock
+            ->expects($this->once())
+            ->method('request')
+            ->with('GET', $this->isString())
+            ->willReturn($response);
+
+        $path = $this->upgrade->downloadPackage('3.1.15');
+
+        $this->assertIsString($path);
+        $this->assertFileExists($path);
+        $this->assertSame('zip-binary-content', file_get_contents($path));
+    }
+
+    /**
+     * @throws Exception
+     */
+    public function testDownloadPackageThrowsOnHttpError(): void
     {
-        if (!$this->isNetworkAvailable()) {
-            $this->markTestSkipped('Network not available');
-        }
-
-        try {
-            $actual = $this->upgrade->downloadPackage('3.1.15');
-            $this->assertIsString($actual);
-        } catch (Exception $e) {
-            if (str_contains($e->getMessage(), 'timeout') || str_contains($e->getMessage(), 'connection')) {
-                $this->markTestSkipped('Network timeout: ' . $e->getMessage());
-            }
-            throw $e;
-        }
-
-        $this->expectException('phpMyFAQ\Core\Exception');
+        $response = $this->createMock(ResponseInterface::class);
+        $response->method('getStatusCode')->willReturn(404);
+
+        $this->httpClientMock
+            ->expects($this->once())
+            ->method('request')
+            ->willReturn($response);
+
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Cannot download package (HTTP Status: 404).');
+
         $this->upgrade->downloadPackage('1.2.3');
     }
 
@@ -63,7 +88,7 @@ public function testCheckFilesystemValid(): void
      */
     public function testCheckFilesystemMissingConfigFiles(): void
     {
-        $this->expectException('phpMyFAQ\Core\Exception');
+        $this->expectException('phpMyFAQ\\Core\\Exception');
         $this->expectExceptionMessage(
             'The files /content/core/config/constant.php and /content/core/config/database.php are missing.'
         );
@@ -98,13 +123,4 @@ public function testGetPathForNonNightly(): void
 
         $this->assertEquals('', $this->upgrade->getPath());
     }
-
-    private function isNetworkAvailable(): bool
-    {
-        $context = stream_context_create([
-            'http' => ['timeout' => 5]
-        ]);
-
-        return @file_get_contents('https://download.phpmyfaq.de', false, $context) !== false;
-    }
 }