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] Fix default theme for Markdown mails #29274

Merged
merged 2 commits into from Jul 25, 2019

Conversation

@ghost
Copy link

commented Jul 23, 2019

As metioned in this comment on pr #29132,
the pull request breaks the config mail.markdown.theme because it will be overwrited on:

if (property_exists($message, 'theme') && ! is_null($message->theme)) {
$this->markdown->theme($message->theme);
}

This PR leave the MailMessage::$theme null so MailChannel will ignore it.
The feature added in PR #29132 still available.

BinotaLIU added some commits Jul 23, 2019

BinotaLIU
set default value to null
we don't need to set default theme value here.
The default theme is take care in Illuminate\Mail\Markdown::__constractut.
Leave it null as default so config('mail.markdown.theme') can take effect.
BinotaLIU
fix tests, theme() no longger be called by default
The default value of theme make the function no longger be called, it only called if it has value
@ghost

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

cc @JackWH, @troccoli can you take a look on this?

I wonder if there need to add more test?

@taylorotwell taylorotwell merged commit e476a94 into laravel:5.8 Jul 25, 2019

2 checks passed

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

@ghost ghost deleted the fix-markdown-default-theme branch Jul 30, 2019

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