Skip to content

Commit d38ef67

Browse files
nicolas-grekasnicolas-grekas
committed
bug #48126 [Mailer] Include all transports' debug messages in RoundRobin transport exception (mixdf)
This PR was merged into the 5.4 branch. Discussion ---------- [Mailer] Include all transports' debug messages in RoundRobin transport exception | Q | A | ------------- | --- | Branch?| 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | ... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | N/A ### Description The RoundRobin Mail transport does not include inner transports' debug messages in case of failure, which makes it very difficult to reason about the failures. This PR attempts to add that debug information to the thrown TransportException. In the process of fixing this issue, I discovered that one of the existing tests is incorrect. The last assertion is never executed and the test passes regardless of the assertion state. The test was also fixed. Note that although this issue exists in https://github.com/symfony/symfony/tree/5.0, I wasn't able to apply the same patch to it since the behavior of the transport (specifically the cursor property) was different. I can open a new PR to fix the same issue for that branch as well. <!-- Replace this notice by a short README for your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Commits ------- 2bfbd92 [Mailer] Include all transports' debug messages in RoundRobin transport exception
2 parents cc61ecb + 2bfbd92 commit d38ef67

File tree

2 files changed

+49
-5
lines changed

2 files changed

+49
-5
lines changed

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

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Mailer\Exception\TransportException;
16+
use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
1617
use Symfony\Component\Mailer\Transport\RoundRobinTransport;
1718
use Symfony\Component\Mailer\Transport\TransportInterface;
1819
use Symfony\Component\Mime\RawMessage;
@@ -60,10 +61,21 @@ public function testSendAllDead()
6061
$t2 = $this->createMock(TransportInterface::class);
6162
$t2->expects($this->once())->method('send')->will($this->throwException(new TransportException()));
6263
$t = new RoundRobinTransport([$t1, $t2]);
63-
$this->expectException(TransportException::class);
64-
$this->expectExceptionMessage('All transports failed.');
65-
$t->send(new RawMessage(''));
66-
$this->assertTransports($t, 1, [$t1, $t2]);
64+
$p = new \ReflectionProperty($t, 'cursor');
65+
$p->setAccessible(true);
66+
$p->setValue($t, 0);
67+
68+
try {
69+
$t->send(new RawMessage(''));
70+
} catch (\Exception $e) {
71+
$this->assertInstanceOf(TransportException::class, $e);
72+
$this->assertStringContainsString('All transports failed.', $e->getMessage());
73+
$this->assertTransports($t, 0, [$t1, $t2]);
74+
75+
return;
76+
}
77+
78+
$this->fail('The expected exception was not thrown.');
6779
}
6880

6981
public function testSendOneDead()
@@ -127,6 +139,34 @@ public function testSendOneDeadAndRecoveryWithinRetryPeriod()
127139
$this->assertTransports($t, 1, []);
128140
}
129141

142+
public function testFailureDebugInformation()
143+
{
144+
$t1 = $this->createMock(TransportInterface::class);
145+
$e1 = new TransportException();
146+
$e1->appendDebug('Debug message 1');
147+
$t1->expects($this->once())->method('send')->will($this->throwException($e1));
148+
$t1->expects($this->once())->method('__toString')->willReturn('t1');
149+
150+
$t2 = $this->createMock(TransportInterface::class);
151+
$e2 = new TransportException();
152+
$e2->appendDebug('Debug message 2');
153+
$t2->expects($this->once())->method('send')->will($this->throwException($e2));
154+
$t2->expects($this->once())->method('__toString')->willReturn('t2');
155+
156+
$t = new RoundRobinTransport([$t1, $t2]);
157+
158+
try {
159+
$t->send(new RawMessage(''));
160+
} catch (TransportExceptionInterface $e) {
161+
$this->assertStringContainsString('Transport "t1": Debug message 1', $e->getDebug());
162+
$this->assertStringContainsString('Transport "t2": Debug message 2', $e->getDebug());
163+
164+
return;
165+
}
166+
167+
$this->fail('Expected exception was not thrown!');
168+
}
169+
130170
private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
131171
{
132172
$p = new \ReflectionProperty($transport, 'cursor');

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,19 @@ public function __construct(array $transports, int $retryPeriod = 60)
4848

4949
public function send(RawMessage $message, Envelope $envelope = null): ?SentMessage
5050
{
51+
$exception = null;
52+
5153
while ($transport = $this->getNextTransport()) {
5254
try {
5355
return $transport->send($message, $envelope);
5456
} catch (TransportExceptionInterface $e) {
57+
$exception = $exception ?? new TransportException('All transports failed.');
58+
$exception->appendDebug(sprintf("Transport \"%s\": %s\n", $transport, $e->getDebug()));
5559
$this->deadTransports[$transport] = microtime(true);
5660
}
5761
}
5862

59-
throw new TransportException('All transports failed.');
63+
throw $exception ?? new TransportException('No transports found.');
6064
}
6165

6266
public function __toString(): string

0 commit comments

Comments
 (0)