[12.x] RequestException: attempt to summarize message before reporting

[cosmastech]

For applications that were logging the RequestException's error message (but not calling report()) or who were using $exception->getMessage() in some other way:

• str_contains($requestException->getMessage(), 'not found')
• throw new ShippingException($requestException->getMessage(), previous: $requestException)

For example, if an application had set RequestException::dontTruncate() inside of their AppServiceProvider instead of through the callback provided to ApplicationBuilder@exceptions(), they would have previously had the body of the response in hand. But now, a separate method needs to be called in order to build the message.

Additionally, it's possible a RequestException could be reported twice, and in so doing would build the summary twice.

---

The benefit to just reverting the change is that we don't need users to update their configuration to use the ApplicationBuilder@registered() callback instead of how it's been documented. (The Exceptions@dontTruncateRequestExceptions() and Exceptions@truncateRequestExceptionsAt() would remain in the code, tho do not function as expected). If the users DO switch to using the new callback described in this docs PR, they'll actually get the full body of the payload even without having to call RequestException@report()

The downside here is that when exceptions are reported, they'll generate the summary twice (once upon construction, once upon reporting).

[crynobone]

This will make #57601 and issue again. The handler isn't always resolved on the web request when the exception triggers.

[cosmastech]

This will make #57601 and issue again. The handler isn't always resolved on the web request when the exception triggers.

Yeah. I think what we have to do is attempt to summarize it twice, once upon construction and a second time when report() is called.

I didn't dig too deep, is there a reason we can just resolve the exception handler sooner?

[crynobone]

I didn't dig too deep, is there a reason we can just resolve the exception handler sooner?

You don't need to have it resolved for every request. As for local/CI environment nunomaduro/collision would prepare the exception handler so you don't see it during tests.

What about adding summarizeResponse(): ?string if you need to assert against the response itself.

[cosmastech]

I didn't dig too deep, is there a reason we can just resolve the exception handler sooner?

You don't need to have it resolved for every request. As for local/CI environment nunomaduro/collision would prepare the exception handler so you don't see it during tests.

Sorry, let me rephrase. The problem is that the callback passed to Exceptions@using() doesn't get executed until the ExceptionHandler interface is resolved. Couldn't we just resolve the ExceptionHandler during app boot and revert the changes in the previous MR?

What about adding summarizeResponse(): ?string if you need to assert against the response itself.

I think that method could make sense, however, what I'm trying to avoid is a breaking change in things like log monitors in observability tools. Such as in a codebase that doesn't leverage report(), but just grabs the thrown exception's message and logs it. See: #57759 (comment)

edit: I am not proposing that forwarding the exception's message is necessarily the best engineering practice, however, legacy codebases don't always have the luxury of mass refactors to fix those sort of issues in one go. 😅

[taylorotwell]

I'm honestly so lost on this entire issue. What do I need to do / merge?

[cosmastech]

I'm honestly so lost on this entire issue. What do I need to do / merge?

The message summarizing aspect previously did not work if you were using the ApplicationBuilder::withExceptions() to indicate you don't want RequestExceptions truncated, because the ExceptionHandler hadn't been booted. If you had done it the other way, like inside of your AppServiceProvider, it would have worked as expected.

In either case, at least a partial summary of the response would be included in the RequestException's message. With the change that went out in the last release, no summary is added to the message UNTIL you call report() (either directly or by the ExceptionHandler doing it's magic).

I personally feel this is probably a better approach than just reverting the change, since we now:

• have the behavior we had prior
• if people are still using the withExceptions() route, if it gets passed to the ExceptionHandler, the full summary will show up (where it would have not previously)