[5.5] Updates the Exception Handler in order to allow custom AuthenticationException message #21575
Conversation
…Exception message. Example use case: the user account gets disabled while the user is logged in the app and we want to logout the user and throw an exception with an appropriate message. Before the PR, in the handler the custom message is overwritten before sending the response.
| @@ -216,7 +216,7 @@ protected function prepareException(Exception $e) | |||
| protected function unauthenticated($request, AuthenticationException $exception) | |||
| { | |||
| return $request->expectsJson() | |||
| ? response()->json(['message' => 'Unauthenticated.'], 401) | |||
| ? response()->json(['message' => $exception->getMessage()], 401) | |||
There was a problem hiding this comment.
Probably should be $exception->getMessage() ?: 'Unauthenticated.'?
There was a problem hiding this comment.
I think it's not necessary because the constructor for AuthenticationException already has a default value for the $message property
namespace Illuminate\Auth;
use Exception;
class AuthenticationException extends Exception
...
public function __construct($message = 'Unauthenticated.', array $guards = [])
{
parent::__construct($message);
$this->guards = $guards;
}
...There was a problem hiding this comment.
I think it's not necessary because the constructor for AuthenticationException already has a default value for the $message property
And what if someone actuallty constructs it with somethinh empty?
There was a problem hiding this comment.
Something empty like:
throw new AuthenticationException('') ?
If this is the case I would expect that this person does not want any message at all.
Or you were thinking of something else?
There was a problem hiding this comment.
I'm with @aocneanu on this.
There was a problem hiding this comment.
Also, while inspecting a little more I found that this exception is used in 2 middlewares and one trait:
-
Authenticatemiddleware
Here the exception is thrown using "Unauthenticated." message. -
AuthenticateSessionmiddleware
thrown without a message. -
GuardHelperstrait
thrown without a message.
If we want to be consistent we could either
a) hardcode the "Unauthenticated." message in AuthenticateSession middleware and in GuardHelpers trait, and maybe remove the default message from the AuthenticationException constructor. In this case, in Handler we could put the an elvis operator if we consider that the user never wants to throw this exception without a message, or a ternary and cover both situations
b) remove the "Unauthenticated." message from the Authenticate middleware and keep the default value in the AuthenticationException constructor.
If you find any of this suitable let me know and I will commit again.
GrahamCampbell
changed the title
|
@taylorotwell can you please tell me what is wrong with the pull request? |
|
What’s next? Am I supposed to do something? If so please let me know. |
Before the PR, in the handler the custom message is overwritten before returning the response.
Example use case: the user account gets disabled while the user is logged in the app and we want to logout the user and throw an exception with an appropriate message.