[Mailer] SmtpTransport triggers a php deprecation if response code assertion fails (during unit test) #47402
Description
Symfony version(s) affected
Taken from #47161 following may be also affected, however not tested.
Description
Our scheduled nightly ci run automatically pulls max dependency version (based on version constraints) and pulled the recently released 6.1.4 of the symfony/mailer packages. This fails now one of our unit tests.
Note: This only occurs with PHP 8.1 (and PHP8.2)
1) TYPO3\CMS\Core\Tests\Unit\Mail\TransportFactoryTest::sendmailTransportCallsDispatchOfDispatcher
Exception::__construct(): Passing null to parameter #2 ($code) of type int is deprecated
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:315
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:254
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:192
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/AbstractTransport.php:68
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:136
/builds/typo3/CI/cms/vendor/symfony/mailer/Transport/SendmailTransport.php:74
/builds/typo3/CI/cms/typo3/sysext/core/Tests/Unit/Mail/TransportFactoryTest.php:322
/builds/typo3/CI/cms/vendor/phpunit/phpunit/phpunit:98
which is related to the change in method assertResponseCode in commit symfony/mailer@0fce8fa
The change has 2 "flaws":
- empty response is no longer handled seperatly leading to null as code and thus emitting a deprecation message
If response is for example empty the line
[$code] = sscanf($response, '%3d');returns null for $code, which cannot passed as int for the exception throw in L315.
See: https://3v4l.org/sUDpE
Maybe it's a unit test setup / mocking / prophesizing issue in our unit test, but I think ensuring that $code is a valid integer should be checked anyway or not ?
We have a unittest which tests sendmail transport calls the dispatch of the event dispatcher, expecting a exception of type TransportExceptionInterface to avoid failing the test. As it emits now a deprecation message, which is converted to an exception this now breaks the test on our side.
We may add something to silent the test on our side, but I wanted to report this so this deprecation can be avoided / handled.
Before the mentioned change an empty respone was handled on its own (without a code at all). This may be readded in some way.
if (!$response) {
throw new TransportException(
sprintf('Expected response code "%s" but got an empty response.', implode('/', $codes))
);
}it seems avoiding to pass the $code argument explicitly as null will avoid that deprecation too (like before).
- 2nd part of the if codition is superflous, as in that case the 1st one would be true already (see below for details)
Additionally, if $response is empty it will have null as $code, thus it will not be $valid, so the:
if (!$valid || !$response) { ... }condition does not make sense anyway ... sencond codition will never match because in that case !$valid will match first.
How to reproduce
See our unit test code:
protected function getSubject(&$eventDispatcher): TransportFactory
{
$eventDispatcher = $this->prophesize(EventDispatcherInterface::class);
$eventDispatcher->dispatch(Argument::any())->willReturn(Argument::any());
$logger = $this->prophesize(LoggerInterface::class);
$logManager = $this->prophesize(LogManagerInterface::class);
$logManager->getLogger(Argument::any())->willReturn($logger->reveal());
$logManager->getLogger()->willReturn($logger->reveal());
$transportFactory = new TransportFactory($eventDispatcher->reveal(), $logManager->reveal());
$transportFactory->setLogger($logger->reveal());
return $transportFactory;
}
public function sendmailTransportCallsDispatchOfDispatcher(): void
{
$mailSettings = [
'transport' => 'sendmail',
'transport_smtp_server' => 'localhost:25',
'transport_smtp_encrypt' => '',
'transport_smtp_username' => '',
'transport_smtp_password' => '',
'transport_smtp_restart_threshold' => 0,
'transport_smtp_restart_threshold_sleep' => 0,
'transport_smtp_ping_threshold' => 0,
'transport_smtp_stream_options' => [],
'transport_sendmail_command' => '',
'transport_mbox_file' => '',
'defaultMailFromAddress' => '',
'defaultMailFromName' => '',
];
$transport = $this->getSubject($eventDispatcher)->get($mailSettings);
$message = new MailMessage();
$message->setTo(['foo@bar.com'])
->text('foo')
->from('bar@foo.com')
;
try {
$transport->send($message);
} catch (TransportExceptionInterface $exception) {
// connection is not valid in tests, so we just catch the exception here.
}
}Possible Solution
- re-add a early empty response check and throw a dedicated message as early throw
private function assertResponseCode(string $response, array $codes): void
{
if (!$codes) {
throw new LogicException('You must set the expected response code.');
}
// *** avoid passing a $code argument (2nd argument) for creating
// *** the TransportException - or eventually hardcode pass 0 as 2nd argument
if ($response === '') {
throw new TransportException(sprintf('Expected response code "%s" but got an empty response.', implode('/', $codes)));
}
[$code] = sscanf($response, '%3d');
$valid = \in_array($code, $codes);
if (!$valid || !$response) {
$codeStr = $code ? sprintf('code "%s"', $code) : 'empty code';
$responseStr = $response ? sprintf(', with message "%s"', trim($response)) : '';
throw new TransportException(sprintf('Expected response code "%s" but got ', implode('/', $codes), $codeStr).$codeStr.$responseStr.'.', $code);
}
}or
- conditional throw with/without code the exception
private function assertResponseCode(string $response, array $codes): void
{
if (!$codes) {
throw new LogicException('You must set the expected response code.');
}
[$code] = sscanf($response, '%3d');
$valid = \in_array($code, $codes);
if (!$valid || !$response) {
$codeStr = $code ? sprintf('code "%s"', $code) : 'empty code';
$responseStr = $response ? sprintf(', with message "%s"', trim($response)) : '';
if ($code !== null) {
throw new TransportException(
sprintf(
'Expected response code "%s" but got ',
implode('/', $codes),
$codeStr
).$codeStr.$responseStr.'.',
$code
);
} else {
throw new TransportException(
sprintf(
'Expected response code "%s" but got ',
implode('/', $codes),
$codeStr
).$codeStr.$responseStr.'.'
);
}
}
}or
- typecast $code
private function assertResponseCode(string $response, array $codes): void
{
if (!$codes) {
throw new LogicException('You must set the expected response code.');
}
[$code] = sscanf($response, '%3d');
$valid = \in_array($code, $codes);
if (!$valid || !$response) {
$codeStr = $code ? sprintf('code "%s"', $code) : 'empty code';
$responseStr = $response ? sprintf(', with message "%s"', trim($response)) : '';
throw new TransportException(sprintf('Expected response code "%s" but got ', implode('/', $codes), $codeStr).$codeStr.$responseStr.'.', (int)$code);
}
}Additional Context
No response