feature #33494 [Mailer] Change DSN syntax (fabpot)

This PR was merged into the 4.4 branch.

Discussion
----------

[Mailer] Change DSN syntax

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | -

The current syntax for failover and roundrobin is confusing. `&&` and `||` do not really convey the right meaning. I realized that while working on a new transport that will send on more than one transport in parallel. `&&` would be a natural fit, but that's already taken.

So, this pull request changes the syntax to be more explicit.

Commits
-------

39dd213 [Mailer] Change the syntax for DSNs using failover or roundrobin

src/Symfony/Component/Mailer/CHANGELOG.md

@@ -4,6 +4,18 @@ CHANGELOG
 4.4.0
 -----
 
+ * [BC BREAK] changed the syntax for failover and roundrobin DSNs
+
+   Before:
+
+   dummy://a || dummy://b (for failover)
+   dummy://a && dummy://b (for roundrobin)
+
+   After:
+
+   failover(dummy://a dummy://b)
+   roundrobin(dummy://a dummy://b)
+
  * added support for multiple transports on a `Mailer` instance
  * [BC BREAK] removed the `auth_mode` DSN option (it is now always determined automatically)
  * STARTTLS cannot be enabled anymore (it is used automatically if TLS is disabled and the server supports STARTTLS)

src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php

@@ -36,7 +36,7 @@ public function testToString()
         $t2 = $this->createMock(TransportInterface::class);
         $t2->expects($this->once())->method('__toString')->willReturn('t2://local');
         $t = new FailoverTransport([$t1, $t2]);
-        $this->assertEquals('t1://local || t2://local', (string) $t);
+        $this->assertEquals('failover(t1://local t2://local)', (string) $t);
     }
 
     public function testSendFirstWork()

src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php

@@ -35,7 +35,7 @@ public function testToString()
         $t2 = $this->createMock(TransportInterface::class);
         $t2->expects($this->once())->method('__toString')->willReturn('t2://local');
         $t = new RoundRobinTransport([$t1, $t2]);
-        $this->assertEquals('t1://local && t2://local', (string) $t);
+        $this->assertEquals('roundrobin(t1://local t2://local)', (string) $t);
     }
 
     public function testSendAlternate()

src/Symfony/Component/Mailer/Tests/TransportTest.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Mailer\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Mailer\Exception\InvalidArgumentException;
 use Symfony\Component\Mailer\SentMessage;
 use Symfony\Component\Mailer\SmtpEnvelope;
 use Symfony\Component\Mailer\Transport;
@@ -44,14 +45,42 @@ public function fromStringProvider(): iterable
         ];
 
         yield 'failover transport' => [
-            'dummy://a || dummy://b',
+            'failover(dummy://a dummy://b)',
             new FailoverTransport([$transportA, $transportB]),
         ];
 
         yield 'round robin transport' => [
-            'dummy://a && dummy://b',
+            'roundrobin(dummy://a dummy://b)',
             new RoundRobinTransport([$transportA, $transportB]),
         ];
+
+        yield 'mixed transport' => [
+            'roundrobin(dummy://a failover(dummy://b dummy://a) dummy://b)',
+            new RoundRobinTransport([$transportA, new FailoverTransport([$transportB, $transportA]), $transportB]),
+        ];
+    }
+
+    /**
+     * @dataProvider fromWrongStringProvider
+     */
+    public function testFromWrongString(string $dsn, string $error): void
+    {
+        $transportFactory = new Transport([new DummyTransportFactory()]);
+
+        $this->expectExceptionMessage($error);
+        $this->expectException(InvalidArgumentException::class);
+        $transportFactory->fromString($dsn);
+    }
+
+    public function fromWrongStringProvider(): iterable
+    {
+        yield 'garbage at the end' => ['dummy://a some garbage here', 'The DSN has some garbage at the end:  some garbage here.'];
+
+        yield 'not a valid DSN' => ['something not a dsn', 'The "something" mailer DSN must contain a scheme.'];
+
+        yield 'failover not closed' => ['failover(dummy://a', 'The "(dummy://a" mailer DSN must contain a scheme.'];
+
+        yield 'not a valid keyword' => ['foobar(dummy://a)', 'The "foobar" keyword is not valid (valid ones are "failover", "roundrobin")'];
     }
 }
 

src/Symfony/Component/Mailer/Transport.php

@@ -18,6 +18,7 @@
 use Symfony\Component\Mailer\Bridge\Mailgun\Transport\MailgunTransportFactory;
 use Symfony\Component\Mailer\Bridge\Postmark\Transport\PostmarkTransportFactory;
 use Symfony\Component\Mailer\Bridge\Sendgrid\Transport\SendgridTransportFactory;
