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.

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

Conversation

stevebauman
Copy link
Contributor

@stevebauman stevebauman commented Dec 2, 2024

Problem

The exception message on Http\Client\RequestException is always truncated, leading to useful debugging information being cropped out. For example:

Illuminate\Http\Client\RequestException
HTTP request returned status code 422:
{"error":{"code":422,"message":"The Request can not be completed because the provided request contains invalid data. Key at 'cus (truncated...)

Unfortunately due to the above truncation, I'm not given enough information in production to debug this. Instead, I have to replicate the HTTP request on the server somehow and try/catch manually (via tinker or some other mechanism) to see the rest. Depending on how complex the HTTP request is and where it was sent in the application, this can be really hard to replicate and debug.

Description

This PR gives developers the option to globally alter the truncation behaviour on Http\Client\RequestException messages.

I initially assumed that this behaviour was built into PHP, but after getting hit with HTTP errors in production and this causing a lot of grief for me in debugging, I dug in and found that luckily this is changeable.

You can see complaints of this truncation causing grief for others here as well: guzzle/guzzle#2185

Usage

Truncation behaviour is controlled with a static variable on the RequestException class.

It can be altered by calling RequestException::dontTrucate() or RequestException::truncateAt($length):

I can move this to the Http\Client\Factory where $preventStrayRequests is if you'd prefer.

// bootstrap/app.php

use Illuminate\Http\Client\RequestException;

return Application::configure(basePath: dirname(__DIR__))
    // ...
    ->withExceptions(function (Exceptions $exceptions) {
        RequestException::dontTruncate();

        // Or ...

        RequestException::truncateAt(260);
    })->create();

If this method of customizing the exception isn't appealing, let me know. I'd love to be able to simply make the customization this exception easier for developers (and me) to save the hair I have left haha.

Let me know your thoughts -- thanks for your time!

Comment on lines +41 to +44
public static function truncate()
{
static::$truncateAt = 120;
}
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

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.

Comment on lines 45 to 65

/**
* Disable truncation of the exception message.
*
* @return void
*/
public static function dontTruncate()
{
static::$truncateAt = false;
}

/**
* 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

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

*/
public static function dontTruncate()
{
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.

@@ -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.

*
* @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

@timacdonald
Copy link
Member

Oh, man, the amount of times this truncation has frustrated me.

I'd like this even if just for debugging locally.

@mateusjunges
Copy link
Contributor

@stevebauman, you are a lifesaver. Thanks so much for this!

@aneeskhan47
Copy link
Contributor

aneeskhan47 commented Dec 4, 2024

@stevebauman

Great lifesaving feature 😄

It would be great if we could customize the truncation globally and in specific places when using the Laravel HTTP client.

For example, I have some API classes and in this class, I don't want the HTTP client to do truncation at all but in some other API classes, i want specific truncate.

@stevebauman
Copy link
Contributor Author

stevebauman commented Dec 4, 2024

@aneeskhan47 That would be nice. For us to do that though we'd have to move the setting to the PendingRequest, where $preventStrayRequests is. I'll wait for the Laravel team to see what they think before moving forward with that 👍

@shaedrich
Copy link
Contributor

It would be great if we could customize the truncation globally

If this was a config value (e.g. app.http.request_exception_message_limit), the default of 120 could be set there and RequestException would just need to read said value

@taylorotwell taylorotwell merged commit 69d3d07 into laravel:11.x Dec 6, 2024
38 checks passed
browner12 pushed a commit to browner12/framework that referenced this pull request Dec 10, 2024
…ion` message truncation (laravel#53734)

* Add ability to disable or customize exception message truncation

* Add tests for truncation

* Spacing

* Fix spacing

* formatting

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
@pboese
Copy link

pboese commented Dec 18, 2024

This is amazing!

Is there a way to do this using the old Laravel 10 bootstrap/app.php? https://laravel.com/docs/11.x/upgrade#application-structure

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants