Requests: Handle requests.failed hook correctly in case of redirects

src/Exception.php

@@ -29,6 +29,13 @@ class Exception extends PHPException {
 	 */
 	protected $data;
 
+	/**
+	 * Whether the exception was already passed to the requests.failed hook or not
+	 *
+	 * @var boolean
+	 */
+	public $failed_hook_handled = FALSE;
+
 	/**
 	 * Create a new exception
 	 *

src/Exception/InvalidArgument.php

@@ -12,6 +12,13 @@
  */
 final class InvalidArgument extends InvalidArgumentException {
 
+	/**
+	 * Whether the exception was already passed to the requests.failed hook or not
+	 *
+	 * @var boolean
+	 */
+	public $failed_hook_handled = FALSE;
+
 	/**
 	 * Create a new invalid argument exception with a standardized text.
 	 *

src/Requests.php

@@ -474,7 +474,10 @@ public static function request($url, $headers = [], $data = [], $type = self::GE
 
 			$parsed_response = self::parse_response($response, $url, $headers, $data, $options);
 		} catch (Exception|InvalidArgument $e) {
-			$options['hooks']->dispatch('requests.failed', [&$e, $url, $headers, $data, $type, $options]);
+			if ($e->failed_hook_handled === FALSE) {
+				$options['hooks']->dispatch('requests.failed', [&$e, $url, $headers, $data, $type, $options]);
+				$e->failed_hook_handled = TRUE;
+			}
 			throw $e;
 		}
 

tests/Fixtures/TransportRedirectMock.php

@@ -0,0 +1,113 @@
+<?php
+
+namespace WpOrg\Requests\Tests\Fixtures;
+
+use WpOrg\Requests\Transport;
+
+final class TransportRedirectMock implements Transport {
+	public $code        = 302;
+	public $chunked     = false;
+	public $body        = '';
+	public $raw_headers = '';
+
+	private $redirected = [];
+
+	public $redirected_transport = NULL;
+
+	private static $messages = [
+		100 => '100 Continue',
+		101 => '101 Switching Protocols',
+		200 => '200 OK',
+		201 => '201 Created',
+		202 => '202 Accepted',
+		203 => '203 Non-Authoritative Information',
+		204 => '204 No Content',
+		205 => '205 Reset Content',
+		206 => '206 Partial Content',
+		300 => '300 Multiple Choices',
+		301 => '301 Moved Permanently',
+		302 => '302 Found',
+		303 => '303 See Other',
+		304 => '304 Not Modified',
+		305 => '305 Use Proxy',
+		306 => '306 (Unused)',
+		307 => '307 Temporary Redirect',
+		400 => '400 Bad Request',
+		401 => '401 Unauthorized',
+		402 => '402 Payment Required',
+		403 => '403 Forbidden',
+		404 => '404 Not Found',
+		405 => '405 Method Not Allowed',
+		406 => '406 Not Acceptable',
+		407 => '407 Proxy Authentication Required',
+		408 => '408 Request Timeout',
+		409 => '409 Conflict',
+		410 => '410 Gone',
+		411 => '411 Length Required',
+		412 => '412 Precondition Failed',
+		413 => '413 Request Entity Too Large',
+		414 => '414 Request-URI Too Long',
+		415 => '415 Unsupported Media Type',
+		416 => '416 Requested Range Not Satisfiable',
+		417 => '417 Expectation Failed',
+		418 => '418 I\'m a teapot',
+		428 => '428 Precondition Required',
+		429 => '429 Too Many Requests',
+		431 => '431 Request Header Fields Too Large',
+		500 => '500 Internal Server Error',
+		501 => '501 Not Implemented',
+		502 => '502 Bad Gateway',
+		503 => '503 Service Unavailable',
+		504 => '504 Gateway Timeout',
+		505 => '505 HTTP Version Not Supported',
+		511 => '511 Network Authentication Required',
+	];
+
+	public function request($url, $headers = [], $data = [], $options = []) {
+		if (array_key_exists($url, $this->redirected))
+		{
+			return $this->redirected_transport->request($url, $headers, $data, $options);
+		}
+
+		$redirect_url = "https://example.com/redirected?url=" . urlencode($url);
+
+		$status    = isset(self::$messages[$this->code]) ? self::$messages[$this->code] : $this->code . ' unknown';
+		$response  = "HTTP/1.0 $status\r\n";
+		$response .= "Content-Type: text/plain\r\n";
+		if ($this->chunked) {
+			$response .= "Transfer-Encoding: chunked\r\n";
+		}
+		$response .= "Location: $redirect_url\r\n";
+		$response .= $this->raw_headers;
+		$response .= "Connection: close\r\n\r\n";
+		$response .= $this->body;
+
+		$this->redirected[$url] = TRUE;
+		$this->redirected[$redirect_url] = TRUE;
+
+		return $response;
+	}
+
+	public function request_multiple($requests, $options) {
+		$responses = [];
+		foreach ($requests as $id => $request) {
+			$handler              = new self();
+			$handler->code        = $request['options']['mock.code'];
+			$handler->chunked     = $request['options']['mock.chunked'];
+			$handler->body        = $request['options']['mock.body'];
+			$handler->raw_headers = $request['options']['mock.raw_headers'];
+			$responses[$id]       = $handler->request($request['url'], $request['headers'], $request['data'], $request['options']);
+
+			if (!empty($options['mock.parse'])) {
+				$request['options']['hooks']->dispatch('transport.internal.parse_response', [&$responses[$id], $request]);
+				$request['options']['hooks']->dispatch('multiple.request.complete', [&$responses[$id], $id]);
+			}
+		}
+
+		return $responses;
+	}
+
+	public static function test($capabilities = []) {
+		return true;
+	}
+}

tests/RequestsTest.php

@@ -20,6 +20,7 @@
 use WpOrg\Requests\Tests\Fixtures\TransportFailedMock;
 use WpOrg\Requests\Tests\Fixtures\TransportInvalidArgumentMock;
 use WpOrg\Requests\Tests\Fixtures\TransportMock;
+use WpOrg\Requests\Tests\Fixtures\TransportRedirectMock;
 
 final class RequestsTest extends TestCase {
 
@@ -177,7 +178,7 @@ public function testDefaultTransport() {
 
 	public function testTransportFailedTriggersRequestsFailedCallback() {
 		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
-		$mock->expects($this->atLeastOnce())->method('failed');
+		$mock->expects($this->once())->method('failed');
 		$hooks = new Hooks();
 		$hooks->register('requests.failed', [$mock, 'failed']);
 
@@ -195,7 +196,7 @@ public function testTransportFailedTriggersRequestsFailedCallback() {
 
 	public function testTransportInvalidArgumentTriggersRequestsFailedCallback() {
 		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
-		$mock->expects($this->atLeastOnce())->method('failed');
+		$mock->expects($this->once())->method('failed');
 		$hooks = new Hooks();
 		$hooks->register('requests.failed', [$mock, 'failed']);
 
@@ -319,7 +320,7 @@ public function testInvalidProtocolVersion() {
 	 */
 	public function testInvalidProtocolVersionTriggersRequestsFailedCallback() {
 		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
-		$mock->expects($this->atLeastOnce())->method('failed');
+		$mock->expects($this->once())->method('failed');
 		$hooks = new Hooks();
 		$hooks->register('requests.failed', [$mock, 'failed']);
 
@@ -357,7 +358,7 @@ public function testSingleCRLFSeparator() {
 	 */
 	public function testSingleCRLFSeparatorTriggersRequestsFailedCallback() {
 		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
-		$mock->expects($this->atLeastOnce())->method('failed');
+		$mock->expects($this->once())->method('failed');
 		$hooks = new Hooks();
 		$hooks->register('requests.failed', [$mock, 'failed']);
 
@@ -389,7 +390,7 @@ public function testInvalidStatus() {
 
 	public function testInvalidStatusTriggersRequestsFailedCallback() {
 		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
-		$mock->expects($this->atLeastOnce())->method('failed');
+		$mock->expects($this->once())->method('failed');
 		$hooks = new Hooks();
 		$hooks->register('requests.failed', [$mock, 'failed']);
 
@@ -418,6 +419,52 @@ public function test30xWithoutLocation() {
 		$this->assertSame(0, $response->redirects);
 	}
 
+	public function testRedirectToExceptionTriggersRequestsFailedCallbackOnce() {
+		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
+		$mock->expects($this->once())->method('failed');
+		$hooks = new Hooks();
+		$hooks->register('requests.failed', [$mock, 'failed']);
+
+		$transport                       = new TransportRedirectMock();
+		$transport->redirected_transport = new TransportFailedMock();
+
+		$options  = [
+			'hooks'     => $hooks,
+			'transport' => $transport,
+		];
+
+		$this->expectException(Exception::class);
+		$this->expectExceptionMessage('Transport failed!');
+
+		$response = Requests::get('http://example.com/', [], $options);
+
+		$this->assertSame(302, $response->status_code);
+		$this->assertSame(1, $response->redirects);
+	}
+
+	public function testRedirectToInvalidArgumentTriggersRequestsFailedCallbackOnce() {
+		$mock = $this->getMockBuilder(stdClass::class)->setMethods(['failed'])->getMock();
+		$mock->expects($this->once())->method('failed');
+		$hooks = new Hooks();
+		$hooks->register('requests.failed', [$mock, 'failed']);
+
+		$transport                       = new TransportRedirectMock();
+		$transport->redirected_transport = new TransportInvalidArgumentMock();
+
+		$options  = [
+			'hooks'     => $hooks,
+			'transport' => $transport,
+		];
+
+		$this->expectException(InvalidArgument::class);
+		$this->expectExceptionMessage('Argument #1 ($url) must be of type string|Stringable');
+
+		$response = Requests::get('http://example.com/', [], $options);
+
+		$this->assertSame(302, $response->status_code);
+		$this->assertSame(1, $response->redirects);
+	}
+
 	public function testTimeoutException() {
 		$options = ['timeout' => 0.5];
 		$this->expectException(Exception::class);