AccordGroup / MandrillSwiftMailerBundle

A Symfony bundle that provides a Swiftmailer transport service for Mandrill
GNU General Public License v2.0
20 stars 15 forks source link

[RFR] Add SwiftmailerBundle configuration bridge #24

Closed vincentchalamon closed 7 years ago

vincentchalamon commented 7 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23
License MIT

With this PR, it should be possible to configure accord_mandrill_swift_mailer through swiftmailer:

swiftmailer:
    # ...
    mandrill:
        transport: accord_mandrill
        api_key: %mandrill_api_key%
        async: %mandrill_async%
        subaccount: %mandrill_subaccount%
jrattue commented 7 years ago

Hi @vincentchalamon,

Thanks for your contribution.

Before we would be happy to merge this in could you change the name of the transport back to accord_mandrill as this will maintain backwards compatibility.

vincentchalamon commented 7 years ago

@Rattler3 fixed :-)

vincentchalamon commented 7 years ago

Ping @Rattler3

jrattue commented 7 years ago

@vincentchalamon Sorry for not getting back to this, I should be able to get this reviewed this week.

Kyoushu commented 7 years ago

@vincentchalamon could you explain a scenario where configuring the bundle directly would not be adequate, and how your PR solves the problem?

I'm not sure I'm seeing the benefit decorating another extension. Would this not also introduce problems if another bundle were to attempt to decorate the same extension?

vincentchalamon commented 7 years ago

@Kyoushu It's still possible to configure the bundle directly, but you'll have to declare 2 configurations: MandrillSwiftMailerBundle and SwiftmailerBundle. This PR allows to configure both at the same time, only if this last one is installed (otherwise, it'll be a normal configuration).

I'm not decorating SwiftmailerExtension, I'm extending it. As this extend checks for transport to be equal to accord_mandrill, it won't conflict with other bundles :-)

vincentchalamon commented 7 years ago

BTW @Rattler3 @Kyoushu: tests are green

capture d ecran 2017-07-04 a 15 13 10
Kyoushu commented 7 years ago

@vincentchalamon I'm afraid I still don't understand the reason for moving the configuration for this bundle into the config for a third party bundle. It only seems to duplicate functionality which exists in the current bundle config, and in a way that interferes with a third party bundle.

By wrapping a third party extensions, and re-registering it, you're decorating the original extension. The call to ContainerBuilder::registerExtension overwrites the original, as the internal array of extensions are keyed by alias.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L175

This could potentially result in fatal errors in the future if new public methods are introduced to \Symfony\Bundle\SwiftmailerBundle\DependencyInjection\SwiftmailerExtension. As a result I'm afraid I can't accept merge this request.

vincentchalamon commented 7 years ago

(…) and in a way that interferes with a third party bundle

I totally agree with your for an OpenSource library, I didn't think about that while writing this PR.

This PR is just an improvement (and a little cleaning) to allow this bundle configuration to be declared more easily. Feel free to close it if it doesn't apply to your bundle.

Kyoushu commented 7 years ago

@vincentchalamon ok, I'll close it for now, but I think there may be a way to achieve what you want to do in another way. I've I have any ideas I'll let you know.