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

More SMTP options in config #1067 #1077

Closed
wants to merge 2 commits into from
Closed

More SMTP options in config #1067 #1077

wants to merge 2 commits into from

Conversation

gyaaniguy
Copy link

Added SMTP port, SMTP authentication type , and SMTP security protocol to config file. So that it can be set from .env file.
I did not get a chance to test it. Apologies if I did something incorrect.

@lcharette lcharette self-requested a review April 8, 2020 19:21
@lcharette lcharette self-assigned this Apr 8, 2020
@lcharette lcharette added this to the 4.4.x milestone Apr 8, 2020
@lcharette lcharette added bakery Related to the Bakery feature core feature request Feature request mail Email related labels Apr 8, 2020
@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #1077 into hotfix will decrease coverage by 0.18%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             hotfix    #1077      +/-   ##
============================================
- Coverage     67.57%   67.39%   -0.19%     
- Complexity     1976     1991      +15     
============================================
  Files           170      170              
  Lines          6917     6937      +20     
============================================
+ Hits           4674     4675       +1     
- Misses         2243     2262      +19     
Impacted Files Coverage Δ Complexity Δ
app/sprinkles/core/src/Bakery/SetupSmtpCommand.php 0.00% <0.00%> (ø) 47.00 <0.00> (+15.00)
app/sprinkles/core/src/Mail/Mailer.php 39.47% <0.00%> (+1.31%) 26.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e6240c...35869dd. Read the comment docs.

Comment on lines +235 to +237
'port' => getenv('SMTP_PORT') ?: null,
'auth' => getenv('SMTP_AUTH') ?: null,
'secure' => getenv('SMTP_SECURE') ?: null, // Enable TLS encryption. Set to `tls`, `ssl` or `false` (to disabled)
Copy link
Member

Choose a reason for hiding this comment

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

Hum... This would create a breaking change for people without those settings already in their env, which push this change to UF 4.5. I wonder if there's a way to keep the default here, and overwrite in the env?

Comment on lines +183 to +184
$smtpAuth = ($input->getOption('smtp_auth')) ?: $this->io->ask('SMTP Server Authentication', 'true');
$smtpSecure = ($input->getOption('smtp_secure')) ?: $this->io->ask('SMTP Server Security type', 'tls');
Copy link
Member

Choose a reason for hiding this comment

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

A yes/no question or choices could probably better here.

@alexweissman
Copy link
Member

I would maybe try to make it consistent with Laravel's environment variables? See https://github.com/laravel/laravel/blob/master/.env.example

@lcharette lcharette modified the milestones: 4.4.x, 4.5.0, 4.x.x Apr 21, 2020
@lcharette lcharette modified the milestones: 4.5.0, 4.6.0 Nov 14, 2020
@lcharette lcharette changed the base branch from hotfix to develop March 9, 2021 00:52
@lcharette
Copy link
Member

I manually merged in 82a1706 as I wasn't able to update the original branch. I manually fixed the comments and conflict at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bakery Related to the Bakery feature core feature request Feature request mail Email related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bakery options for: smtp:<port>, secure:<true|false>, auth:<true|false>
3 participants