Skip to content

WIP: Configure mailer messenger #11180

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

Closed
wants to merge 57 commits into from

Conversation

mabumusa1
Copy link
Member

@mabumusa1 mabumusa1 commented May 20, 2022

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [X]
Deprecations? [X]
BC breaks? (use the c.x branch) [ X]
Automated tests included? [N]
Related user documentation PR URL I need help writing the new docs for the changes
Related developer documentation PR URL mautic/developer-documentation-new#40
Issue(s) addressed Swiftmailer has ended and this PR refactor Mautic and use Symfony/Messenger and Symfony/Mailer instead

Description:

Messenger Bundle
This PR introduce a new bundle called MessengerBundle which integrates Symfony/Messenger which acts as the main bus and queue engine for Mautic.

How this bundle works?

  1. It inject itself as a dependency all the time, I need help to make the injection dynamic. I tried several approaches but all of them would require cleaning the cache. In my opinion we do not need Mautic to send the email directly from memory. let's make use the queue all the time.
  2. It uses doctrine (database) as the default queue where it store messages before sending.
  3. The bundle uses a single parameter in local.php in format of DNS, this DNS is composed in each plugin that inject itself as a messenger plugin
  4. This bundle is extendable to support other queues like Redis, I wrote a plugin for Redis to demo how the code can be used to to inject more transports. I intended to open-source this plugin.
  5. There is an env var that we use to define the consumer name. MAUTIC_MESSENGER_CONSUMER_NAME this will change the name of consumer that will listen to the queue and dispatch the messages.
  6. The command mautic:email:send has been deleted because of this bundle, and the spool functionality as well.
  7. From now on the command to send messages from the queue will be php bin/console messenger:consume

What is missing for this bundle

  1. Automated tests
  2. Deciding whether we need to inject it all the time or have the option to enable/disable it
  3. Manual testing

Email Bundle
In the email bundle we rely on [Symfony/Mailer](https://symfony.com/doc/current/mailer.html) and removed all the references to swiftmailer

What has changed ?

  1. All the transports has been removed, because we want them to plugins outside the core of Mautic
  2. Mailer is integrated with messenger so emails are queued directly to the database (or your choice of queue).
  3. Callbacks classes remained the same but their URLs will be changed depending on the transport name from example amazon_api has been changed to ses+api.
  4. There is a single parameter that configure mailer in a format of DSN like this ses+api:username:password@default?region=us-east-1
  5. I included SES plugin that will be open-sourced as well.

What is missing for this bundle

  1. Automated tests
  2. Including other transports as plugins
  3. Manual testing

This PR introduce the following improvements:

  1. Faster email sending, I tested with SES API and managed to get 3.5X speed increase compared to what we have in M4.
  2. Less memory consumption as messenger compacts the emails when they are stored in the queue
  3. Storage optimization, with M4 file spool, the disk may get filled with messages pending but with this change the messages will be loaded to redis or database and multiple processes can send at the same time.
  4. We were able to run 32 processes to send emails simultaneously
  5. This PR will be the door to introduce faster and more lightweight transports
  6. We can use the transports that are maintained by Symfony without extra work on our side

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label May 20, 2022
@mabumusa1 mabumusa1 force-pushed the configure_mailer_messenger branch 2 times, most recently from a8280dc to 57cb36a Compare May 20, 2022 08:36
@mabumusa1 mabumusa1 closed this May 20, 2022
@mabumusa1 mabumusa1 force-pushed the configure_mailer_messenger branch from dff5f58 to 973e9ca Compare May 20, 2022 08:47
@mabumusa1 mabumusa1 reopened this May 20, 2022
@mabumusa1 mabumusa1 force-pushed the configure_mailer_messenger branch from 7c59457 to e4dd656 Compare May 26, 2022 23:09
@mabumusa1
Copy link
Member Author

@escopecz I would appreciate your support in fixing the failing tests, the failing tests are failing only on GitHub but on my local machine they work. is there something I am missing?
image

For Rector, It changes core files with doctrine which makes Mautic fail.

As for stan, it is showing errors for files that I did not change

And CSfixer is trying to fix files I deleted, maybe there is PR on 4.x that would fix this issue.

@escopecz
Copy link
Member

Try to run the whole phpunit test suite. It looks like the hard-coded IDs in some tests do not match anymore. Maybe you added a new test before these that is messing with the hard-coded IDs that it expects. There are 2 ways to fix it:

  1. Try to call this method: MauticMysqlTestCase::resetAutoincrement(); in the broken test in the setUp method.
  2. Refactor the test out of the hard-coded IDs.

PHPSTAN is checking the whole project. If you change one file it can affect others. That's normal and must be fixed.

I'm not sure what Rector is doing. 4.3 is coming with new versions of PHPSTAN and Rector so we can see what will happen after we merge that into this PR.

@mabumusa1 mabumusa1 self-assigned this May 31, 2022
@mabumusa1 mabumusa1 added this to the 5.0-alpha milestone May 31, 2022
@mabumusa1 mabumusa1 added T3 Hard difficulty to fix (issue) or test (PR) feature A new feature for inclusion in minor or major releases code-review-needed PR's that require a code review before merging needs-documentation PR's that need documentation before they can be merged needs-automated-tests PR's that need automated tests before they can be merged bc-break A BC break PR for major release milestones only needs-rebase PR's that need to be rebased email Anything related to email channels Anything related to channels queue Anything related to the queue performance-scalability Anything related to performance and scalability mautic-5 Relates to Mautic 5.x labels May 31, 2022
@mabumusa1 mabumusa1 changed the title WIP: Configure mailer messenger Configure mailer messenger May 31, 2022
@mabumusa1
Copy link
Member Author

I am waiting on this symfony/symfony#33394 to be merged so we can save the messages as JSON instead of serilazed PHP object

@mabumusa1 mabumusa1 requested review from escopecz, fedys and kuzmany May 31, 2022 15:36
@mabumusa1 mabumusa1 force-pushed the configure_mailer_messenger branch 2 times, most recently from 80b0efb to 22d4940 Compare October 13, 2022 10:19
@mabumusa1 mabumusa1 force-pushed the configure_mailer_messenger branch from 22d4940 to 0e641d0 Compare October 13, 2022 10:57
@mabumusa1
Copy link
Member Author

This PR got closed in favor of #11613 & #11597

@mabumusa1 mabumusa1 closed this Oct 21, 2022
@mabumusa1 mabumusa1 deleted the configure_mailer_messenger branch December 7, 2022 09:27
@escopecz escopecz removed this from the 5.0-alpha milestone Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc-break A BC break PR for major release milestones only channels Anything related to channels cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging email Anything related to email feature A new feature for inclusion in minor or major releases mautic-5 Relates to Mautic 5.x needs-automated-tests PR's that need automated tests before they can be merged needs-documentation PR's that need documentation before they can be merged needs-rebase PR's that need to be rebased performance-scalability Anything related to performance and scalability queue Anything related to the queue T3 Hard difficulty to fix (issue) or test (PR) WIP PR's that are not ready for review and are currently in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants