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.8] Add template theme to mail notifications #29132

Merged

Conversation

@troccoli
Copy link
Contributor

commented Jul 10, 2019

It is already possible, and quite easily, to set a theme for any Mailables. This is not the case for the mail Notifications, which always use the default theme.

With this PR it would be possible to do something like

public function toMail($notifiable)
{
  $url = url('/invoice/'.$this->invoice->id);

  return (new MailMessage)
      ->theme('notification')
      ->greeting('Hello!')
      ->line('One of your invoices has been paid!')
      ->action('View Invoice', $url)
      ->line('Thank you for using our application!');
}

This will use notification.css theme in resources/views/vendor/mail/html/themes.

Merge pull request #5 from laravel/5.8
Merge latest 5.8

@troccoli troccoli force-pushed the troccoli:add-template-theme-to-mail-notifications branch 2 times, most recently from 5deaa5f to dfb7ef5 Jul 10, 2019

@troccoli troccoli changed the title Add template theme to mail notifications [5.8] Add template theme to mail notifications Jul 10, 2019

@troccoli troccoli force-pushed the troccoli:add-template-theme-to-mail-notifications branch from dfb7ef5 to 6efe70b Jul 10, 2019

Giulio Troccoli-Allard
Add ability to set theme for mail notifications
We already can set a different theme than the default one for our
Mailable. It would be very useful if there was a way to set the theme
for the mail notifications too.

@troccoli troccoli force-pushed the troccoli:add-template-theme-to-mail-notifications branch from 6efe70b to aed06b1 Jul 10, 2019

@taylorotwell taylorotwell merged commit 2e96129 into laravel:5.8 Jul 11, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@troccoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Would there be any interest in backporting this to 5.7?

We are using 5.7 here and it would be useful for us.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Laravel 5.7 is not accepting new features now, since it is EOL. Only security fixes. Please upgrade to Laravel 5.8.

@martinbean

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Please upgrade to Laravel 5.8.

@GrahamCampbell Upgrading a framework is easier said that done for some teams and products. Especially if 5.7 to 5.8 is a major framework upgrade as defined here: https://laravel.com/docs/5.8/releases#versioning-scheme

@driesvints

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@martinbean Laravel Shift can help you: https://laravelshift.com/

@martinbean

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@driesvints I’m not having issues upgrading…?

I was just empathising with @troccoli and trying to convey to @GrahamCampbell that not every developer is able to perform an upgrade to a major framework version every ~6 months, i.e. if a developer is working for a company with limited resources.

@driesvints

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@martinbean you said:

Upgrading a framework is easier said that done for some teams and products.

So I was just pointing out to a tool that could help you upgrade faster/easier.

@martinbean

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@driesvints Again, it’s not me in that situation, but have worked in product teams in the past where shipping features is a higher priority for teams than keeping package versions up to date.

@driesvints

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@martinbean I don't want to get into a "priority" discussion here. I've worked in teams before where upgrading isn't always a top priority. But you're gonna be left behind and run into security risks/bugs eventually if you do. Expecting others to keep maintaining old versions if you're not willing to upgrade is kind of demanding if you ask me.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

LTS releases are also available. For example, StyleCI is actually running on Laravel 5.5 LTS. This is so I don't have to spent lots of time upgrading every few months.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Features are very rarely backported however. If you need new features from the core, then upgrading is the only way.

@DevDavido

This comment has been minimized.

Copy link

commented Jul 21, 2019

Nice idea, definitely like it! It would be great though to get it documented in the docs.

@troccoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Absolutely @DevDavido. I'm on holiday atm though so it will have to wait a week or two.

And with regards to all the comments about my asking about 5.7 I just want to say that I'm ok not doing it. I find it a bit odd that it already accept only security fixes but it's not my repo or my policy.

@JackWH

This comment has been minimized.

Copy link

commented Jul 21, 2019

Unless I'm misunderstanding, I don't think this change is backwards-compatible — it's broken the custom Markdown mail styling I was using in my app.

In src/Illuminate/Notifications/Messages/MailMessage.php, the $theme property is now being set:

/**
 * The current theme being used when generating emails.
 *
 * @var string|null
 */
public $theme = 'default';

Previously, the MailServiceProvider determined the Markdown theme based on your config file:

return new Markdown($this->app->make('view'), [
    'theme' => $config->get('mail.markdown.theme', 'default'),
    'paths' => $config->get('mail.markdown.paths', []),
]);

After this change, anyone who had changed their mail.php config item for mail.markdown.theme will no longer see their custom theme being used as the default. Instead, they'll have to manually specify which theme to use on every Notification class.

@troccoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

That's interesting. Which version are you using? The reason I submitted the PR was that I needed an easy way to change the theme for notification and I didn't find one in 5.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.