8000 bug #54572 [Mailer] Fix sendmail transport failure handling and inter… · symfony/symfony@f7de3e1 · GitHub
[go: up one dir, main page]
Skip to content

Commit f7de3e1

Browse files
fabpotfabpot
committed
bug #54572 [Mailer] Fix sendmail transport failure handling and interactive mode (bobvandevijver)
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Mailer] Fix sendmail transport failure handling and interactive mode | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | Fix #54532 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT #54239 introduced an issue for us when using sendmail in interactive mode using a long running background worker. It will throw exceptions due to an unclean shutdown in case of an SMTP timeout (5 minute by default), while in interactive mode all output is actually handled by the SMTP transport, including that timeout. That makes the exit code of the long running process not relevant in that mode. See the bug report for some more details. I have verified that this change solves my production issues, although I am not particularly fond on the hoops I had to jump to show this in the test. cc `@aboks` Commits ------- 1b2ead3 [Mailer] Fix sendmail transport failure handling and interactive mode
2 parents 8d08a73 + 1b2ead3 commit f7de3e1

File tree

4 files changed

+106
-21
lines changed

4 files changed

+106
-21
lines changed

‎src/Symfony/Component/Mailer/Tests/Transport/Fixtures/fake-failing-sendmail.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
#!/usr/bin/env php
22
<?php
3+
$argsPath = sys_get_temp_dir().\DIRECTORY_SEPARATOR.'sendmail_args';
4+
5+
file_put_contents($argsPath, implode(' ', $argv));
6+
37
print "Sending failed";
48
exit(42);

‎src/Symfony/Component/Mailer/Tests/Transport/SendmailTransportTest.php‎

Lines changed: 89 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,21 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Mailer\DelayedEnvelope;
16+
use Symfony\Component\Mailer\Envelope;
1617
use Symfony\Component\Mailer\Exception\TransportException;
18+
use Symfony\Component\Mailer\SentMessage;
1719
use Symfony\Component\Mailer\Transport\SendmailTransport;
20+
use Symfony\Component\Mailer\Transport\Smtp\Stream\ProcessStream;
21+
use Symfony\Component\Mailer\Transport\TransportInterface;
1822
use Symfony\Component\Mime\Address;
1923
use Symfony\Component\Mime\Email;
24+
use Symfony\Component\Mime\RawMessage;
2025

