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

Email locale set back to default, when locale set within Mailable #25691

Closed
amadeann opened this issue Sep 18, 2018 · 8 comments
Closed

Email locale set back to default, when locale set within Mailable #25691

amadeann opened this issue Sep 18, 2018 · 8 comments
Labels

Comments

@amadeann
Copy link
Contributor

amadeann commented Sep 18, 2018

  • Laravel Version: 5.6.33
  • PHP Version: 7.2.5
  • Database Driver & Version: n/a

Description:

Illuminate\Mail\Mailable has a public method locale, which allows to set a locale for a specific Mailable. It was added in #23178.

I would like to set the locale in Mailable's constructor, based on the value of an object passed to Mailable, as in:

class AppointmentSummary extends Mailalbe
{
    ...
    public function __construct(Appointment $appointment)
    {
        $this->appointment = $appointment;

        $this->locale($this->appointment->customer->locale);
    }
    ...
}

I'm setting the locale in the constructor, and not by chaining the locale method as is Mail::to($appointment->customer->email)->locale($appointment->customer->locale)->send(new AppointmentSummary($appointment)) to avoid code duplication (the user should be able to preview the email before sending), and to be able to test whether the email was properly translated using render method.

Setting locale in the constructor (as I want) works fine for email preview purposes, but when I use the 'chaining' approach (Mail::to($appointment->customer->email)->send(new AppointmentSummary($appointment)) without chaining on the locale, the locale set in constructor gets overwritten to default before sending.

This is because Mail::to() returns a PendingMail object, which sets back the mail locale to either default, or the one passed to it via chaining. That is:

  1. Mail::to($appointment->customer->email) instantiates, and returns PendingMail
  2. PendingMail::send(new AppointmentSummary($appointment)) calls fill its fill method, which refills the locale.

And since the locale was sent on the Mailable, and not on the newly instantiated PendingMail, the locale set in the constructor of Mailable is forgotten by the time the mailable is sent.

EDIT:

AppointmentSummary extends my custom Mailable, which overrides a default render method. This allows to display translated preview, but has nothing to do with the fact that you cannot set the locale in Mailable's constructor (the unexpected behavior that I am reporting here).

<?php

namespace App;

use Illuminate\Mail\Mailable;
use Illuminate\Container\Container;
use Illuminate\Contracts\Translation\Translator;

class CustomMailalbe extends Mailable
{
    /**
     * Overriding Laravel's implementation to be able to 'hot-translate'
     * Mailables
     */
    public function render()
    {
        $translator = Container::getInstance()->make(Translator::class);

        return $this->withLocale($this->locale, $translator, function () {
            Container::getInstance()->call([$this, 'build']);

            return Container::getInstance()->make('mailer')->render(
                $this->buildView(), $this->buildViewData()
            );
        });
    }
}
@amadeann amadeann changed the title Email Locale set back to default, when locale set withing Mailable Email locale set back to default, when locale set within Mailable Sep 18, 2018
@derekmd
Copy link
Contributor

derekmd commented Sep 20, 2018

Framework fix

The current implementation puts the full localization responsibility on the intermediate PendingMail object:

https://github.com/laravel/framework/blob/5.7/src/Illuminate/Mail/PendingMail.php#L162

    protected function fill(Mailable $mailable)
    {
        return $mailable->to($this->to)
                        ->cc($this->cc)
                        ->bcc($this->bcc)
                        ->locale($this->locale);
    }

To allow the Mailable itself to decide on locale, it should probably only pass on the locale when Mail::to()->locale() was called:

    /**
     * Populate the mailable with the addresses.
     *
     * @param  \Illuminate\Mail\Mailable  $mailable
     * @return \Illuminate\Mail\Mailable
     */
    protected function fill(Mailable $mailable)
    {
        return tap($mailable, function ($mailable) {
            $mailable->to($this->to)
                ->cc($this->cc)
                ->bcc($this->bcc);

            if ($this->locale) {
                $mailable->locale($this->locale);
            }
        });
    }

App-level fix

An immediate app-level solution is adding an App\Mail\Mailable class to your project and override Mailable@locale() so the first call is always used:

https://github.com/laravel/framework/blob/5.7/src/Illuminate/Mail/Mailable.php#L407

    public function locale($locale)
    {
        $this->locale = $locale;

        return $this;
    }

to:

namespace App\Mail;

use Illuminate\Mail\Mailable as BaseMailable;

class Mailable extends BaseMailable
{
    public function locale($locale)
    {
        $this->locale = $this->locale ?? $locale;

        return $this;
    }
}

or:

    public function locale($locale)
    {
        $this->locale = $locale ?? $this->locale;

        return $this;
    }

will ensure the already-assigned locale is never be erased.

class AppointmentSummary extends \App\Mail\Mailable
{
    // ...

Complete solution

I have another framework pull request I never got around to for mailer/notification localization. For this:

Mail::to($appointment->customer)->queue(new AppointmentSummary($appointment));

If the $appointment->customer object implements contract HasLocalePreference, mail localization is covered for you. I'll see if I can get that done this weekend.

@amadeann
Copy link
Contributor Author

amadeann commented Sep 20, 2018

@derekmd

I think the problem lies here:

https://github.com/laravel/framework/blob/5.7/src/Illuminate/Mail/Mailer.php#L135

    /**
     * Begin the process of mailing a mailable class instance.
     *
     * @param  mixed  $users
     * @return \Illuminate\Mail\PendingMail
     */
    public function to($users)
    {
        return (new PendingMail($this))->to($users);
    }

From what I tested, this is what is used when you call Mail facade.

I'm already using a custom Mailable, but I don't know the way to swap PendingMail - I'm not even sure if it is possible.

My end goals are:

  1. Preview the mailable using render method by a different user - customer service person, who's using English locale (customer service agents are multilingual, but their UI is in English). This is done in an iframe in a popup.

  2. Send the previewed locale in German/French/English to the end customer (end customer gets the email in their preferred language).

EDIT:

I have both points working now, but it requires me to set the same locale in 2 places: in 'preview' route, and in 'send' method of the controller. I think it useful to be able to 'bind' the locale to a Mailable in its constructor. It even seems to me like it was the intended behavior:

When I am setting German locale in the Mailable's contructor and do this:

$mail = (new AppointmentSummary($appointment))
    ->to($appointment->customer->email);

Mail::send($mail);

The email is sent in German (:+1:).

When I do that:

Mail::to($appointment->customer->email)
    ->send(new AppointmentSummary($appointment));

The email defaults to English (:-1:).

@derekmd
Copy link
Contributor

derekmd commented Oct 5, 2018

5.7.7 adds a HasLocalePreference contract you can add to your Customer object: https://laravel-news.com/mail-localization

class Customer implements HasLocalePreference
{
    public function preferredLocale()
    {
        return $this->locale;
    }
}
Mail::to($appointment->customer)
    ->queue(new AppointmentSummary($appointment));

Mail::to($employee)
    ->locale($appointment->customer->locale)
    ->queue(new AppointmentSummary($appointment));

Mailable classes themselves should never have to refer to locale as the Mail service handles switching between locales.

@driesvints
Copy link
Member

Please try the suggestion provided by @derekmd.

@danijelk
Copy link
Contributor

Lol, you can't just close the issue without a proper fix in place.

We had the same issue, and 'solved' it by setting locale on the Mail facade. But according to docs locale 'should' be set to the Mailable, so either docs need an update, or the fill method needs patching to honour $locale from Mailable before trying to overwrite it.

Let us know what you prefer and we'll provide a PR if needed.

@driesvints
Copy link
Member

@danijelk okay, I'll reopen this and have a thorough look later.

@derekmd
Copy link
Contributor

derekmd commented Jan 19, 2019

according to docs locale 'should' be set to the Mailable

The documentation mistakenly stated that due to a miscommunication when the localization feature was added. The docs have since been fixed to note the Mail facade (or HasLocalePreference contract) should be used to set the locale: laravel/docs#4875

@driesvints
Copy link
Member

@derekmd thanks for pointing that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants