Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mailer does not collect profiling #31592

Open
garak opened this issue May 23, 2019 · 19 comments

Comments

Projects
None yet
7 participants
@garak
Copy link
Contributor

commented May 23, 2019

I was not sure if opening this issue, since Mailer component is experimental and 4.3 version is still in beta. Anyway, may is about to end and it looks like an important feature is missing in Mailer. I'm not even sure if this is a bug or a new feature (I opted for last one, being a new component)

My expectation is to replace current combo of Swiftmailer library and Swifmailer bundle with new combo of Mailer and MIME components, and to get more or less the same functionalities.
So, for example, I expect to see some info in the profiler and in the web debug toolbar (you know, the old good envelope icon linked to info about headers, text, rendered message, etc.).

I couldn't find anything in current mailer, for example the NullTransport is doing absolutely nothing

@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

My expectation is to replace current combo of Swiftmailer library and Swifmailer bundle with new combo of Mailer and MIME components, and to get more or less the same functionalities.

:+1

I am also missing the delivery_addresses functionality and worked around it in userland. Otherwise I am pretty psyched about the new mailer. Perhaps a tracking issue for the Mailer would help exploring wishes ?

@tarlepp

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@Devristo I'm interested to see your solution for that delivery_addresses "issue", personally I'm using this on my services.yaml for that atm.

Symfony\Component\Mailer\EventListener\EnvelopeListener:
    tags: ['kernel.event_subscriber']
    arguments:
        $recipients: '%env(key:MAILER_RECIPIENTS:json:file:APPLICATION_CONFIG)%'
@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@tarlepp Your solution is much fancier. I basically have a function which returns a fixed mail address when a specific env is set. Something like $mail->to(generateAddressOrDeliverToFixed($from, $name)) basically.

@tarlepp

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@Devristo that is what I first tried too, but I didn't like that solution so much so I dig a bit deeper and found that solution - I hope that helps you too.

Although not sure if that is the "correct" way to do that, but it works.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 23, 2019

The integration in the bundle is not finished yet... and quite non-existent TBH. We need someone to do the work I suppose.

Regarding the profiler, it's not "easy" as most of the time, the email won't be sent right away, but async. So, not sure what we want to do.

Regarding delivery_addresses, @tarlepp discovered the right solution :) The EnvelopeListener class allows to change the envelope of the email. It's quite different from Swiftmailer as the email itself does not change. This is nice as you receive the exact message that would have been sent to the original recipient.

If someone wants to work on that, I would be more than happy to help on Slack or here or on a PR.

@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

If I understand correctly there should be some configuration / extension which configures the parameters of the EnvelopeListener to alter the from-address and the recipient addresses? Basically @tarlepp is almost there, except the Configuration / Extension part. However I wouldn't mind playing with it and creating a PR.

The profiler will be definitely more complicated due to the varying ways messages can be routed through the system. I think mails can even be sent without using the \Symfony\Component\Mailer\Mailer convenience class?

Perhaps we can collect emails by creating a separate always-synchronous MessageHandler which will just collect the envelope? In case Symfony Messenger is not used it should still be collected in the Mailer (TraceableMailer?) itself I guess?

Hmm above is flawed by the fact that the Envelope can change later when the MessageEvent is dispatched by the transport.

@tarlepp

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I think that for that delivery_addresses override, writing a documentation about how to use it would be enough - just because there is no really need for any extra code - only configuration.

@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

It could ease the migration from SwiftMailer considerably though if the functionality is used a lot. For us it has been a quite common use case to deliver mails in a catch-all kind of fashion on non-production servers.

@tarlepp

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

So seems like current documentation about that delivery_addresses is following:

# config/packages/dev/swiftmailer.yaml
swiftmailer:
    delivery_addresses: ['dev@example.com']

So basically that part could be just replaced with following:

# config/packages/dev/services.yaml
Symfony\Component\Mailer\EventListener\EnvelopeListener:
    arguments:
        $recipients: ['dev@example.com']

So I don't really see so much difference - just documentation stuff.

And also remember that Mailer is a new component, so it won't be the same as the old one - so most likely you need to make some changes to your code if you want to use new component.

@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

The service configuration makes it much more brittle though; as it is tied to the implementation (EnvelopeListener and its constructor's signature). The configuration option gives more flexibility, even when the EnvelopeListener is removed / renamed / refactored everything will still work.

But in the end it is not up to us ;). I am willing to dive a bit deeper into the profiling issue if someone could give me some pointers.

@fabpot

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I would definitely use a configuration for that. What about something like this:

mailer:
    envelope:
        recipients: ['dev@example.com']
        sender: 'from@example.com'
@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Would we want the delivery_whitelistfunction as well? Perhaps it would be good to check which features we want to keep and which we want to drop?

These are the options for the SwiftMailer

    antiflood
        sleep
        threshold
    auth_mode (covered by DSN)
    command (covered by DSN)
    delivery_addresses
    delivery_whitelist
    disable_delivery (covered by DSN)
    encryption (covered by DSN)
    host (covered by DSN)
    local_domain
    logging
    password (covered by DSN)
    port (covered by DSN)
    sender_address
    source_ip
    spool (think we are all glad this is gone?)
        path
        type
    timeout
    transport (covered by DSN)
    url (covered by DSN)
    username  (covered by DSN)
@fabpot

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I think we should make it possible to configure everything related to the Transport in the DSN (that makes it easier to have more than one mailer for instance).

For all transports:

  • max_per_second (calls setMaxPerSecond)

And for SMTP transports only:

  • restart_threshold (calls setRestartThreshold) -- we need two numbers here, the threshold and the sleep in seconds (separated by a comma?)
  • local_domain (calls setLocalDomain)

I would not make timeout and source_ip configurable for now (they are used for SMTP when a socket stream).

@Simperfit

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

I see multiple things to do:

  • Add configuration for some keys (like said in the issue)
  • Doing the profiler implementation

@Devristo do you want to work on one of these, maybe we can share the work ;) ?

@Devristo

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

I started on the configuration. Just struggling a bit with the tests. If you want to start on the profiler go ahead :). Hopefully I will manage to finish up the configuration this week :)

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@Devristo Do you think you can share your work with us? I'd like to move forward with this one. I can help if needed.

@ajgarlag

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@garak To view sent messages in profiler, while this feature is implemented for the new symfony/mailer component, you can use ajgl/swiftmailer-mailer

This package includes a transport that will use the default swift mailer instance as a transport for emails sent with the new symfony mailer service, so you can view them in swift mailer profiler panel.

DISCLAIMER: I'm the author of the package, and I'd like to see this feature implemented in the near future, so I can abandon it.

@Devristo

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@fabpot See Devristo@f023514

My apologies for not finishing it. Could use help with the test case. Would we want an functional test? I struggled to use the private mailer service from a functional test.

@Devristo

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

I managed to create some tests. Could someone check out my PR and see what else is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.