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

[9.x] Include validation errors in ValidationException::$message #35524

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Dec 7, 2020

This is a re-submit of #35496 against master rather than the 8.x branch.

The default exception message for validation exceptions is "The given data was invalid." which is not particularly descriptive. There are lots of times when I'm running tests with exception handling turned off and I find I need to either dd() the validation messages or turn on exception handling and add a call to assertSessionHasNoErrors() to find the issue.

This PR adds a summarize() static method to the validation exception that includes the first error message, and if there are more than one, a count of the additional errors. That way, instead of "The given data was invalid." the message will look something like:

"The first name field is required (and 2 more errors)"

@GrahamCampbell
Copy link
Member

The summarize method could still be added to 8.x IMO, just modified to return the same thing it always used to.

@inxilpro
Copy link
Contributor Author

inxilpro commented Dec 7, 2020

The summarize method could still be added to 8.x IMO

I'm not really sure what the benefit of adding that in 8.x would be, though. The value really comes from having a more descriptive error message by default… I think it's fine to just wait for 9.x.

@GrahamCampbell
Copy link
Member

The benefit would be the ability to customize the message, as is possible in this PR.

@inxilpro
Copy link
Contributor Author

inxilpro commented Dec 7, 2020

Would you do something like pass it a callback? I don't see how having a summarize() method on the existing exception does much without having to subclass a bunch of things, in which case you can just add your own logic to the constructor…

To be clear, the summarize() method is just there to set the string that is passed up to the parent constructor. It's not meant to be public-facing.

@taylorotwell taylorotwell merged commit 6bc063e into laravel:master Dec 7, 2020
@miken32
Copy link
Contributor

miken32 commented Feb 10, 2022

I like this change, but I feel it should have been mentioned in the upgrade guide for 9.0. All my feature tests started failing because they were checking for the old exception message. Pretty low impact, but there are smaller changes than this in the upgrade guide.

@driesvints
Copy link
Member

@miken32 can you PR that to the Laravel 9 upgrade guide?

@driesvints
Copy link
Member

Also, in general I suggest to never rely on exception messages because they're suppose to be fluid and not part of any public API. Instead check on the exception class type itself.

@miken32
Copy link
Contributor

miken32 commented Feb 10, 2022

@driesvints I'll give it a shot. I don't use the value for public consumption, just in feature testing where I expect a certain JSON response to my requests. For unit testing, agreed; I just check for a ValidationException being thrown.

@OrkhanAlikhanov
Copy link

This change break our tests where we were expecting message not to be dynamic.

$test = fn($body, $errors) => $this
    ->actingAs($user)
    ->postJson("api/...", $body)
    ->assertStatus(422)
    ->assertExactJson([
        'message' => 'The given data was invalid.',
        'errors' => $errors,
    ]);

Would be nice if PR had included ability to disable this feature.

@driesvints
Copy link
Member

Please do not rely on asserting exception messages as these don't fall under any BC promise.

@ghost
Copy link

ghost commented Mar 17, 2022

Tests aside, is there a way to override this to still return the old/another message?

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.

6 participants