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

[5.6] Ability to set locale on sent Mailable #23178

Merged
merged 3 commits into from
Feb 22, 2018

Conversation

themsaid
Copy link
Member

This PR is an attempt to allow configuring a specific locale that'll be used while sending a Mailable.

Let's say the current locale is English, while user B has a default language of Arabic, system is sending a queued mail to user B and want to specify that the language to be used while building the email message is to be Arabic.

Mail::to('userb@mail.com')->locale('ar')->send(new InvoiceMail());

Doing so will instruct Laravel to switch the default language right before building the email to Arabic, and then once the email is sent it sets it back to whatever it was before the change.

This, based on my tests, ensures the daemon worker process will have the default locale set back to its original value after the mail is sent.

@sisve
Copy link
Contributor

sisve commented Feb 15, 2018

Does this also work if we queue the mails using the queue and later methods?

@themsaid
Copy link
Member Author

Yes @sisve, both works:

\Mail::to('asd@asd.com')->locale('ar')->queue(new \App\Mail\TestMail());

\Mail::to('asd@asd.com')->locale('it')->later(5, new \App\Mail\TestMail());

@garygreen
Copy link
Contributor

Would it be possible to only initialise the translator if a locale is set? Seems unnecessary to always initialise translator at the service provider level when there's many apps that aren't multi lingual.

@themsaid
Copy link
Member Author

@garygreen but we don't initialise it, we just register the instance if it's already bound to the container.

