Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions BigQuery/tests/Unit/BigQueryClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Psr7\Request;
use GuzzleHttp\Psr7\Response;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Exception\ServerException;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\PhpUnit\ProphecyTrait;
Expand Down Expand Up @@ -1033,7 +1033,7 @@ public function testRetryLogging()
$doneJobResponse['status']['state'] = 'DONE';

$apiMockHandler = new MockHandler([
new RequestException(
new ServerException(
'Transient error',
new Request('POST', ''),
new Response(502)
Expand Down
11 changes: 9 additions & 2 deletions BigQuery/tests/Unit/Connection/RestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,14 @@ function ($args) use (&$actualRequest, $response) {

private function getMetadata(Request $request)
{
$lines = explode(PHP_EOL, (string) $request->getBody());
return json_decode($lines[5], true);
// guzzle/psr7 2 generates per-part Content-Length headers, psr7 3
// does not; strip them so the metadata line offset matches on both.
$lines = array_values(array_filter(
explode(PHP_EOL, (string) $request->getBody()),
function ($line) {
return strpos($line, 'Content-Length:') !== 0;
}
Comment on lines +217 to +219

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
function ($line) {
return strpos($line, 'Content-Length:') !== 0;
}
fn ($line) => strpos($line, 'Content-Length:') !== 0;

));
return json_decode($lines[4], true);
}
}
8 changes: 4 additions & 4 deletions Core/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
"require": {
"php": "^8.1",
"rize/uri-template": "~0.3||~0.4",
"google/auth": "^1.34",
"guzzlehttp/guzzle": "^6.5.8||^7.4.4",
"guzzlehttp/promises": "^1.4||^2.0",
"guzzlehttp/psr7": "^2.6",
"google/auth": "^1.53",
"guzzlehttp/guzzle": "^7.8.2||^8.0",
"guzzlehttp/promises": "^2.0.3||^3.0",
"guzzlehttp/psr7": "^2.6.3||^3.0",
"monolog/monolog": "^2.9||^3.0",
"psr/http-message": "^1.0||^2.0",
"google/gax": "^1.38.0"
Expand Down
4 changes: 3 additions & 1 deletion Core/src/RequestWrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,9 @@ private function convertToGoogleException(\Exception $ex)
*/
private function getExceptionMessage(\Exception $ex)
{
if ($ex instanceof RequestException && $ex->hasResponse()) {
// Guzzle 7 carries the response on RequestException, Guzzle 8 only on
// its ResponseException subclass, hence the method_exists() check.
if ($ex instanceof RequestException && method_exists($ex, 'getResponse') && $ex->getResponse()) {
return (string) $ex->getResponse()->getBody();
}

Expand Down
9 changes: 7 additions & 2 deletions Core/src/RetryDeciderTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,13 @@ private function getRetryFunction($shouldRetryMessages = true)
return false;
}

$message = ($ex instanceof RequestException && $ex->hasResponse())
? (string) $ex->getResponse()->getBody()
// Guzzle 7 carries the response on RequestException, Guzzle 8 only
// on its ResponseException subclass, hence the method_exists() check.
$response = $ex instanceof RequestException && method_exists($ex, 'getResponse')
? $ex->getResponse()
: null;
$message = $response
? (string) $response->getBody()
: $ex->getMessage();

try {
Expand Down
4 changes: 2 additions & 2 deletions Core/tests/Unit/RequestWrapperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
use Google\Cloud\Core\Exception\ServerException;
use Google\Cloud\Core\Exception\ServiceException;
use Google\Cloud\Core\RequestWrapper;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Exception\ClientException;
use GuzzleHttp\Promise\Create;
use GuzzleHttp\Promise\PromiseInterface;
use GuzzleHttp\Psr7\Request;
Expand Down Expand Up @@ -335,7 +335,7 @@ public function testExceptionMessageIsNotTruncatedWithGuzzle()
$msg = str_repeat('0', 121);
$jsonMsg = '{"msg":"' . $msg . '"}';

throw new RequestException(
throw new ClientException(
$jsonMsg,
$request,
new Response(400, [], $jsonMsg)
Expand Down
6 changes: 3 additions & 3 deletions Gax/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
"license": "BSD-3-Clause",
"require": {
"php": "^8.1",
"google/auth": "^1.52",
"google/auth": "^1.53",
"google/grpc-gcp": "^0.4",
"grpc/grpc": "^1.13",
"google/protobuf": "^4.31||^5.34",
"guzzlehttp/promises": "^2.0",
"guzzlehttp/psr7": "^2.0",
"guzzlehttp/promises": "^2.0.3||^3.0",
"guzzlehttp/psr7": "^2.6.3||^3.0",
"google/common-protos": "^4.9",
"google/longrunning": "~0.4",
"ramsey/uuid": "^4.0"
Expand Down
4 changes: 3 additions & 1 deletion Gax/src/ApiException.php
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,9 @@ public static function createFromRpcStatus(Status $status)
*/
public static function createFromRequestException(RequestException $ex, bool $isStream = false)
{
$res = $ex->getResponse();
// Guzzle 7 carries the response on RequestException, Guzzle 8 only on
// its ResponseException subclass, hence the method_exists() check.
$res = method_exists($ex, 'getResponse') ? $ex->getResponse() : null;
Comment thread
GrahamCampbell marked this conversation as resolved.
$body = (string) $res->getBody();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance, we would get a PHP fatal error ("Call to a member function getBody() on null") if the above exception does not have method getResponse. However, the current behavior is the same - if getResponse() returns null, the same PHP fatal error is thrown.

If I am understanding this correctly, EVERY instance in Guzzle 7 which returned a response for RequestException::getResponse() now returns a ResponseException in Guzzle 8, is that accurate? If so this should be safe, and any changes to this behavior would be a separate bug fix.

Even so it might be a good idea to protect from fatal errors like this, and to add a comment in the method description that the method expects either a RequestException with a response in Guzzle 7, or a ResponseException in Guzzle 8.

Suggested change
$body = (string) $res->getBody();
$body = (string) $res?->getBody();

$decoded = json_decode($body, true);

Expand Down
4 changes: 3 additions & 1 deletion Gax/src/Transport/GrpcFallbackTransport.php
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ private function getCallOptions(array $options)
*/
private function transformException(\Exception $ex)
{
if ($ex instanceof RequestException && $ex->hasResponse()) {
// Guzzle 7 carries the response on RequestException, Guzzle 8 only on
// its ResponseException subclass, hence the method_exists() check.
if ($ex instanceof RequestException && method_exists($ex, 'getResponse') && $ex->getResponse()) {
$res = $ex->getResponse();
$body = (string) $res->getBody();
$status = new Status();
Expand Down
4 changes: 3 additions & 1 deletion Gax/src/Transport/Rest/RestServerStreamingCall.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ public function start($request, array $headers = [], array $callOptions = [])
$callOptions
)->wait();
} catch (\Exception $ex) {
if ($ex instanceof RequestException && $ex->hasResponse()) {
// Guzzle 7 carries the response on RequestException, Guzzle 8 only
// on its ResponseException subclass, hence the method_exists() check.
if ($ex instanceof RequestException && method_exists($ex, 'getResponse') && $ex->getResponse()) {
$ex = ApiException::createFromRequestException($ex, /* isStream */ true);
}
throw $ex;
Expand Down
5 changes: 4 additions & 1 deletion Gax/src/Transport/RestTransport.php
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ function (ResponseInterface $response) use ($call, $options) {
return $return;
},
function (\Throwable $ex) {
if ($ex instanceof RequestException && $ex->hasResponse()) {
// Guzzle 7 carries the response on RequestException, Guzzle 8
// only on its ResponseException subclass, hence the
// method_exists() check.
if ($ex instanceof RequestException && method_exists($ex, 'getResponse') && $ex->getResponse()) {
throw ApiException::createFromRequestException($ex);
}

Expand Down
5 changes: 4 additions & 1 deletion Storage/src/Connection/Rest.php
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,11 @@ public function downloadObject(array $args = [])
&$attempt,
) {
// if the exception has a response for us to use
// (Guzzle 7 carries it on RequestException, Guzzle 8 only on its
// ResponseException subclass, hence the method_exists() check)
if ($e instanceof RequestException
&& $e->hasResponse()
&& method_exists($e, 'getResponse')
&& $e->getResponse()
&& $e->getResponse()->getStatusCode() >= 200
&& $e->getResponse()->getStatusCode() < 300
) {
Expand Down
17 changes: 12 additions & 5 deletions Storage/tests/Unit/Connection/RestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
use Google\Cloud\Storage\Connection\Rest;
use Google\Cloud\Storage\Connection\RetryTrait;
use GuzzleHttp\Client;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Exception\BadResponseException;
use GuzzleHttp\Promise\Create;
use GuzzleHttp\Promise\PromiseInterface;
use GuzzleHttp\Psr7\Request;
Expand Down Expand Up @@ -511,7 +511,7 @@ function ($args) use (
$actualRequests[$requestIndex] = $args[0];
$requestHeaders[$requestIndex] = $args[1]['headers'] ?? [];
if ($requestIndex++ === 0) {
throw new RequestException("Server error", $args[0], new Response($status1, [], $body1));
throw new BadResponseException("Server error", $args[0], new Response($status1, [], $body1));
}
return new Response($status2, [], $body2);
}
Expand Down Expand Up @@ -1041,10 +1041,17 @@ private function getContentTypeAndMetadata(RequestInterface $request)
}

// Multipart upload request
$lines = explode(PHP_EOL, (string) $request->getBody());
// (guzzle/psr7 2 generates per-part Content-Length headers, psr7 3
// does not; strip them so the line offsets match on both)
$lines = array_values(array_filter(
explode(PHP_EOL, (string) $request->getBody()),
function ($line) {
return strpos($line, 'Content-Length:') !== 0;
}
));
return [
trim(explode(':', $lines[7])[1]),
json_decode($lines[5], true)
trim(explode(':', $lines[6])[1]),
json_decode($lines[4], true)
];
}
}
Expand Down
6 changes: 3 additions & 3 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@
"require": {
"php": "^8.1",
"rize/uri-template": "~0.3",
"guzzlehttp/guzzle": "^7.4.5",
"guzzlehttp/psr7": "^2.6",
"guzzlehttp/promises": "^2.0",
"guzzlehttp/guzzle": "^7.8.2||^8.0",
"guzzlehttp/psr7": "^2.6.3||^3.0",
"guzzlehttp/promises": "^2.0.3||^3.0",
"monolog/monolog": "^2.9||^3.0",
"psr/http-message": "^1.0|^2.0",
"grpc/grpc": "^1.13",
Expand Down
5 changes: 4 additions & 1 deletion dev/src/GitHub.php
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,10 @@ public function getChangelog($target, $tagName): string|null|false
return json_decode((string) $res->getBody(), true)['body'];
} catch (RequestException $e) {
$this->logException($e);
return $e->getResponse()?->getStatusCode() === 404 ? false : null;
// Guzzle 7 carries the response on RequestException, Guzzle 8 only
// on its ResponseException subclass, hence the method_exists() check.
$response = method_exists($e, 'getResponse') ? $e->getResponse() : null;
return $response?->getStatusCode() === 404 ? false : null;
}
}

Expand Down
10 changes: 6 additions & 4 deletions dev/tests/Unit/Command/ReleaseInfoCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Google\Cloud\Dev\Command\ReleaseInfoCommand;
use Google\Cloud\Dev\Composer;
use GuzzleHttp\Client;
use GuzzleHttp\Exception\BadResponseException;
use GuzzleHttp\Exception\RequestException;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ResponseInterface;
Expand Down Expand Up @@ -146,7 +147,9 @@ public function testInvalidTagThrowsException()
$response->getStatusCode()
->shouldBeCalledOnce()
->willReturn(404);
$exception = $this->prophesize(RequestException::class);
// On Guzzle 8 the response lives on the BadResponseException subclass,
// not on the base RequestException.
$exception = $this->prophesize(BadResponseException::class);
$exception->getResponse()
->shouldBeCalledOnce()
->willReturn($response->reveal());
Expand All @@ -168,10 +171,9 @@ public function testBadRequestThrowsException()
$this->expectExceptionMessage('Unable to retrieve tag');

$tag = 'v0.1000.0';
// A base RequestException carries no response on Guzzle 8, so the
// changelog lookup cannot determine the status and returns null.
$exception = $this->prophesize(RequestException::class);
$exception->getResponse()
->shouldBeCalledOnce()
->willReturn(null);
$http = $this->prophesize(Client::class);
$http->get(
'https://api.github.com/repos/googleapis/google-cloud-php/releases/tags/' . $tag,
Expand Down
Loading