OpenBuildings / postmark

Postmark transport for Swift Mailer
https://github.com/OpenBuildings/postmark
BSD 3-Clause "New" or "Revised" License
16 stars 8 forks source link

Not working with FOSUserBundle registration #25

Closed romaricdrigon closed 9 years ago

romaricdrigon commented 9 years ago

Hi,

I'm encountering some issue to get Postmark to work with FOSUserBundle. For instance at the registration an email is sent. Using "basic" SMTP, no issues. But actually no way to get this working with Postmark: the transport used by SwiftMailer is still \Swift_Transport_SpoolTransport.

I'm not sure the issue lies really in this bundle. But I'm wandering if somebody else encountered some similar issues. Maybe SwiftMailer is not handling correctly the transport change?

hkdobrev commented 9 years ago

@romaricdrigon I haven't used Symfony very much and I'm not familiar with the FOSUserBundle at all. Do you think you could create a simple repo with composer.json for dependencies and reproducible example? Or at least point us where the bundle is sending the email and where it is setting the Postmark transport. Thanks!

romaricdrigon commented 9 years ago

Hi @hkdobrev,

Heads up for the quick answer! Mails in FOSUserBundle are sent from listeners on domain events; and eventually a "Mailer" is called. That one could be any of those: https://github.com/FriendsOfSymfony/FOSUserBundle/tree/master/Mailer

At the moment I'm using the TwigSwiftMailer, changing does not help. Those are actually a wrapper around SwiftMailer. I suspect there may be some issue with SwiftMailer being injected to those at compilation.

hkdobrev commented 9 years ago

@romaricdrigon Could you paste in the relevant mailer configuration from app/config/config.yml?

Do you define the Postmark service like stated in the readme?

Define Postmark Transport as service:

services:
    swift_transport.postmark:
        class: Openbuildings\Postmark\Swift_PostmarkTransport
        arguments:
            - POSTMARK_API_KEY

In config.yml change transport to defined service:

swiftmailer:
    transport: swift_transport.postmark

Do you create an instance of TwigSwiftMailer yourself?

From what I gather so far this is either a problem with the FOSUserBundle or from your usage of it.

romaricdrigon commented 9 years ago

Yep, exactly. Sending emails manually is fine. Every other place works fine, but the FOSUserBundle.

# my bundle services.yml
    postmark:
        class: Openbuildings\Postmark\Swift_PostmarkTransport
        arguments:
            - %postmark_api_key%
# app/config/config.yml
swiftmailer:
    transport: postmark
    spool:     { type: memory }
romaricdrigon commented 9 years ago

I've been working around. The bug does not seem to be FOSUser related. Some pretty severe exception/error is happening after response is returned, but before the kernel.terminate Symfony2 event used to send emails (flush spool). Nothing is logged and even the Symfony2 debug toolbar crash and records nothing. I've to find an appropriate debug tool and will give you more details.

romaricdrigon commented 9 years ago

Hi,

So the issue was pretty simple: I did not configured the "from" address yet, and Postmark was refusing my "example" address. What made this issue so hard is that because emails are sent on kernel.terminate in Symfony2, this exception is not logged and may not be displayed to user. Even worse it will crash the debug toolbar; so it won't appear in debug toolbar history.

I will close this issue as everything is all right with the Postmark transport. Though a suggestion: what about accepting the logger as an (optional) dependency, and then log errors returned by Postmark API? Even in production it would make a lot of sense. I'm ok on working on the PR, it would be pretty short.

romaricdrigon commented 9 years ago

@hkdobrev what about my proposal about the logger? As said, I'm ok implementing it though I prefer not to work for anything.

hkdobrev commented 9 years ago

@romaricdrigon The transport is throwing a Postmark\Exception here: https://github.com/OpenBuildings/postmark/blob/a51bb76b79164ef251c4dba48678f37df3ad3d86/src/Api.php#L122-L127. I think it'd be better to catch this exception outside the transport instead of actually coupling it with a logger.

P.S. You can use the exception code and the constants defined in the exception class to understand reliably what is the Postmark exception and whether to do something or not.

romaricdrigon commented 9 years ago

Because that transport is handling way more "information" that the standard one, such as Postmark responses, failures, or even network failures, I feel like logging should be considered and we are not in a case of "it should be handled somewhere else".

Secondly, it's also about how much you want the transport to be easy to use and feel like a "modern" library, logging is pretty considered a best practice now; whereas old Swiftmailer code skips it. When the transport eventually throws Postmark\Exception, Symfony2 exception handler is off ; and Swiftmailer just dies right away do nothing.

And I see 2 options:

    postmark:
        class: Openbuildings\Postmark\Swift_PostmarkTransport
        arguments:
            - %postmark_api_key%

Or with logger:

    postmark:
        class: Openbuildings\Postmark\Swift_PostmarkTransport
        arguments:
            - %postmark_api_key%
            - @logger

LoggerInterface interface is part of Psr, so no vendor lock-in here.

hkdobrev commented 9 years ago

@romaricdrigon Don't you think you can attach a SwiftMailer logger plugin for your needs? Take a look at https://github.com/OpenBuildings/html-email/blob/master/classes/Kohana/Email.php#L97-L104. This is a Kohana module which uses this transport and attaches a custom SwiftMailer logger and plugs in the Kohana logging system.

In conclusion:

Don't underestimate the value of modularity and loose coupling.

Cheers!

romaricdrigon commented 9 years ago

Definitely #22 would be good, at the moment I will use a Logger plugin :)

romaricdrigon commented 9 years ago

Hum, second thoughts, after having tried the Swiftmailer logging is really not helpful here.

So back to my initial point: Swiftmailer is crappy now, and really not really up to best-practices and modularity as you say. I feel like Swiftmailer is just unable to handle Postmark transport failures...

hkdobrev commented 9 years ago

@romaricdrigon

I think these Swiftmailer events should be enough:

The problem is that our transport does not set enough data. That doesn't mean we need to implement a custom logging solution. We just need to behave better in the Switfmailerland. If this was just a Postmark API wrapper, I'd be more than willing to add a PSR-3 logger.

As a matter of fact the recently released official Swiftmailer Postmark transport does not set anything as well: https://github.com/wildbit/swiftmailer-postmark/blob/master/src/Postmark/Transport.php