2126
class SendmailTransportTest extends TestCase
2227
{
2328
private const FAKE_SENDMAIL = __DIR__.'/Fixtures/fake-sendmail.php -t';
2429
private const FAKE_FAILING_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -t';
30+
private const FAKE_INTERACTIVE_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -bs';
2531

2632
/**
2733
* @var string
@@ -49,9 +55,7 @@ public function testToString()
4955

5056
public function testToIsUsedWhenRecipientsAreNotSet()
5157
{
52-
if ('\\' === \DIRECTORY_SEPARATOR) {
53-
$this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
54-
}
58+
$this->skipOnWindows();
5559

5660
$mail = new Email();
5761
$mail
@@ -71,20 +75,9 @@ public function testToIsUsedWhenRecipientsAreNotSet()
7175

7276
public function testRecipientsAreUsedWhenSet()
7377
{
74-
if ('\\' === \DIRECTORY_SEPARATOR) {
75-
$this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
76-
}
78+
$this->skipOnWindows();
7779

78-
$mail = new Email();
79-
$mail
80-
->from('from@mail.com')
81-
->to('to@mail.com')
82-
->subject('Subject')
83-
->text('Some text')
84-
;
85-
86-
$envelope = new DelayedEnvelope($mail);
87-
$envelope->setRecipients([new Address('recipient@mail.com')]);
80+
[$mail, $envelope] = $this->defaultMailAndEnvelope();
8881

8982
$sendmailTransport = new SendmailTransport(self::FAKE_SENDMAIL);
9083
$sendmailTransport->send($mail, $envelope);
@@ -93,11 +86,90 @@ public function testRecipientsAreUsedWhenSet()
9386
}
9487

9588
public function testThrowsTransportExceptionOnFailure()
89+
{
90+
$this->skipOnWindows();
91+
92+
[$mail, $envelope] = $this->defaultMailAndEnvelope();
93+
94+
$sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
95+
$this->expectException(TransportException::class);
96+
$this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
97+
$sendmailTransport->send($mail, $envelope);
98+
99+
$streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
100+
$streamProperty->setAccessible(true);
101+
$stream = $streamProperty->getValue($sendmailTransport);
102+
$this->assertNull($stream->stream);
103+
}
104+
105+
public function testStreamIsClearedOnFailure()
106+
{
107+
$this->skipOnWindows();
108+
109+
[$mail, $envelope] = $this->defaultMailAndEnvelope();
110+
111+
$sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
112+
try {
113+
$sendmailTransport->send($mail, $envelope);
114+
} catch (TransportException $e) {
115+
}
116+
117+
$streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
118+
$streamProperty->setAccessible(true);
119+
$stream = $streamProperty->getValue($sendmailTransport);
120+
$innerStreamProperty = new \ReflectionProperty(ProcessStream::class, 'stream');
121+
$innerStreamProperty->setAccessible(true);
122+
$this->assertNull($innerStreamProperty->getValue($stream));
123+
}
124+
125+
public function testDoesNotThrowWhenInteractive()
126+
{
127+
$this->skipOnWindows();
128+
129+
[$mail, $envelope] = $this->defaultMailAndEnvelope();
130+
131+
$sendmailTransport = new SendmailTransport(self::FAKE_INTERACTIVE_SENDMAIL);
132+
$transportProperty = new \ReflectionProperty(SendmailTransport::class, 'transport');
133+
$transportProperty->setAccessible(true);
134+
135+
// Replace the transport with an anonymous consumer that trigger the stream methods
136+
$transportProperty->setValue($sendmailTransport, new class($transportProperty->getValue($sendmailTransport)->getStream()) implements TransportInterface {
137+
private $stream;
138+
139+
public function __construct(ProcessStream $stream)
140+
{
141+
$this->stream = $stream;
142+
}
143+
144+
public function send(RawMessage $message, ?Envelope $envelope = null): ?SentMessage
145+
{
146+
$this->stream->initialize();
147+
$this->stream->write('SMTP');
148+
$this->stream->terminate();
149+
150+
return new SentMessage($message, $envelope);
151+
}
152+
153+
public function __toString(): string
154+
{
155+
return 'Interactive mode test';
156+
}
157+
});
158+
159+
$sendmailTransport->send($mail, $envelope);
160+
161+
$this->assertStringEqualsFile($this->argsPath, __DIR__.'/Fixtures/fake-failing-sendmail.php -bs');
162+
}
163+
164+
private function skipOnWindows()
96165
{
97166
if ('\\' === \DIRECTORY_SEPARATOR) {
98167
$this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
99168
}
169+
}
100170

171+
private function defaultMailAndEnvelope(): array
172+
{
101173
$mail = new Email();
102174
$mail
103175
->from('from@mail.com')
@@ -109,9 +181,6 @@ public function testThrowsTransportExceptionOnFailure()
109181
$envelope = new DelayedEnvelope($mail);
110182
$envelope->setRecipients([new Address('recipient@mail.com')]);
111183

112-
$sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
113-
$this->expectEx 6DB6 ception(TransportException::class);
114-
$this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
115-
$sendmailTransport->send($mail, $envelope);
184+
return [$mail, $envelope];
116185
}
117186
}

‎src/Symfony/Component/Mailer/Transport/SendmailTransport.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ public function __construct(?string $command = null, ?EventDispatcherInterface $
6464
$this->stream = new ProcessStream();
6565
if (str_contains($this->command, ' -bs')) {
6666
$this->stream->setCommand($this->command);
67+
$this->stream->setInteractive(true);
6768
$this->transport = new SmtpTransport($this->stream, $dispatcher, $logger);
6869
}
6970
}

‎src/Symfony/Component/Mailer/Transport/Smtp/Stream/ProcessStream.php‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,18 @@ final class ProcessStream extends AbstractStream
2525
{
2626
private $command;
2727

28+
private $interactive = false;
29+
2830
public function setCommand(string $command)
2931
{
3032
$this->command = $command;
3133
}
3234

35+
public function setInteractive(bool $interactive)
36+
{
37+
$this->interactive = $interactive;
38+
}
39+
3340
public function initialize(): void
3441
{
3542
$descriptorSpec = [
@@ -57,11 +64,15 @@ public function terminate(): void
5764
$err = stream_get_contents($this->err);
5865
fclose($this->err);
5966
if (0 !== $exitCode = proc_close($this->stream)) {
60-
throw new TransportException('Process failed with exit code '.$exitCode.': '.$out.$err);
67+
$errorMessage = 'Process failed with exit code '.$exitCode.': '.$out.$err;
6168
}
6269
}
6370

6471
parent::terminate();
72+
73+
if (!$this->interactive && isset($errorMessage)) {
74+
throw new TransportException($errorMessage);
75+
}
6576
}
6677

6778
protected function getReadConnectionDescription(): string

0 commit comments

Comments
 (0)
0