allow using env variables in transport configuration #159

mykiwi · 2017-02-16T15:49:47Z

Example:

# MAILER_URL=transport://user:pass@host:port/?encryption=...&auth_mode=...
swiftmailer:
    url: "%env(MAILER_URL)%"

To do:

add the doc [SwiftMailer] Update future reference symfony-docs#7510
add tests

nicolas-grekas · 2017-02-17T09:36:44Z

DependencyInjection/Configuration.php

+            // Deprecate url
+            ->beforeNormalization()
+                ->ifTrue(function ($v) {
+                    return is_array($v) && array_key_exists('url', $v);


&& !array_key_exists('dsn', $v)?

I will keep this condition to keep the trigger_error anyway, but check if dsn is already set.

why renaming the config key to expose the same feature ?

nicolas-grekas · 2017-02-17T09:38:39Z

DependencyInjection/SwiftmailerExtension.php

@@ -11,6 +11,7 @@

 namespace Symfony\Bundle\SwiftmailerBundle\DependencyInjection;

+use Symfony\Component\DependencyInjection\Definition;


should be moved below, alpha order in the "DependencyInjection" group

nicolas-grekas · 2017-02-17T09:40:36Z

DependencyInjection/SwiftmailerTransportFactory.php

+     *
+     * @return string transport
+     */
+    public static function resolveOptions(array &$options)


instead of passing this by reference, I'd make the function return $options, with $options['transport'] always set

nicolas-grekas · 2017-02-17T09:41:49Z

DependencyInjection/SwiftmailerTransportFactory.php

+     */
+    public static function resolveOptions(array &$options)
+    {
+        if (null === $options['transport']) {


!isset($options['transport']) now that the code is "public"

nicolas-grekas · 2017-02-17T09:42:50Z

DependencyInjection/SwiftmailerTransportFactory.php

+
+        if (isset($options['dsn'])) {
+            $parts = parse_url($options['dsn']);
+            if (!empty($parts['scheme'])) {


I'd use isset everywhere

nicolas-grekas · 2017-02-17T09:55:16Z

DependencyInjection/SwiftmailerExtension.php

+            ->setDefinition(sprintf('swiftmailer.mailer.%s.transport.eventdispatcher', $name), $definitionDecorator)
+        ;
+
+        $options = [];


not need for this line

nicolas-grekas · 2017-02-17T09:55:29Z

DependencyInjection/SwiftmailerExtension.php

+        ;
+
+        $options = [];
+        $container->resolveEnvPlaceholders($mailer, null, $options);


s/$options/$usedEnvs/

nicolas-grekas · 2017-02-17T09:56:29Z

DependencyInjection/SwiftmailerExtension.php

+        $options = [];
+        $container->resolveEnvPlaceholders($mailer, null, $options);
+
+        if ($options && isset($mailer['dsn'])) {


no need for the && isset($mailer['dsn']) part, isn't it?

nicolas-grekas · 2017-02-17T09:59:05Z

DependencyInjection/SwiftmailerExtension.php

+                ->replaceArgument(0, new Reference(sprintf('swiftmailer.mailer.%s.transport', $name)))
+            ;
+
+            $enable = !(isset($mailer['disable_delivery']) && $mailer['disable_delivery']);


isset(foo) && foo is the same as !empty(foo)

nicolas-grekas · 2017-02-17T09:59:41Z

DependencyInjection/SwiftmailerExtension.php

-                if (!empty($query['auth_mode'])) {
-                    $mailer['auth_mode'] = $query['auth_mode'];
-                }
+            if (isset($mailer['disable_delivery']) && $mailer['disable_delivery']) {


nicolas-grekas · 2017-02-17T21:49:40Z

LGTM. We should just make this work on 5.3 since that's the minimum version.

nicolas-grekas

👍

fabpot · 2017-02-23T15:49:17Z

Thank you @mykiwi.

…kiwi) This PR was merged into the 2.5-dev branch. Discussion ---------- allow using env variables in transport configuration Example: ```yml # MAILER_URL=transport://user:pass@host:port/?encryption=...&auth_mode=... swiftmailer: url: "%env(MAILER_URL)%" ``` To do: - [x] add the doc symfony/symfony-docs/pull/7510 - [x] add tests Commits ------- 183b291 allow using env variables in transport configuration

fabpot · 2017-02-28T23:22:24Z

While testing this PR, I noticed that 2 configurations cannot be used as env vars: encryption and auth_mode because there are using a validator, which is obviously not compatible with a dynamic value as provided by env vars.

danrot · 2017-03-01T09:20:34Z

This breaks some tests for me... We have tests checking if a mail was sent, which is done as described in http://symfony.com/doc/current/email/testing.html.

We've also added a configuration to the config:

swiftmailer:
    disable_delivery: true

But somehow this seems not to be considered anymore. Swiftmailer throws an error at https://github.com/swiftmailer/swiftmailer/blob/c5bd821/lib/classes/Swift/Transport/StreamBuffer.php#L268

I guess this is connected to this PR, because it is working with version 2.4.2 of this bundle, but not anymore with 2.5. Any configuration I am missing now?

mykiwi · 2017-03-01T12:15:27Z

@fabpot ok, so should we just not manage encryption and auth_mode options (and the gmail transport) from env variable or is it more complex?

@danrot do you use an env variable in your swiftmailer config? If not, I doubt that this PR is the cause of your problem.

danrot · 2017-03-01T12:31:52Z

@mykiwi It has to be connected somehow... This PR is the only being merged in 2.5.0.

fabpot · 2017-03-01T13:49:12Z

@mykiwi We have another report for the exact same issue, so it is definitely a problem related to the changes done here. Can you investigate?

fabpot · 2017-03-01T13:49:52Z

Regarding encryption and auth_mode, they cannot be managed (as of today) as env var, but I would let them as is in case it becomes possible in the future.

danrot · 2017-03-02T07:13:57Z

It works for me again when I set the configuration as follows:

swiftmailer:
    disable_delivery: true
    transport: "null"

Mind that transport: null is not working. So I guess the issue is somehow related to this line. But I don't have time to further investigate it...

…luz) This PR was merged into the master branch. Discussion ---------- [SwiftMailer] Update future reference - [x] Require symfony/swiftmailer-bundle#159 Commits ------- 6aa0117 Removed "encryption" and "auth_mode" from the list of %env()% compatible options 76ce6af Fixed ... again ... the versionadded directives ef7536c Added the missing "versionadded" directives 635a0ab add mail transport as deprecated 54ad41c Minor tweaks 287b71f update swiftmailer reference

mykiwi mentioned this pull request Feb 17, 2017

[SwiftMailer] Update future reference symfony/symfony-docs#7510

Merged

1 task

mykiwi force-pushed the env-configuration branch 2 times, most recently from 1a70e60 to 69a961d Compare February 17, 2017 09:34

mykiwi changed the title ~~[WIP] Use environment variable~~ [RFR] Use environment variable Feb 17, 2017

nicolas-grekas reviewed Feb 17, 2017

View reviewed changes

mykiwi force-pushed the env-configuration branch 3 times, most recently from 19ebd3a to b39da7a Compare February 17, 2017 17:28

mykiwi changed the title ~~[RFR] Use environment variable~~ allow using env variables in transport configuration Feb 17, 2017

mykiwi force-pushed the env-configuration branch 16 times, most recently from 73d2ec3 to e1975b5 Compare February 23, 2017 12:23

allow using env variables in transport configuration

183b291

mykiwi force-pushed the env-configuration branch from 0bb8f42 to 183b291 Compare February 23, 2017 12:49

nicolas-grekas approved these changes Feb 23, 2017

View reviewed changes

fabpot merged commit 183b291 into symfony:master Feb 23, 2017

danrot mentioned this pull request Mar 2, 2017

fixed breaking change in swiftmailer for tests sulu/sulu#3244

Merged

teohhanhui mentioned this pull request Jun 1, 2018

Add Docker setup Sylius/Sylius-Standard#232

Closed

1 task

alanhartless mentioned this pull request Feb 3, 2020

Environment variables with custom transports registered as services are not supported #298

Closed

		@@ -11,6 +11,7 @@

		namespace Symfony\Bundle\SwiftmailerBundle\DependencyInjection;

		use Symfony\Component\DependencyInjection\Definition;

Uh oh!

allow using env variables in transport configuration #159

allow using env variables in transport configuration #159

Uh oh!

Conversation

mykiwi commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Feb 17, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commented Feb 23, 2017

Uh oh!

fabpot commented Feb 28, 2017

Uh oh!

danrot commented Mar 1, 2017

Uh oh!

mykiwi commented Mar 1, 2017

Uh oh!

danrot commented Mar 1, 2017

Uh oh!

fabpot commented Mar 1, 2017

Uh oh!

fabpot commented Mar 1, 2017

Uh oh!

danrot commented Mar 2, 2017

Uh oh!

Uh oh!

mykiwi commented Feb 16, 2017 •

edited

Loading