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

fix: remove test@example.org from BCC in mailpit sendmail_path, fixes #5381 #5383

Merged
merged 2 commits into from Sep 29, 2023

Conversation

joelpittet
Copy link
Contributor

@joelpittet joelpittet commented Sep 26, 2023

The Issue

The test@example.org is added to all email's BCC in MailPit through PHP's sendmail_path config.

How This PR Solves The Issue

Remove unneeded args from sendmail_path

Manual Testing Instructions

Send email from PHP inside the web container and go to mailpit to look at BCC line.

Automated Testing Overview

@joelpittet joelpittet requested a review from a team as a code owner September 26, 2023 17:02
@joelpittet joelpittet changed the title Remove test@example.org from mailpit sendmail_path fix: Remove test@example.org from BCC in mailpit sendmail_path, fixes #5381 Sep 26, 2023
@joelpittet
Copy link
Contributor Author

I think the title does conform to the standard

@rfay rfay changed the title fix: Remove test@example.org from BCC in mailpit sendmail_path, fixes #5381 fix: remove test@example.org from BCC in mailpit sendmail_path, fixes #5381 Sep 26, 2023
@rfay
Copy link
Member

rfay commented Sep 26, 2023

It's pretty picky. Not sure why it has to be that picky. fixed the capitalization.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Sep 26, 2023
@rfay
Copy link
Member

rfay commented Sep 26, 2023

I pushed the needed docker images and added a commit to refer to them. I'm not sure why github actions hasn't provided the normal link to the artifacts for you to test. Maybe it will show up before long.

@rfay
Copy link
Member

rfay commented Sep 26, 2023

Artifacts for testing are at https://github.com/ddev/ddev/actions/runs/6319112543

@rfay rfay requested a review from tyler36 September 26, 2023 23:51
@tyler36
Copy link
Collaborator

tyler36 commented Sep 27, 2023

Laravel 10

Issue not present with Laravel 10.4.1 & DDEV 1.22.3

  • Generic mail
    Mail::raw('Hello World!', function($msg) {$msg->to('example@example.com')->subject('Test Email'); });

image

  • Specify 'secret@example.com as BCC
    Mail::raw('Hello World!', function($msg) {$msg->to('example@example.com')->bcc('secret@example.com')->subject('Test Email'); });

image

Drupal 10

Confirm issue on Drupal 10 with DDEV 1.22.3

  • Use UI to send password reset email to admin@example.com
    image

However, DDEV 1.22.3-13-gc2dd4f439 appears to fix the issue.

  • Use UI to send password reset email to admin@example.com
    image

@rfay
Copy link
Member

rfay commented Sep 27, 2023

@tyler36 I think Laravel doesn't use the php config and connects directly via SMTP, which would be why you wouldn't see this problem on Laravel. Things like Symfony Mailer also use directly configured SMTP so wouldn't be affected by #5381

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This looks right to me, but would love to have some other people give it a go and review like @tyler36 did.

@tyler36
Copy link
Collaborator

tyler36 commented Sep 27, 2023

I think Laravel doesn't use the php config

Yes, thats right. Just confirming no issue with Laravel.

@tyler36
Copy link
Collaborator

tyler36 commented Sep 27, 2023

Confirming Laravel 10.4.1 & DDEV 1.22.3-13-gc2dd4f439 works exactly the same too; so nothing broke. 😄

@rfay rfay merged commit ba3cee4 into ddev:master Sep 29, 2023
24 checks passed
@rfay
Copy link
Member

rfay commented Nov 1, 2023

I think this may have been a mistake, or we don't know yet how far we have to go. Shopware 6 croaks installing demo data with this:

Internal Server Error
Unsupported sendmail command flags "/usr/local/bin/mailpit sendmail --smtp-addr 127.0.0.1:1025"; must be one of "-bs" or "-t" but can include additional flags.
image

@joelpittet
Copy link
Contributor Author

@rfay oh sorry maybe I over stepped with the -t removal :( Would you like me to submit a new PR to add it back?

@rfay
Copy link
Member

rfay commented Nov 1, 2023

It was reasonable, and @Axilent approved in the issue.

@Axilent, would you mind commenting on #5383 (comment) ? Is that just poor usage by Shopware6?

@joelpittet I'd love it if you'd follow up with an option depending on the discussion. If you know anything about Shopware 6, that much better!

@joelpittet
Copy link
Contributor Author

I know nothing of Shopware6, but it's trying to parse the command for valid flags. I tried to get symfony to stop doing that one time... might be related (was rejected).
https://www.drupal.org/project/swiftmailer/issues/3174215
symfony/symfony#47020

@rfay
Copy link
Member

rfay commented Nov 2, 2023

I'll bet it is the same thing. This is a symfony build.

@joelpittet
Copy link
Contributor Author

@rfay Separate PR adding back the -t flag?

@rfay
Copy link
Member

rfay commented Nov 2, 2023

Yeah, please do. Thanks so much.

rfay added a commit that referenced this pull request Nov 7, 2023
…mand (#5496)

Co-authored-by: Randy Fay <randy@randyfay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants