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] Revert "[9.x] Translation of the default message of the validator" #46378

Merged
merged 5 commits into from
Mar 13, 2023

Conversation

GrahamCampbell
Copy link
Member

Reverts #43837. This was a major mistake. Exceptions should not know about translation - it should be handled by the exception rendering layer. Moreover, the validator contract doesn't have a getTranslator method, so this will result in a crash, in general.

@ankurk91
Copy link
Contributor

ankurk91 commented Mar 7, 2023

Reverting a change after 6+ months 😅

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Mar 7, 2023

It is never late to do the right thing.

I have mixed feelings about this. I fully agree with @GrahamCampbell that localizing an exception message should be done on the exception rendering layer, or any other layer closer to the user, and that calling a method not present in the interface is a mistake.

But in this particular case there should be at least a way to assert the errors count, so one wanting to have a localized message, somehow, can easily assess this information, without defaulting to string parsing, that could break anytime the error message is changed.

Maybe adding the count as a property, and having a getter would be a good middle-ground solution.

@taylorotwell
Copy link
Member

I agree this change was bad but we can't revert it until the next major release.

@taylorotwell taylorotwell deleted the revert-43837-patch/2022-08-24/14-46 branch March 8, 2023 18:46
@GrahamCampbell GrahamCampbell restored the revert-43837-patch/2022-08-24/14-46 branch March 8, 2023 18:55
@GrahamCampbell
Copy link
Member Author

let's re-target at 11.x ;)

@GrahamCampbell GrahamCampbell reopened this Mar 8, 2023
@GrahamCampbell GrahamCampbell changed the base branch from 9.x to master March 8, 2023 18:55
@GrahamCampbell GrahamCampbell changed the title [9.x] Revert "[9.x] Translation of the default message of the validator" [11.x] Revert "[9.x] Translation of the default message of the validator" Mar 8, 2023
@driesvints
Copy link
Member

@GrahamCampbell can you resolve the conflict?

@GrahamCampbell
Copy link
Member Author

The conflict can be resolved only with branch merges 9.x -> 10.x -> master. I'll see if I can do that now.

@GrahamCampbell
Copy link
Member Author

Done.

@andrey-helldar
Copy link
Contributor

I agree that the decision was suboptimal. But if a project provides translations of validator errors, then it should also provide a translation of this message out of the box without manual intervention from developers.

@rodrigopedra
Copy link
Contributor

@andrey-helldar, out of curiosity, when an error, such as 429 Too Many Requests, happens, which a translation package could consider handling, as such error can occur during validation, and is a client error, not a server error, how are you handling its error's message translation?

The default error message on this exception is like a top-level error, and should be handled in a generic way.

Also, you can test for this exception instance with the instanceof operator and provide a proper translation.

The only piece I think is missing is a proper way to retrieve the exception count, without needing to parse the exception, as it is an added feature to the exception instance.

But this can be added in an additional PR.

@lk77
Copy link

lk77 commented Oct 18, 2023

It seems to me you can do that :

 count($exception->validator->errors()->all());

@rodrigopedra
Copy link
Contributor

Or even (I was just looking into the exception's code)

$count = count($messages = $exception->errors());

\dd($messages, $count);

public function errors()
{
return $this->validator->errors()->messages();
}

@andrey-helldar
Copy link
Contributor

andrey-helldar commented Oct 19, 2023

@rodrigopedra, if error 429 is detected, the message is returned by Nginx. So Laravel basically can't handle it. But if this error is returned by the result of calling the HTTP facade, then I simply implement translation when catching errors.

But in those rare cases when Laravel displays a 429 error message, the translation is quite simple:

{{ __('Too Many Requests') }}

Why in rare cases? I just use Laravel as an API in several recent projects. I haven't used Blade templates for several years and I have never used Livewire at all.

@rodrigopedra
Copy link
Contributor

@andrey-helldar , in the same measure, for 422 errors on API requests, the error message is also handled by nginx: 422 Unprocessable Content, isn't it?

When giving the 429 example, I thought you were worried about the JSON payload, or any other text content returned in the response's body.

But if:

by the result of calling the HTTP facade, then I simply implement translation when catching errors.

And you seem, by the response you gave, to ignore the message key on the JSON payload, and prefer to implement translation when "catching errors", then I don't see where the problem is.

For completeness, this is the output, through nginx, for a 429 error, and for a 422 error.

429


422

For 422 errors, you'll get all translated errors on the errors key, as you seem to ignore the message key, based on your 429 example, and prefer to handle error responses by their status code, I guess the current behavior suffices.

@andrey-helldar
Copy link
Contributor

andrey-helldar commented Oct 19, 2023

@rodrigopedra, Not certainly in that way. Code 422 is returned by Laravel when validating incoming data, while code 429 is returned by nginx after several redirects. On your screenshots, you yourself return code 429 using the "abort" function. This has nothing to do with "Too Many Redirects" being caught by the server.

It's actually very easy to reproduce the problem:

// routes/web.php

app('router')->get('some', fn () => redirect('some'));

And in the first screenshot you highlighted the HTTP response header, and not what Laravel returns.

But that’s not what this is all about. The problem is that the validation rules are successfully translated into another language, but the default message is not. And in Laravel 11 it will not be translated again.

For example:

{
    "message": "The given data was invalid.",
    "errors": {
        "email": [
            "Taki adres e-mail już występuje."
        ]
    }
}

{
    "message": "Taki adres e-mail już występuje. (and 1 more error)",
    "errors": {
        "email": [
            "Taki adres e-mail już występuje."
        ],
        "name": [
            "Taki nazwa już występuje."
        ]
    }
}

Cancelled changes: #43837

@rodrigopedra
Copy link
Contributor

Code 429 can also be returned by Laravel, on rate limit configuration.

I tried to keep the example minimal.

Check Illuminate\Routing\Middleware\ThrottleRequests

https://laravel.com/docs/10.x/rate-limiting

image

Either way, it seems the JSON response has all you need to properly offer a translation.

The message key, can also be used to check the error returned, once one expects them not to change.

A 422 can be checked against the initial part with String().startsWith() in JS land.

If you still believe this should make into the framework, you could send a PR proposing to add the missing validator method to its interface.

And then trying to propose a change to this exception.

Not having the getTranslator() method was the cause of this issue, for example:

#46452

I still, think everything is fine as it is, and that translation doesn't belong within an exception.

And mind that I am also not a native English speaker, and most projects I work on require translation.

But you can give it a try if you disagree.

Have a nice day =)

@driesvints
Copy link
Member

Reverting this one: #50546

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