if ($mailer->translator) {
$mailer->translator->setLocale($currentLocale);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you have an interest in offering a ChangesLocale concerns trait that can be re-purposed in user-land?

trait ChangesLocale
{
    /**
     * @param \Illuminate\Contracts\Translation\Translator $translator
     * @param string                                       $locale
     * @param callable                                     $callback
     *
     * @return mixed
     */
    public function asLocale($translator, $locale, callable $callback)
    {
        if ($translator && $locale) {
            try {
                $currentLocale = $translator->getLocale();

                $translator->setLocale($locale);

                return $callback($locale);
            } finally {
                $translator->setLocale($currentLocale);
            }
        } else {
            return $callback($locale);
        }
    }
}

Then send() simplifies to:

public function send(MailerContract $mailer)
{
    $this->asLocale($mailer->translator, $this->locale, function () use ($mailer) {
        Container::getInstance()->call([$this, 'build']);

        $mailer->send($this->buildView(), $this->buildViewData(), function ($message) {
            $this->buildFrom($message)
                ->buildRecipients($message)
                ->buildSubject($message)
                ->runCallbacks($message)
                ->buildAttachments($message);
        });
    });
}

The trait I've been wrapping Mailer@send() with for awhile is much simpler when assuming a translator is bound to the app container:

trait ChangesLocale
{
    public function asLocale($locale, callable $callback)
    {
        try {
            $currentLocale = app()->getLocale();

            if ($locale) {
                app()->setLocale($locale);
            }

            return $callback($locale);
        } finally {
            if ($locale) {
                app()->setLocale($currentLocale);
            }
        }
    }
}

@derekmd
Copy link
Contributor

derekmd commented Feb 17, 2018

It would be great to also have the ability for users to have an email region/language preference. It can be pulled from a model attribute in a Mail::to($user) object where locale is in e.g., $user->locale or $user->email_locale.

Mail::to($user)->locale($user->locale)->queue(...) would do but when you have a codebase heavy in transactional mailables, those expressions get really repetitive.

Here's an potential commit to follow this pull request: derekmd@3c48884

@themsaid
Copy link
Member Author

@derekmd per recipient locale makes sense yes, I'll just wait and see if this implementation is the best way to go and then add your idea. Will also need to work on the Notifications locale part, so will just wait till this PR is merged/rejected/edited.

@taylorotwell
Copy link
Member

Thinking about @derekmd's suggestion of a pretty generic trait that wraps up @themsaid's try / finally logic is pretty interesting. Could we basically "solve" this feature by adding that trait in Illuminate\Support or Illuminate\Foundation and not even make any changes to mailables or notifications? We would just let people pass whatever locale they want into the mailable or notification and call the trait method if they need it?

@taylorotwell
Copy link
Member

@derekmd @themsaid some kind of "HasLocalePreference" thing on users would indeed be nice so we could check for it in various places where not only users send mail but the framework itself (password reminders, etc.)

@sebastiaanluca
Copy link
Contributor

Would be nice to have one point of setup that handles mailables, notifications, ánd framework notifications like password resets! Makes total sense to enable sending stuff in the language of the recipient and not the sender or default app locale.

@themsaid
Copy link
Member Author

Thing is recipients can be multiple each with different locales, so you must decide which locale the mail will be sent with.

@taylorotwell
Copy link
Member

taylorotwell commented Feb 22, 2018

Yeah, otherwise we would basically have to sort out the recipients by locale and send a mail for each locale. That may be complicated to implement. Not sure. It could possibly be done in the PendingDispatch destructor.

@sebastiaanluca
Copy link
Contributor

sebastiaanluca commented Feb 22, 2018

How would sending a notification in a specific locale look like?

Notification::locale('ar')->send($users, new InvoicePaid($invoice));

// and

$user->locale('ar')->notify(new InvoicePaid($invoice));

Like that? Or even:

$user->notify(new InvoicePaid($invoice));

Where the locale is resolved automatically like routeNotificationFor does.

I'm not aware of all the internal code that handles this so thinking out loud here, but if you leave handling locales for multiple recipients to the developer, they can e.g. create a Notification macro that loops the recipients and sends the notification in each recipient's locale separately (I'm doing something similar in a current project).

Or you could provide an internal method that splits recipients with locales and those without, then send a generic version of the notification to those without (in the current app's locale or a default provided one) and a translated notification to each user that has a locale resolve method implemented.

@taylorotwell
Copy link
Member

I don't know. We can probably do the simplest implementation on notifications and go from there.

@sebastiaanluca
Copy link
Contributor

Of course, already happy this issue is getting some traction :)

Think the most "urgent" use case would be sending any kind of notification (whether it be a notification, mailable, …) to a single recipient in their locale and ideally having that auto-resolved like what's done for notification's to value. Sending to multiple recipients can be offloaded to a custom implementation after that for the time being.

@cayaner
Copy link

cayaner commented Feb 22, 2018

timezones needs to be localized as well, great job.

@derekmd
Copy link
Contributor

derekmd commented Feb 22, 2018

Sorry, this post turned out too long. It doesn't apply to notification channels like Slack and SMS that don't offer any multi-reply ability (as in, the recipient responds to the notification they received.) TLDR: Make user-land decide what to do for multiple recipients of mailables.

Proposed branch for recipient locale preference on Mail service

My branch is updated to meet to meet a new Illuminate\Contracts\Translation\HasLocalePreference:

https://github.com/derekmd/framework/tree/mail-notification-preferred-locale

Mail sends only one email for multiple recipients

When it comes to the Mail service, I think the framework auto-localizing every recipient (Mail::to($anArray) / Mail::to($aCollection)) abandons the behavior of email itself.

  • mailable -> Swift Message -> actual email.

When email has to be broken up for different email subjects and bodies:

  • The outbound email service (MailGun, Mandrill, etc.) will receive more than one request for each translated version of the Mailable.
  • PendingMail now must queue multiple Mailable items for every translation.
  • Email To:, Cc:, etc. integrity is lost. If any recipient tries a Reply-All in their email client, some of the other recipients may be missing since they were sent another translation.

Put multiple recipients in a class meeting contract HasLocalePreference

It seems best to make user-land decide what how they want to approach translations for a notification of multiple recipients. As long as the Mail::to() or Notification::send() argument meets the HasLocalePreference contract, a single translation can be resolved while keeping the notification object atomic.

e.g., a Collection class or DistributionList iterable object can decide what locale should be used for all recipients, such as German-only distribution list.

class LocalizedCollection extends Collection implements HasLocalePreference
{
    public function preferredLocale()
    {
        return optional($this->first())->preferredLocale();
    }
});

$recipients = new LocalizedCollection(
    User::where('locale', 'de')->get()
);

As long as the services are macroable, apps can define the common idiom themselves:

Illuminate\Mail\Mailer::macro('queueTranslated', function ($users, $mailable) {
    // `groupBy` probably could be a higher-order proxy, hey.

    return $users->groupBy(function ($user) {
        return $user->preferredLocale();
    })->map(function ($users, $locale) use ($mailable) {
        return Mail::to($users)->locale($locale)->queue($mailable);
    });
});

Mail::queueTranslated(
    $users, new Newsletter(request('body'))
);

In many cases for a notification being shared between recipients I can see apps instead putting every translation in a single email view to keep To:, Cc:, etc. of every recipient, so a Blade directive @locale() block would make sense for that.

Most notifications channels are fine to localize every recipient

Again, text messages, Slack notifications, etc. would be fine to localize for each individual recipient. I just think email is going to be the problem area here.

@sisve
Copy link
Contributor

sisve commented Feb 22, 2018

So, perhaps we should unlock the internals thread again? Because this PR does not completely cover all the cases discussed in it. Or should this PR be considered incomplete based on lacking functionality discussed in the internals thread?

@daison12006013
Copy link

daison12006013 commented Feb 23, 2018

Might be good inside the Notification too @themsaid.

  • since most structure we pull the user's locale from their settings and set it as their default locale on their session.
  • or we actually have a locale selection in our web apps and we modify their current session to use the selected one.

Suggest:

$user
    ->notify(new SendSomethingThatNotifiesTheUser)
    ->locale(... user's locale setting or the selected locale in the selection);

This should help when the notification is pushed to queuing, as we have localization inside the messages when using OneSignal/Pusher, and should automatically switch from the passed locale.


Workaround/Old Way

// handle the locale inside a base class Notification, which is fine too
$user->notify((new SendSomethingThatNotifiesTheUser())->setLocale('cn'));

@CyrilMazur
Copy link
Contributor

Why only set the locale of the translator, and not the locale of the app?

My use case is that I rely on App::getLocale() inside my views (I have helpers that format numbers and dates based on the user's locale, and they get it from App::getLocale()).

@kamui545
Copy link
Contributor

I've got exactly the same use case as @CyrilMazur.
@themsaid Could you please tell us if there is any reason behind this choice?

@Razorsheep
Copy link

Is something like this for notifications already working in the current release?

// handle the locale inside a base class Notification, which is fine too $user->notify((new SendSomethingThatNotifiesTheUser())->setLocale('cn'));

@daison12006013
Copy link

daison12006013 commented May 3, 2018

@Razorsheep you might interpreted as $user->notify($instance)->setLocale(...)

It was like this: $user->notify($instance->setLocale(...)) it will always be on your notification instance instead.

And you need to implement it on your own, which does the same thing. Then under your base notification you can create an accessor getLocale() to fetch the instance settled value.

@iwasherefirst2
Copy link

When you save the email text in a variable in your constructor, then Mail::to(..)->locale(..) won't work.

@CyrilMazur
Copy link
Contributor

When you save the email text in a variable in your constructor, then Mail::to(..)->locale(..) won't work.

Translatable text must be set in your mailable's build() method.

@olivermbs
Copy link

Perhaps late for the party, but can fallback locale be set on a Mailable? For example, Swiss-German (de_CH) has some subtle changes to German (de), we want to set the primary locale to de_CH and then fallback to de if a translation isn't found. On an app level we do this through a helper function in Middleware:

function setAppropriateLocale($locale = 'en'): string
    {
        // Set the primary locale ie 'de_CH'
        app()->setLocale($locale);

        // Extract the primary language code (first 2 letters like de,it,es)
        $primaryLanguage = substr($locale, 0, 2);

        // Set the fallback locale based on the primary language code
        app()->setFallbackLocale($primaryLanguage);

        Session::put('locale', $primaryLanguage);

        return $primaryLanguage;
    }

It would however be great to have this possible also for queued Mailables, perhaps a second optional parameter to the locale method?

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.