Support setLocalDomain() and setStreamOptions() #141
Conversation
|
The PHP 7 build fails on Travis because phpunit/phpunit-mock-objects versions 3.2.4 and 3.2.5 are broken. I don't know how to upgrade this package on Travis. |
|
|
||
| public function configureLocalDomain(\Swift_Transport_AbstractSmtpTransport $transport) | ||
| { | ||
| if ($this->requestContext) { |
There was a problem hiding this comment.
If you explicitly set a localDomain, I expect it to have priority over anything else, no?
| @@ -14,6 +14,7 @@ | |||
| use Symfony\Component\HttpKernel\Bundle\Bundle; | |||
| use Symfony\Component\DependencyInjection\ContainerBuilder; | |||
| use Symfony\Bundle\SwiftmailerBundle\DependencyInjection\Compiler\RegisterPluginsPass; | |||
| use Symfony\Bundle\SwiftmailerBundle\DependencyInjection\Compiler\RegisterHostnamePass; | |||
| $container | ||
| ->setDefinition(sprintf('swiftmailer.configurator.transport.%s', $name), $configuratorDecorator) | ||
| ->setArguments(array( | ||
| '%swiftmailer.mailer.' . $name . '.transport.smtp.local_domain%', |
There was a problem hiding this comment.
Symfony CS does not use spaces around dots, and in such situations, we prefer to use sprintf (the same goes in different places in this PR).
| $this->requestContext = $requestContext; | ||
| } | ||
|
|
||
| public function configureLocalDomain(\Swift_Transport_AbstractSmtpTransport $transport) |
There was a problem hiding this comment.
What about renaming if to just configure, so that we can use this for other things in the future if needed?
There was a problem hiding this comment.
If the method name is Configurator::configure(), there is no obvious name to use if we some day want to configure other classes. So I have also changed the class name, so the method name becomes AbstractSmtpConfigurator::configure(). Ok?
| @@ -27,6 +27,7 @@ | |||
| <parameter key="swiftmailer.email_sender.listener.class">Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener</parameter> | |||
|
|
|||
| <parameter key="swiftmailer.data_collector.class">Symfony\Bundle\SwiftmailerBundle\DataCollector\MessageDataCollector</parameter> | |||
| <parameter key="swiftmailer.configurator.class">Symfony\Bundle\SwiftmailerBundle\DependencyInjection\Configurator</parameter> | |||
There was a problem hiding this comment.
We don't define class name as parameter anymore, so this one need to be inlined
|
👍 (when my 2 small comments are adressed) |
| /** | ||
| * Service configurator. | ||
| */ | ||
| class AbstractSmtpTransportConfigurator |
There was a problem hiding this comment.
Why is it called abstract? It should be removed as I would expect the class to be abstract, which it is not.
There was a problem hiding this comment.
Because it configures subclasses of Swift_Transport_AbstractSmtpTransport. But I get your point, so I have stripped the Abstract prefix.
|
|
||
| /** | ||
| * Service configurator. | ||
| */ |
| { | ||
| /** | ||
| * @var string | ||
| */ |
There was a problem hiding this comment.
Fixed. But why? The coding guidelines on http://symfony.com/doc/current/contributing/code/standards.html does not go into much detail about PHPdoc, but it does include an example of a @var used together with a private variable.
| /** | ||
| * @var string | ||
| */ | ||
| protected $localDomain; |
| /** | ||
| * @var RequestContext | ||
| */ | ||
| protected $requestContext; |
|
|
||
| /** | ||
| * @var RequestContext | ||
| */ |
| * Sets the local domain based on the current request context. | ||
| * | ||
| * @param string $localDomain Fallback value if there is no request context. | ||
| * @param RequestContext $requestContext |
| protected $requestContext; | ||
|
|
||
| /** | ||
| * Sets the local domain based on the current request context. |
There was a problem hiding this comment.
The comment is probably more appropriate for the configure method.
|
Thank you @c960657. |
* The `url` option was added in version 2.3.10 of the SwiftmailerBundle (see symfony/swiftmailer-bundle#118). * The `timeout` and `source_ip` options are part of SwiftmailerBundle since symfony/swiftmailer-bundle#14 which was part of the 2.1.0 release. * The `local_domain` option was first included in the 2.4.0 release (see symfony/swiftmailer-bundle#141).
This PR adds support for Swift_Transport_AbstractSmtpTransport::setLocalDomain() and Swift_Transport_EsmtpTransport::setStreamOptions().