[Mailer] Fix sendmail transport failure handling and interactive mode

Author: bobvandevijver

Tests/Transport/Fixtures/fake-failing-sendmail.php

@@ -1,4 +1,8 @@
 #!/usr/bin/env php
 <?php
+$argsPath = sys_get_temp_dir().\DIRECTORY_SEPARATOR.'sendmail_args';
+
+file_put_contents($argsPath, implode(' ', $argv));
+
 print "Sending failed";
 exit(42);

Tests/Transport/SendmailTransportTest.php

@@ -13,15 +13,21 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Mailer\DelayedEnvelope;
+use Symfony\Component\Mailer\Envelope;
 use Symfony\Component\Mailer\Exception\TransportException;
+use Symfony\Component\Mailer\SentMessage;
 use Symfony\Component\Mailer\Transport\SendmailTransport;
+use Symfony\Component\Mailer\Transport\Smtp\Stream\ProcessStream;
+use Symfony\Component\Mailer\Transport\TransportInterface;
 use Symfony\Component\Mime\Address;
 use Symfony\Component\Mime\Email;
+use Symfony\Component\Mime\RawMessage;
 
 class SendmailTransportTest extends TestCase
 {
     private const FAKE_SENDMAIL = __DIR__.'/Fixtures/fake-sendmail.php -t';
     private const FAKE_FAILING_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -t';
+    private const FAKE_INTERACTIVE_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -bs';
 
     /**
      * @var string
@@ -49,9 +55,7 @@ public function testToString()
 
     public function testToIsUsedWhenRecipientsAreNotSet()
     {
-        if ('\\' === \DIRECTORY_SEPARATOR) {
-            $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
-        }
+        $this->skipOnWindows();
 
         $mail = new Email();
         $mail
@@ -71,20 +75,9 @@ public function testToIsUsedWhenRecipientsAreNotSet()
 
     public function testRecipientsAreUsedWhenSet()
     {
-        if ('\\' === \DIRECTORY_SEPARATOR) {
-            $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
-        }
+        $this->skipOnWindows();
 
-        $mail = new Email();
-        $mail
-            ->from('from@mail.com')
-            ->to('to@mail.com')
-            ->subject('Subject')
-            ->text('Some text')
-        ;
-
-        $envelope = new DelayedEnvelope($mail);
-        $envelope->setRecipients([new Address('recipient@mail.com')]);
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
 
         $sendmailTransport = new SendmailTransport(self::FAKE_SENDMAIL);
         $sendmailTransport->send($mail, $envelope);
@@ -93,11 +86,90 @@ public function testRecipientsAreUsedWhenSet()
     }
 
     public function testThrowsTransportExceptionOnFailure()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
+        $this->expectException(TransportException::class);
+        $this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
+        $sendmailTransport->send($mail, $envelope);
+
+        $streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
+        $streamProperty->setAccessible(true);
+        $stream = $streamProperty->getValue($sendmailTransport);
+        $this->assertNull($stream->stream);
+    }
+
+    public function testStreamIsClearedOnFailure()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
+        try {
+            $sendmailTransport->send($mail, $envelope);
+        } catch (TransportException $e) {
+        }
+
+        $streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
+        $streamProperty->setAccessible(true);
+        $stream = $streamProperty->getValue($sendmailTransport);
+        $innerStreamProperty = new \ReflectionProperty(ProcessStream::class, 'stream');
+        $innerStreamProperty->setAccessible(true);
+        $this->assertNull($innerStreamProperty->getValue($stream));
+    }
+
+    public function testDoesNotThrowWhenInteractive()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_INTERACTIVE_SENDMAIL);
+        $transportProperty = new \ReflectionProperty(SendmailTransport::class, 'transport');
+        $transportProperty->setAccessible(true);
+
+        // Replace the transport with an anonymous consumer that trigger the stream methods
+        $transportProperty->setValue($sendmailTransport, new class($transportProperty->getValue($sendmailTransport)->getStream()) implements TransportInterface {
+            private $stream;
+
+            public function __construct(ProcessStream $stream)
+            {
+                $this->stream = $stream;
+            }
+
+            public function send(RawMessage $message, ?Envelope $envelope = null): ?SentMessage
+            {
+                $this->stream->initialize();
+                $this->stream->write('SMTP');
+                $this->stream->terminate();
+
+                return new SentMessage($message, $envelope);
+            }
+
+            public function __toString(): string
+            {
+                return 'Interactive mode test';
+            }
+        });
+
+        $sendmailTransport->send($mail, $envelope);
+
+        $this->assertStringEqualsFile($this->argsPath, __DIR__.'/Fixtures/fake-failing-sendmail.php -bs');
+    }
+
+    private function skipOnWindows()
     {
         if ('\\' === \DIRECTORY_SEPARATOR) {
             $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
         }
+    }
 
+    private function defaultMailAndEnvelope(): array
+    {
         $mail = new Email();
         $mail
             ->from('from@mail.com')
@@ -109,9 +181,6 @@ public function testThrowsTransportExceptionOnFailure()
         $envelope = new DelayedEnvelope($mail);
         $envelope->setRecipients([new Address('recipient@mail.com')]);
 
-        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
-        $this->expectException(TransportException::class);
-        $this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
-        $sendmailTransport->send($mail, $envelope);
+        return [$mail, $envelope];
     }
 }

Transport/SendmailTransport.php

@@ -64,6 +64,7 @@ public function __construct(?string $command = null, ?EventDispatcherInterface $
         $this->stream = new ProcessStream();
         if (str_contains($this->command, ' -bs')) {
             $this->stream->setCommand($this->command);
+            $this->stream->setInteractive(true);
             $this->transport = new SmtpTransport($this->stream, $dispatcher, $logger);
         }
     }

Transport/Smtp/Stream/ProcessStream.php

@@ -25,11 +25,18 @@ final class ProcessStream extends AbstractStream
 {
     private $command;
 
+    private $interactive = false;
+
     public function setCommand(string $command)
     {
         $this->command = $command;
     }
 
+    public function setInteractive(bool $interactive)
+    {
+        $this->interactive = $interactive;
+    }
+
     public function initialize(): void
     {
         $descriptorSpec = [
@@ -57,11 +64,15 @@ public function terminate(): void
             $err = stream_get_contents($this->err);
             fclose($this->err);
             if (0 !== $exitCode = proc_close($this->stream)) {
-                throw new TransportException('Process failed with exit code '.$exitCode.': '.$out.$err);
+                $errorMessage = 'Process failed with exit code '.$exitCode.': '.$out.$err;
             }
         }
 
         parent::terminate();
+
+        if (!$this->interactive && isset($errorMessage)) {
+            throw new TransportException($errorMessage);
+        }
     }
 
     protected function getReadConnectionDescription(): string