Fix Http::retry so that throw is respected for call signature Http::r…

…etry([1,2], throw: false) (#52002) * Update HttpClientTest.php * Fix PendingRequest.php to deal with tries as an array * Style CI spaces * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Jul 3, 2024 · fc70c5e · fc70c5e
1 parent 500d93a
commit fc70c5e
Show file tree

Hide file tree

Showing 2 changed files with 145 additions and 2 deletions.
diff --git a/src/Illuminate/Http/Client/PendingRequest.php b/src/Illuminate/Http/Client/PendingRequest.php
@@ -911,11 +911,15 @@ public function send(string $method, string $url, array $options = [])
                             $response->throw($this->throwCallback);
                         }
 
-                        if ($attempt < $this->tries && $shouldRetry) {
+                        $potentialTries = is_array($this->tries)
+                            ? count($this->tries) + 1
+                            : $this->tries;
+
+                        if ($attempt < $potentialTries && $shouldRetry) {
                             $response->throw();
                         }
 
-                        if ($this->tries > 1 && $this->retryThrow) {
+                        if ($potentialTries > 1 && $this->retryThrow) {
                             $response->throw();
                         }
                     }

diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php
@@ -1711,6 +1711,28 @@ public function testRequestExceptionIsThrownWhenRetriesExhausted()
         $this->factory->assertSentCount(2);
     }
 
+    public function testRequestExceptionIsThrownWhenRetriesExhaustedWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->response(['error'], 403),
+        ]);
+
+        $exception = null;
+
+        try {
+            $this->factory
+                ->retry([1], 0, null, true)
+                ->get('http://foo.com/get');
+        } catch (RequestException $e) {
+            $exception = $e;
+        }
+
+        $this->assertNotNull($exception);
+        $this->assertInstanceOf(RequestException::class, $exception);
+
+        $this->factory->assertSentCount(2);
+    }
+
     public function testRequestExceptionIsThrownWithoutRetriesIfRetryNotNecessary()
     {
         $this->factory->fake([
@@ -1740,6 +1762,35 @@ public function testRequestExceptionIsThrownWithoutRetriesIfRetryNotNecessary()
         $this->factory->assertSentCount(1);
     }
 
+    public function testRequestExceptionIsThrownWithoutRetriesIfRetryNotNecessaryWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->response(['error'], 500),
+        ]);
+
+        $exception = null;
+        $whenAttempts = 0;
+
+        try {
+            $this->factory
+                ->retry([1000, 1000], 1000, function ($exception) use (&$whenAttempts) {
+                    $whenAttempts++;
+
+                    return $exception->response->status() === 403;
+                }, true)
+                ->get('http://foo.com/get');
+        } catch (RequestException $e) {
+            $exception = $e;
+        }
+
+        $this->assertNotNull($exception);
+        $this->assertInstanceOf(RequestException::class, $exception);
+
+        $this->assertSame(1, $whenAttempts);
+
+        $this->factory->assertSentCount(1);
+    }
+
     public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted()
     {
         $this->factory->fake([
@@ -1755,6 +1806,21 @@ public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted()
         $this->factory->assertSentCount(2);
     }
 
+    public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhaustedWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->response(['error'], 403),
+        ]);
+
+        $response = $this->factory
+            ->retry([1, 2], throw: false)
+            ->get('http://foo.com/get');
+
+        $this->assertTrue($response->failed());
+
+        $this->factory->assertSentCount(3);
+    }
+
     public function testRequestExceptionIsNotThrownWithoutRetriesIfRetryNotNecessary()
     {
         $this->factory->fake([
@@ -1778,6 +1844,29 @@ public function testRequestExceptionIsNotThrownWithoutRetriesIfRetryNotNecessary
         $this->factory->assertSentCount(1);
     }
 
+    public function testRequestExceptionIsNotThrownWithoutRetriesIfRetryNotNecessaryWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->response(['error'], 500),
+        ]);
+
+        $whenAttempts = 0;
+
+        $response = $this->factory
+            ->retry([1, 2], 0, function ($exception) use (&$whenAttempts) {
+                $whenAttempts++;
+
+                return $exception->response->status() === 403;
+            }, false)
+            ->get('http://foo.com/get');
+
+        $this->assertTrue($response->failed());
+
+        $this->assertSame(1, $whenAttempts);
+
+        $this->factory->assertSentCount(1);
+    }
+
     public function testRequestCanBeModifiedInRetryCallback()
     {
         $this->factory->fake([
@@ -1803,6 +1892,31 @@ public function testRequestCanBeModifiedInRetryCallback()
         });
     }
 
+    public function testRequestCanBeModifiedInRetryCallbackWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->sequence()
+                ->push(['error'], 500)
+                ->push(['ok'], 200),
+        ]);
+
+        $response = $this->factory
+            ->retry([2], when: function ($exception, $request) {
+                $this->assertInstanceOf(PendingRequest::class, $request);
+
+                $request->withHeaders(['Foo' => 'Bar']);
+
+                return true;
+            }, throw: false)
+            ->get('http://foo.com/get');
+
+        $this->assertTrue($response->successful());
+
+        $this->factory->assertSent(function (Request $request) {
+            return $request->hasHeader('Foo') && $request->header('Foo') === ['Bar'];
+        });
+    }
+
     public function testExceptionThrownInRetryCallbackWithoutRetrying()
     {
         $this->factory->fake([
@@ -1828,6 +1942,31 @@ public function testExceptionThrownInRetryCallbackWithoutRetrying()
         $this->factory->assertSentCount(1);
     }
 
+    public function testExceptionThrownInRetryCallbackWithoutRetryingWithBackoffArray()
+    {
+        $this->factory->fake([
+            '*' => $this->factory->response(['error'], 500),
+        ]);
+
+        $exception = null;
+
+        try {
+            $this->factory
+                ->retry([1, 2, 3], when: function ($exception) use (&$whenAttempts) {
+                    throw new Exception('Foo bar');
+                }, throw: false)
+                ->get('http://foo.com/get');
+        } catch (Exception $e) {
+            $exception = $e;
+        }
+
+        $this->assertNotNull($exception);
+        $this->assertInstanceOf(Exception::class, $exception);
+        $this->assertEquals('Foo bar', $exception->getMessage());
+
+        $this->factory->assertSentCount(1);
+    }
+
     public function testRequestsWillBeWaitingSleepMillisecondsReceivedBeforeRetry()
     {
         Sleep::fake();