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

refactor: add sendmail -t flag to mailhog call for symfony/mailer native transport compat, fixes #4363 #5151

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

rfay
Copy link
Member

@rfay rfay commented Jul 17, 2023

The Issue

This PR replaces

Will make sure you get commit credit for it. I couldn't push the image spec into your PR.

How This PR Solves The Issue

Add a harmless -t to the mailhog call

Manual Testing Instructions

Automated Testing Overview

Mailhog integration is not tested AFAIK and soon will be replaced by mailpit

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

@rfay rfay marked this pull request as ready for review July 17, 2023 12:17
@rfay rfay requested a review from a team as a code owner July 17, 2023 12:17
@rfay
Copy link
Member Author

rfay commented Jul 19, 2023

This obviously isn't urgent so I'll wait for testing responses and it will wait until after v1.22.0 is out.

@rodrigoaguilera
Copy link
Contributor

No hurry of course.

My testing steps:

  • First I reproduced the issue on ddev 1.21.6, Drupal 10.1.1 and drupal/symfony_mailer 1.3.0-rc2.
  • I installed a fresh debian 12 with the ddev from the PR and the same fresh Drupal 10.1.1 projects
  • Verified that it was using the 20230716_rodrigoaguilera_sendmail_t docker image
  • Without installing symfony mailer I verified that mailhog captures the reset password email
  • Installed symfony mailer and sent a test email from its interface that was captured by mailhog
  • Created a zero-configuration native mail transport and set it as default
  • Sent another test email with no errors and it was captured by mail so the bug is fixed.
  • Verified the -t flag is not only set for php-fpm but for php-cli also.

This change is good to be included from my testing. Thanks for fixing the PR and uploading the images.

Additional testing

  • Removed the symfony_mailer overrides from settings.ddev.php and tried the default "Sendmail" transport but unfortunately it doesn't use what is set on sendmail_path but it is hardcoded to use the binary sendmail -bs.

@rfay
Copy link
Member Author

rfay commented Jul 26, 2023

So @rodrigoaguilera as far as you're concerned this is ready?

May still need some "ordinary" testing.

@rodrigoaguilera
Copy link
Contributor

I still have the fresh debian 12 machine around with the PR installed so I cloned two of my clients Drupal projects. One of them has nightwatch tests that send emails and they all came green. This project uses the symfony_mailer and the tests check that emails are received in mailhog.
For the other project I did the usual checks as when I deploy to production and more tests around mail sending: cron notifications and newsletter subscription confirmation. All good.

From my side this change is ready.

@rfay rfay force-pushed the 20230716_rodrigoaguilera_sendmail_t branch from 7180472 to e31d922 Compare August 2, 2023 19:03
@rfay rfay changed the title refactor: add -t flag to mailhog call for symfony/mailer native transport compat, fixes #4363 refactor: add sendmail -t flag to mailhog call for symfony/mailer native transport compat, fixes #4363 Aug 2, 2023
@rfay
Copy link
Member Author

rfay commented Aug 2, 2023

Rebased, pushed new image.

@rfay rfay merged commit 29e1cac into ddev:master Aug 3, 2023
23 checks passed
@rfay rfay deleted the 20230716_rodrigoaguilera_sendmail_t branch August 3, 2023 16:00
@rfay
Copy link
Member Author

rfay commented Aug 3, 2023

Thanks so much for chasing this and doing the PR and testing. I tested manually and seemed to be fine.

@rodrigoaguilera
Copy link
Contributor

Thanks to you for maintaining an awesome piece of software

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants