daniel-de-wit / lighthouse-sanctum

Laravel Sanctum support for Laravel Lighthouse
MIT License
57 stars 9 forks source link

Provide Localization and customization #71

Open ArneLibbrecht opened 2 years ago

ArneLibbrecht commented 2 years ago

Localization and customization

This PR provides translations for exceptions and messages that were static in the current version of lighthouse-sanctum. These files can be published with a seperate tag, which allows users to customize the exceptions and add more translations for their project. This is also mentioned in the readme.

php artisan vendor:publish --tag=lighthouse-sanctum-translations

This PR introduces new optional behavior and according to the tests no breaking changes, because of this I would suggest a minor version upgrade.

Squashed commit messages

wimski commented 2 years ago

I'd suggest you create your own translation package to accompany this one. Just like this package for Laravel itself (which doesn't come with all translations either). Here's an article about doing this.

Some other points:

@daniel-de-wit do you agree?

ArneLibbrecht commented 2 years ago

Thank you for your feedback.

The package you referred too only provides translation files but the function to use these translation files is already implemented in the projects. In this project the functionality is also implemented where the translator class is used.

However as you said this is not done in exceptions. Normally I would agree that exceptions do not need to be localized but since some of these exception messages even are directed to the end user, it's clear to me that they are meant for the end user, hence they should be translated.

        if ($user instanceof MustVerifyEmail && ! $user->hasVerifiedEmail()) {
            throw new AuthenticationException('Your email address is not verified.');

When the exceptions are thrown graphql will return these to the frontend (unless caught by a wrapper directive applied to the fields in the sanctum graphql file).

If translation is handled client-side this is no problem, but if server-side translation is preferred it needs to happen in the package itself or in a wrapping directive around the the fields of this package. In my opinion, the direct approach is cleaner and less prone to bugs.

I included the password and validation files to be able to add attributes specifically for authentication messages.

@wimski Do these clarifications change your stance on the matter?

wimski commented 2 years ago

Seeing as this package is in the Laravel ecosystem, we try to keep it consistent with Laravel itself.

Laravel does not translate the email verification error.

Illuminate\Auth\Middleware\EnsureEmailIsVerified

            return $request->expectsJson()
                    ? abort(403, 'Your email address is not verified.')
                    : Redirect::guest(URL::route($redirectToRoute ?: 'verification.notice'));

They take the same stance on translating exception messages: https://github.com/laravel/framework/issues/36634

So let's make a deal. If you can get Laravel to translate their exception messages, I'll be happy to implement them here 😉