-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make "mailer" service optional #2801
Conversation
* @param \Twig_Environment $twig | ||
* @param array $parameters | ||
*/ | ||
public function __construct($mailer, UrlGeneratorInterface $router, \Twig_Environment $twig, array $parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$mailer Should be typed as \Swift_Mailer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it should not =) With a type hint it would not be possible to pass null
value. Old TwigSwiftMailer
had that type hint. I could not remove it from there because it would be BC break. So - i created a new class TwigMailer
, a parent for old TwigSwiftMailer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, what's about
if(null !== $mailer && !($mailer instanceof \Swift_Mailer)
{
// throw ....
}
?
Currently you can pass anything as $mailer
... :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out that there are more then one mailer! I found this out in @stof's comment last year in #2562 (comment) =) That is a reason why symfony/swiftmailer-bundle
is not required dependency, btw. So, it is not necessary, i guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there is no another mailer which uses the Swift_Message class (which comes from the swiftmailer package)
https://github.com/covex-nn/FOSUserBundle/blob/b3000d0088536a1384b7057aa0888a9caa42a45e/Mailer/TwigMailer.php#L115
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devtronic actualy, i agree with you, and i am on your side! May be symfony/swiftmailer-bundle
should be in require
section of composer.json! Create a PR and i will be very glad if it will be accepted, then i will close this one =)
But, Swift_Message
comes from swiftmailer/swiftmailer
and it is not a bundle. So, it is possible to create a bundle, that provides mailer
service and uses Swift_Message
as a first argument of send
method at the same time.
A travis job with |
With this PR the
mailer
service will not be required any more while compiling Container. Both mailers will throw exceptions on sending email ifmailer
service was not available.Also a compiler pass
CheckForMailerPass
, introduced in #2639 was deprecated, and i created a new classTwigMailer
without type hint in first argument of constructor. OldTwigSwiftMailer
was deprecated, i moved all properties and methods to a new class to avoid BC breaks.This PR with #2708 will make creating a recipe for Symfony Flex easier