Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Add ability to customize or disable Http\Client\RequestException message truncation #53734

Merged
merged 5 commits into from
Dec 6, 2024
Merged

[11.x] Add ability to customize or disable Http\Client\RequestException message truncation #53734

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bbd7dc
Add ability to disable or customize exception message truncation
stevebauman Dec 2, 2024
5317e05
Add tests for truncation
stevebauman Dec 2, 2024
db9b318
Spacing
stevebauman Dec 2, 2024
20b4122
Fix spacing
stevebauman Dec 2, 2024
2f4a756
formatting
taylorotwell Dec 6, 2024
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
42 changes: 41 additions & 1 deletion src/Illuminate/Http/Client/RequestException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ class RequestException extends HttpClientException
*/
public $response;

/**
* The truncation length for the exception message.
*
* @var int|false
*/
public static $truncateAt = 120;

/**
* Create a new exception instance.
*
Expand All @@ -26,6 +33,37 @@ public function __construct(Response $response)
$this->response = $response;
}

/**
* Enable truncation of the exception message.
*
* @return void
*/
public static function truncate()
{
static::$truncateAt = 120;
}
Comment on lines +41 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that a little redundant as you already set the property to this value by default? Or is that for resetting purposes? If it is needed, I personally don't like magic numbers being set on properties with no indication why they are the default

Copy link
Contributor Author

@stevebauman stevebauman Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yea this is to reset the value back to the default.


/**
* Disable truncation of the exception message.
*
* @return void
*/
public static function dontTruncate()
Copy link
Contributor

Choose a reason for hiding this comment

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

just a suggestion withoutTruncate

{
static::$truncateAt = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static::$truncateAt = false;
static::$truncateAt = 0;

force as int instead of int|false

Copy link
Contributor

Choose a reason for hiding this comment

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

This is counterintuitive as this implies truncating the message entirely instead of not truncating at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about -1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly better, yet I prefer to use a distinct type to indicate a behavioral flag.

}

/**
* Set the truncation length for the exception message.
*
* @param int $length
* @return void
*/
public static function truncateAt($length)
{
static::$truncateAt = $length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional, that truncateAt() can take anything as a parameter because it is only type-hinted in the PHPDoc? If that is true, wouldn't it make sense, to call truncateAt() from truncate() and dontTruncate() so that, if the variable name changes, you only have to change it in one other place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case with various setters in Laravel and first party packages. I also didn't want to apply a parameter or property type as the existing property in the class doesn't have a type. Happy to change it if the team wants it changed 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

the existing property in the class doesn't have a type.

Didn't you add $truncateAt yourself? From the changed files, it looks like it. Or did you mean the other properties of the class?

This is the case with various setters in Laravel and first party packages.

Afaik, this is mostly for historical reasons coming from a time when PHP didn't have (much) native typing. But the codebase is improved towards typing, so I'd say, newly introduced functionality tends to have types when possible.

Copy link
Contributor Author

@stevebauman stevebauman Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Didn't you add $truncateAt yourself? From the changed files, it looks like it. Or did you mean the other properties of the class?

The other properties of the class

Afaik, this is mostly for historical reasons coming from a time when PHP didn't have (much) native typing. But the codebase is improved towards typing, so I'd say, newly introduced functionality tends to have types when possible.

I think this is the case with new classes but not new properties on existing classes that already have properties with undefined types. Otherwise there'd be a mishmash of some properties with and without types. Happy to be corrected by the team though of course 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This mismatch would be in looks only but not in functionality


/**
* Prepare the exception message.
*
Expand All @@ -36,7 +74,9 @@ protected function prepareMessage(Response $response)
{
$message = "HTTP request returned status code {$response->status()}";

$summary = Message::bodySummary($response->toPsrResponse());
$summary = static::$truncateAt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$summary = static::$truncateAt
$summary = static::$truncateAt > 0

Copy link
Contributor

Choose a reason for hiding this comment

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

See #53734 (comment)

? Message::bodySummary($response->toPsrResponse(), static::$truncateAt)
: Message::toString($response->toPsrResponse());

return is_null($summary) ? $message : $message .= ":\n{$summary}\n";
}
Expand Down
38 changes: 38 additions & 0 deletions tests/Http/HttpClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ protected function setUp(): void
parent::setUp();

$this->factory = new Factory;

RequestException::truncate();
}

protected function tearDown(): void
Expand Down Expand Up @@ -1242,6 +1244,42 @@ public function testRequestExceptionTruncatedSummary()
throw new RequestException(new Response($response));
}

public function testRequestExceptionWithoutTruncatedSummary()
{
RequestException::dontTruncate();

$this->expectException(RequestException::class);
$this->expectExceptionMessage('{"error":{"code":403,"message":"The Request can not be completed because quota limit was exceeded. Please, check our support team to increase your limit');

$error = [
'error' => [
'code' => 403,
'message' => 'The Request can not be completed because quota limit was exceeded. Please, check our support team to increase your limit',
],
];
$response = new Psr7Response(403, [], json_encode($error));

throw new RequestException(new Response($response));
}

public function testRequestExceptionWithCustomTruncatedSummary()
{
RequestException::truncateAt(60);

$this->expectException(RequestException::class);
$this->expectExceptionMessage('{"error":{"code":403,"message":"The Request can not be compl (truncated...)');

$error = [
'error' => [
'code' => 403,
'message' => 'The Request can not be completed because quota limit was exceeded. Please, check our support team to increase your limit',
],
];
$response = new Psr7Response(403, [], json_encode($error));

throw new RequestException(new Response($response));
}

public function testRequestExceptionEmptyBody()
{
$this->expectException(RequestException::class);
Expand Down