From c99d3fc82692cd303a21375d79d0e185572473af Mon Sep 17 00:00:00 2001 From: Heinz Wiesinger Date: Mon, 14 Mar 2022 14:50:56 +0100 Subject: [PATCH] Requests: Handle requests.failed hook correctly in case of redirects --- src/Exception.php | 7 ++ src/Exception/InvalidArgument.php | 7 ++ src/Requests.php | 5 +- tests/Fixtures/TransportRedirectMock.php | 113 +++++++++++++++++++++++ tests/RequestsTest.php | 57 +++++++++++- 5 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 tests/Fixtures/TransportRedirectMock.php diff --git a/src/Exception.php b/src/Exception.php index 2f1c38150..e7bb7d259 100644 --- a/src/Exception.php +++ b/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 * diff --git a/src/Exception/InvalidArgument.php b/src/Exception/InvalidArgument.php index 0ab7332f4..2536bea77 100644 --- a/src/Exception/InvalidArgument.php +++ b/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. * diff --git a/src/Requests.php b/src/Requests.php index eaf338ca1..6bace677a 100644 --- a/src/Requests.php +++ b/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; } diff --git a/tests/Fixtures/TransportRedirectMock.php b/tests/Fixtures/TransportRedirectMock.php new file mode 100644 index 000000000..6cc689e78 --- /dev/null +++ b/tests/Fixtures/TransportRedirectMock.php @@ -0,0 +1,113 @@ + '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; + } +} diff --git a/tests/RequestsTest.php b/tests/RequestsTest.php index 3de448aa9..78d323b83 100644 --- a/tests/RequestsTest.php +++ b/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);