Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[RFC][Mailer][FrameworkBundle] Add support for rate limited mailer transports#59985
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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.
Hi,
Many thanks for the proposal. THis is a nice and useful idea.
I reviewed this PR and it is a good start except the missing tests and a few things I spotted.
| foreach ($transports as $name => $transport) { | ||
| if ($transport['rate_limiter']) { | ||
| if (!interface_exists(LimiterInterface::class)) { |
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.
Thisif could be moved outside thefor loop.
The verification can be done just after if$transportRateLimiterReferences is not empty and the interface is missing.
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.
Yes - however, the same would apply to this existing code:
symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Lines 2458 to 2460 in3ec30b6
| if (!interface_exists(LimiterInterface::class)) { | |
| thrownewLogicException('Rate limiter cannot be used within Messenger as the RateLimiter component is not installed. Try running "composer require symfony/rate-limiter".'); | |
| } |
Do you want me to change it in the unrelated section as well?
| /** | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| */ | ||
| abstract class AbstractTransport implements TransportInterface | ||
| { | ||
| private LoggerInterface $logger; | ||
| private ?RateLimiterFactoryInterface $rateLimiterFactory = null; |
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.
New argument should be placed after to avoid BC breaks.
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.
Hm, how is this related to BC? This is just a private member variable - and not a constructor variable with constructor property promotion.
| $transportRateLimiterReferences = []; | ||
| foreach ($transports as $name => $transport) { | ||
| if ($transport['rate_limiter']) { |
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.
isset orarray_key_exists?
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.
Not necessary as the key will always exist due to the processed configuration. See also:
symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Line 2457 in3ec30b6
| if ($transport['rate_limiter']) { |
| foreach ($transports as $name => $transport) { | ||
| if ($transport['rate_limiter']) { | ||
| if (!interface_exists(LimiterInterface::class)) { |
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.
shouldn't this rather checkisConfigEnabled() for therate_limiter config ? the fact that the component is installed does not guarantee that the config is enabled for it (smart defaults are based on the availability of the component, but projects can still configure things explicitly)
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.
I am using the same logic as here:
symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Lines 2421 to 2464 in3ec30b6
| $senderAliases = []; | |
| $transportRetryReferences = []; | |
| $transportRateLimiterReferences = []; | |
| foreach ($config['transports']as$name =>$transport) { | |
| $serializerId =$transport['serializer'] ??'messenger.default_serializer'; | |
| $tags = [ | |
| 'alias' =>$name, | |
| 'is_failure_transport' =>\in_array($name,$failureTransports,true), | |
| ]; | |
| if (str_starts_with($transport['dsn'],'sync://')) { | |
| $tags['is_consumable'] =false; | |
| } | |
| $transportDefinition = (newDefinition(TransportInterface::class)) | |
| ->setFactory([newReference('messenger.transport_factory'),'createTransport']) | |
| ->setArguments([$transport['dsn'],$transport['options'] + ['transport_name' =>$name],newReference($serializerId)]) | |
| ->addTag('messenger.receiver',$tags) | |
| ; | |
| $container->setDefinition($transportId ='messenger.transport.'.$name,$transportDefinition); | |
| $senderAliases[$name] =$transportId; | |
| if (null !==$transport['retry_strategy']['service']) { | |
| $transportRetryReferences[$name] =newReference($transport['retry_strategy']['service']); | |
| }else { | |
| $retryServiceId =\sprintf('messenger.retry.multiplier_retry_strategy.%s',$name); | |
| $retryDefinition =newChildDefinition('messenger.retry.abstract_multiplier_retry_strategy'); | |
| $retryDefinition | |
| ->replaceArgument(0,$transport['retry_strategy']['max_retries']) | |
| ->replaceArgument(1,$transport['retry_strategy']['delay']) | |
| ->replaceArgument(2,$transport['retry_strategy']['multiplier']) | |
| ->replaceArgument(3,$transport['retry_strategy']['max_delay']) | |
| ->replaceArgument(4,$transport['retry_strategy']['jitter']); | |
| $container->setDefinition($retryServiceId,$retryDefinition); | |
| $transportRetryReferences[$name] =newReference($retryServiceId); | |
| } | |
| if ($transport['rate_limiter']) { | |
| if (!interface_exists(LimiterInterface::class)) { | |
| thrownewLogicException('Rate limiter cannot be used within Messenger as the RateLimiter component is not installed. Try running "composer require symfony/rate-limiter".'); | |
| } | |
| $transportRateLimiterReferences[$name] =newReference('limiter.'.$transport['rate_limiter']); | |
| } | |
| } |
Should this not employ the same logic as therate_limiter config formessenger for consistency?
| { | ||
| $this->rateLimiterFactory = $rateLimiterFactory; | ||
| return $this; |
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.
I don't think we should return$this here. It makes the API less clear whether this is a method mutating the instance or returning a new one. Our setters generally don't return$this in Symfony.
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.
The setters inSymfony\Component\Mailer\Transport\AbstractTransport all return$this - so I think we should also return$this here, otherwise it would be inconsistent, would it not?
Uh oh!
There was an error while loading.Please reload this page.
Assume you have to use multiple SMTP servers in your application in order to be able to send emails on behalf of specific email accounts on a variety of hosting providers. Some of these hosting providers may impose limits on how many emails you can send per timeframe. For example on Hetzner the limit is500 emails per hour, on webgo50 emails per hour. Hetzner does not force this limit - but they may disable your account completely, if you break this rule. The same applies to services like sendgrid etc., where you will have certain limits.
Now, if you use
symfony/messengerin order to send your emails asynchronously, you are able to rate limit the messenger transport:However, this will only rate limit the email queue globally - and not per SMTP server. So if you have to use multiple SMTP servers, where some use different rate limits and others do not need to be rate limited at all, this is not ideal.
Thus this PR introduces support for rate limited mailer transports. The changes are fairly straight forward:
rate_limiter, just as with messenger transports.Transportfactory, if applicable.AbstractTransport::send()method - usingensureAccepted(), so that aRateLimitExceededExceptionwill be thrown.available_atset to the respective time in the future according to the rate limit.Tests are still missing, as I wanted to hear comments first :)