-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Updated mail config - needs testing #17775
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: snipe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunately very hard to see what actually is changing here. For some of the issues that our user reported. I think the MAIL_FROM_ADDR
issue that I mentioned could be a problem. Some comments in here would be helpful, as some of these config settings are not self-explanatory.
'from' => [ | ||
'address' => env('MAIL_FROM_ADDR', null), | ||
'name' => env('MAIL_FROM_NAME', null), | ||
'address' => env('MAIL_FROM_ADDRESS', '[email protected]'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the name of this .env
var is going to cause a lot of problems - if we want to use the 'new' way going forward, can we do something like:
env('MAIL_FROM_ADDRESS',env('MAIL_FROM_ADDR','[email protected]'))
That way the old env var will still work - this could definitely be an issue for our hosted platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See how @jerm handled this in MAIL_MAILER
and MAIL_DRIVER
in this file, above)
'theme' => 'default', | ||
|
||
'paths' => [ | ||
resource_path('views/vendor/mail'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path is still populated - is removing this needed? Or is it a default that's now fully-baked in, and thus no longer needed in this config file?
| | ||
*/ | ||
|
||
'scheme' => env('MAIL_SCHEME'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
*/ | ||
|
||
'scheme' => env('MAIL_SCHEME'), | ||
'url' => env('MAIL_URL'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this also - is that the base URL for images or links in mail, or something else?
These values are pulled from the current Laravel 11 mail config. They don't explain all of them well, so I don't actually know how required they are. |
Not sure if this will work as expected, still needs a lot of testing.