+use Symfony\Component\Mailer\Exception\InvalidArgumentException;
 use Symfony\Component\Mailer\Exception\UnsupportedHostException;
 use Symfony\Component\Mailer\Transport\Dsn;
 use Symfony\Component\Mailer\Transport\FailoverTransport;
@@ -82,17 +83,59 @@ public function fromStrings(array $dsns): Transports
 
     public function fromString(string $dsn): TransportInterface
     {
-        $dsns = preg_split('/\s++\|\|\s++/', $dsn);
-        if (\count($dsns) > 1) {
-            return new FailoverTransport($this->createFromDsns($dsns));
+        list($transport, $offset) = $this->parseDsn($dsn);
+        if ($offset !== \strlen($dsn)) {
+            throw new InvalidArgumentException(sprintf('The DSN has some garbage at the end: %s.', substr($dsn, $offset)));
         }
 
-        $dsns = preg_split('/\s++&&\s++/', $dsn);
-        if (\count($dsns) > 1) {
-            return new RoundRobinTransport($this->createFromDsns($dsns));
-        }
+        return $transport;
+    }
+
+    private function parseDsn(string $dsn, int $offset = 0): array
+    {
+        static $keywords = [
+            'failover' => FailoverTransport::class,
+            'roundrobin' => RoundRobinTransport::class,
+        ];
+
+        while (true) {
+            foreach ($keywords as $name => $class) {
+                $name .= '(';
+                if ($name === substr($dsn, $offset, \strlen($name))) {
+                    $offset += \strlen($name) - 1;
+                    preg_match('{\(([^()]|(?R))*\)}A', $dsn, $matches, 0, $offset);
+                    if (!isset($matches[0])) {
+                        continue;
+                    }
+
+                    ++$offset;
+                    $args = [];
+                    while (true) {
+                        list($arg, $offset) = $this->parseDsn($dsn, $offset);
+                        $args[] = $arg;
+                        if (\strlen($dsn) === $offset) {
+                            break;
+                        }
+                        ++$offset;
+                        if (')' === $dsn[$offset - 1]) {
+                            break;
+                        }
+                    }
+
+                    return [new $class($args), $offset];
+                }
+            }
+
+            if (preg_match('{(\w+)\(}A', $dsn, $matches, 0, $offset)) {
+                throw new InvalidArgumentException(sprintf('The "%s" keyword is not valid (valid ones are "%s"), ', $matches[1], implode('", "', array_keys($keywords))));
+            }
+
+            if ($pos = strcspn($dsn, ' )', $offset)) {
+                return [$this->fromDsnObject(Dsn::fromString(substr($dsn, $offset, $pos))), $offset + $pos];
+            }
 
-        return $this->fromDsnObject(Dsn::fromString($dsn));
+            return [$this->fromDsnObject(Dsn::fromString(substr($dsn, $offset))), \strlen($dsn)];
+        }
     }
 
     public function fromDsnObject(Dsn $dsn): TransportInterface
@@ -106,21 +149,6 @@ public function fromDsnObject(Dsn $dsn): TransportInterface
         throw new UnsupportedHostException($dsn);
     }
 
-    /**
-     * @param string[] $dsns
-     *
-     * @return TransportInterface[]
-     */
-    private function createFromDsns(array $dsns): array
-    {
-        $transports = [];
-        foreach ($dsns as $dsn) {
-            $transports[] = $this->fromDsnObject(Dsn::fromString($dsn));
-        }
-
-        return $transports;
-    }
-
     private static function getDefaultFactories(EventDispatcherInterface $dispatcher = null, HttpClientInterface $client = null, LoggerInterface $logger = null): iterable
     {
         foreach (self::FACTORY_CLASSES as $factoryClass) {

src/Symfony/Component/Mailer/Transport/FailoverTransport.php

@@ -31,6 +31,6 @@ protected function getNextTransport(): ?TransportInterface
 
     protected function getNameSymbol(): string
     {
-        return '||';
+        return 'failover';
     }
 }

src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php

@@ -58,9 +58,9 @@ public function send(RawMessage $message, SmtpEnvelope $envelope = null): ?SentM
 
     public function __toString(): string
     {
-        return implode(' '.$this->getNameSymbol().' ', array_map(function (TransportInterface $transport) {
+        return $this->getNameSymbol().'('.implode(' ', array_map(function (TransportInterface $transport) {
             return (string) $transport;
-        }, $this->transports));
+        }, $this->transports)).')';
     }
 
     /**
@@ -99,7 +99,7 @@ protected function isTransportDead(TransportInterface $transport): bool
 
     protected function getNameSymbol(): string
     {
-        return '&&';
+        return 'roundrobin';
     }
 
     private function moveCursor(int $cursor): int