From 4a1976b4efe13df964537079b378578622007083 Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Wed, 16 Dec 2020 20:36:06 +0100 Subject: [PATCH 1/6] [Notifier] Use assertSame() --- .../Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php b/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php index c7407ed06d..bed1d8e260 100644 --- a/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php +++ b/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php @@ -91,7 +91,7 @@ final class TelegramTransportTest extends TestCase ]; $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface { - $this->assertEquals($expectedBody, json_decode($options['body'], true)); + $this->assertSame($expectedBody, json_decode($options['body'], true)); return $response; }); @@ -120,7 +120,7 @@ final class TelegramTransportTest extends TestCase ]; $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface { - $this->assertEquals($expectedBody, json_decode($options['body'], true)); + $this->assertSame($expectedBody, json_decode($options['body'], true)); return $response; }); From e61363c1f26face6f53664636ec883dfa9731e4b Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Thu, 17 Dec 2020 11:43:47 +0100 Subject: [PATCH 2/6] [Notifier] Fix toString when optional parameter is not set --- .../Bridge/RocketChat/RocketChatTransport.php | 4 ++++ .../RocketChat/Tests/RocketChatTransportTest.php | 11 +++++++++-- .../Notifier/Bridge/Telegram/TelegramTransport.php | 4 ++++ .../Bridge/Telegram/Tests/TelegramTransportTest.php | 9 ++++++++- 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Notifier/Bridge/RocketChat/RocketChatTransport.php b/src/Symfony/Component/Notifier/Bridge/RocketChat/RocketChatTransport.php index 79cfd1d502..ef2f68b0fb 100644 --- a/src/Symfony/Component/Notifier/Bridge/RocketChat/RocketChatTransport.php +++ b/src/Symfony/Component/Notifier/Bridge/RocketChat/RocketChatTransport.php @@ -43,6 +43,10 @@ final class RocketChatTransport extends AbstractTransport public function __toString(): string { + if (null === $this->chatChannel) { + return sprintf('rocketchat://%s', $this->getEndpoint()); + } + return sprintf('rocketchat://%s?channel=%s', $this->getEndpoint(), $this->chatChannel); } diff --git a/src/Symfony/Component/Notifier/Bridge/RocketChat/Tests/RocketChatTransportTest.php b/src/Symfony/Component/Notifier/Bridge/RocketChat/Tests/RocketChatTransportTest.php index f28e57ff52..87ec4e563a 100644 --- a/src/Symfony/Component/Notifier/Bridge/RocketChat/Tests/RocketChatTransportTest.php +++ b/src/Symfony/Component/Notifier/Bridge/RocketChat/Tests/RocketChatTransportTest.php @@ -30,6 +30,13 @@ final class RocketChatTransportTest extends TestCase $this->assertSame('rocketchat://host.test?channel=testChannel', (string) $transport); } + public function testToStringContainsNoChannelBecauseItsOptional() + { + $transport = $this->createTransport(null); + + $this->assertSame('rocketchat://host.test', (string) $transport); + } + public function testSupportsChatMessage() { $transport = $this->createTransport(); @@ -46,8 +53,8 @@ final class RocketChatTransportTest extends TestCase $transport->send($this->createMock(MessageInterface::class)); } - private function createTransport(): RocketChatTransport + private function createTransport(?string $channel = 'testChannel'): RocketChatTransport { - return (new RocketChatTransport('testAccessToken', 'testChannel', $this->createMock(HttpClientInterface::class)))->setHost('host.test'); + return (new RocketChatTransport('testAccessToken', $channel, $this->createMock(HttpClientInterface::class)))->setHost('host.test'); } } diff --git a/src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php b/src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php index b736cccb54..c5f8524c84 100644 --- a/src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php +++ b/src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php @@ -48,6 +48,10 @@ final class TelegramTransport extends AbstractTransport public function __toString(): string { + if (null === $this->chatChannel) { + return sprintf('telegram://%s', $this->getEndpoint()); + } + return sprintf('telegram://%s?channel=%s', $this->getEndpoint(), $this->chatChannel); } diff --git a/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php b/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php index ededeec27f..1cd15d2eac 100644 --- a/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php +++ b/src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php @@ -31,6 +31,13 @@ final class TelegramTransportTest extends TestCase $this->assertSame('telegram://host.test?channel=testChannel', (string) $transport); } + public function testToStringContainsNoChannelBecauseItsOptional() + { + $transport = $this->createTransport(null); + + $this->assertSame('telegram://host.test', (string) $transport); + } + public function testSupportsChatMessage() { $transport = $this->createTransport(); @@ -186,7 +193,7 @@ JSON; $this->assertEquals('telegram://host.test?channel=defaultChannel', $sentMessage->getTransport()); } - private function createTransport($channel = 'testChannel', ?HttpClientInterface $client = null): TelegramTransport + private function createTransport(?string $channel = 'testChannel', ?HttpClientInterface $client = null): TelegramTransport { return (new TelegramTransport('token', $channel, $client ?: $this->createMock(HttpClientInterface::class)))->setHost('host.test'); } From 9dd09e632d4b13c8ed95cbff66ac134f5af94eea Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Wed, 16 Dec 2020 12:35:09 +0100 Subject: [PATCH 3/6] [Notifier] Set message id on SentMessage --- .../Component/Notifier/Bridge/LinkedIn/LinkedInTransport.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInTransport.php b/src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInTransport.php index 26f4801b7a..9030496307 100644 --- a/src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInTransport.php +++ b/src/Symfony/Component/Notifier/Bridge/LinkedIn/LinkedInTransport.php @@ -90,7 +90,10 @@ final class LinkedInTransport extends AbstractTransport throw new TransportException(sprintf('Unable to post the Linkedin message : "%s".', $result['error']), $response); } - return new SentMessage($message, (string) $this); + $sentMessage = new SentMessage($message, (string) $this); + $sentMessage->setMessageId($result['id']); + + return $sentMessage; } private function bodyFromMessageWithNoOption(MessageInterface $message): array From 165c87247df4169ebb6c8fbaf243e9c2ed6ff63c Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Mon, 14 Dec 2020 13:29:07 +0100 Subject: [PATCH 4/6] [Notifier] [Discord] Use private const and mb_strlen() --- .../Component/Notifier/Bridge/Discord/DiscordTransport.php | 4 +++- .../Notifier/Bridge/Discord/Tests/DiscordTransportTest.php | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Notifier/Bridge/Discord/DiscordTransport.php b/src/Symfony/Component/Notifier/Bridge/Discord/DiscordTransport.php index afa98f77d6..3fb2061290 100644 --- a/src/Symfony/Component/Notifier/Bridge/Discord/DiscordTransport.php +++ b/src/Symfony/Component/Notifier/Bridge/Discord/DiscordTransport.php @@ -29,6 +29,8 @@ final class DiscordTransport extends AbstractTransport { protected const HOST = 'discord.com'; + private const SUBJECT_LIMIT = 2000; + private $token; private $webhookId; @@ -65,7 +67,7 @@ final class DiscordTransport extends AbstractTransport $content = $message->getSubject(); - if (\strlen($content) > 2000) { + if (mb_strlen($content, 'UTF-8') > self::SUBJECT_LIMIT) { throw new LogicException('The subject length of a Discord message must not exceed 2000 characters.'); } diff --git a/src/Symfony/Component/Notifier/Bridge/Discord/Tests/DiscordTransportTest.php b/src/Symfony/Component/Notifier/Bridge/Discord/Tests/DiscordTransportTest.php index 154a795530..ead3027580 100644 --- a/src/Symfony/Component/Notifier/Bridge/Discord/Tests/DiscordTransportTest.php +++ b/src/Symfony/Component/Notifier/Bridge/Discord/Tests/DiscordTransportTest.php @@ -56,7 +56,7 @@ final class DiscordTransportTest extends TestCase $this->expectException(LogicException::class); $this->expectExceptionMessage('The subject length of a Discord message must not exceed 2000 characters.'); - $transport->send(new ChatMessage(str_repeat('d', 2001))); + $transport->send(new ChatMessage(str_repeat('囍', 2001))); } public function testSendWithErrorResponseThrows() From 78da70615b1a5734dde5f03325f21a59c0865fc4 Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Fri, 18 Dec 2020 07:28:27 +0100 Subject: [PATCH 5/6] Remove void return type from test methods --- src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php | 6 +++--- .../Tests/Transport/AmazonSqsIntegrationTest.php | 4 ++-- .../StopWorkerOnFailureLimitListenerTest.php | 4 ++-- .../Constraints/ExpressionLanguageSyntaxValidatorTest.php | 8 ++++---- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php b/src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php index 04ee14c6cb..e755c7936f 100644 --- a/src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php +++ b/src/Symfony/Component/Mailer/Tests/Transport/DsnTest.php @@ -20,12 +20,12 @@ class DsnTest extends TestCase /** * @dataProvider fromStringProvider */ - public function testFromString(string $string, Dsn $dsn): void + public function testFromString(string $string, Dsn $dsn) { $this->assertEquals($dsn, Dsn::fromString($string)); } - public function testGetOption(): void + public function testGetOption() { $options = ['with_value' => 'some value', 'nullable' => null]; $dsn = new Dsn('smtp', 'example.com', null, null, null, $options); @@ -38,7 +38,7 @@ class DsnTest extends TestCase /** * @dataProvider invalidDsnProvider */ - public function testInvalidDsn(string $dsn, string $exceptionMessage): void + public function testInvalidDsn(string $dsn, string $exceptionMessage) { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage($exceptionMessage); diff --git a/src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/AmazonSqsIntegrationTest.php b/src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/AmazonSqsIntegrationTest.php index c840822abe..189a4e5489 100644 --- a/src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/AmazonSqsIntegrationTest.php +++ b/src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/AmazonSqsIntegrationTest.php @@ -21,7 +21,7 @@ use Symfony\Component\Messenger\Bridge\AmazonSqs\Transport\Connection; */ class AmazonSqsIntegrationTest extends TestCase { - public function testConnectionSendToFifoQueueAndGet(): void + public function testConnectionSendToFifoQueueAndGet() { if (!getenv('MESSENGER_SQS_FIFO_QUEUE_DSN')) { $this->markTestSkipped('The "MESSENGER_SQS_FIFO_QUEUE_DSN" environment variable is required.'); @@ -30,7 +30,7 @@ class AmazonSqsIntegrationTest extends TestCase $this->execute(getenv('MESSENGER_SQS_FIFO_QUEUE_DSN')); } - public function testConnectionSendAndGet(): void + public function testConnectionSendAndGet() { if (!getenv('MESSENGER_SQS_DSN')) { $this->markTestSkipped('The "MESSENGER_SQS_DSN" environment variable is required.'); diff --git a/src/Symfony/Component/Messenger/Tests/EventListener/StopWorkerOnFailureLimitListenerTest.php b/src/Symfony/Component/Messenger/Tests/EventListener/StopWorkerOnFailureLimitListenerTest.php index 9f12b0b258..a713a2820a 100644 --- a/src/Symfony/Component/Messenger/Tests/EventListener/StopWorkerOnFailureLimitListenerTest.php +++ b/src/Symfony/Component/Messenger/Tests/EventListener/StopWorkerOnFailureLimitListenerTest.php @@ -26,7 +26,7 @@ class StopWorkerOnFailureLimitListenerTest extends TestCase /** * @dataProvider countProvider */ - public function testWorkerStopsWhenMaximumCountReached(int $max, bool $shouldStop): void + public function testWorkerStopsWhenMaximumCountReached(int $max, bool $shouldStop) { $worker = $this->createMock(Worker::class); $worker->expects($shouldStop ? $this->atLeastOnce() : $this->never())->method('stop'); @@ -53,7 +53,7 @@ class StopWorkerOnFailureLimitListenerTest extends TestCase yield [4, false]; } - public function testWorkerLogsMaximumCountReachedWhenLoggerIsGiven(): void + public function testWorkerLogsMaximumCountReachedWhenLoggerIsGiven() { $logger = $this->createMock(LoggerInterface::class); $logger->expects($this->once())->method('info') diff --git a/src/Symfony/Component/Validator/Tests/Constraints/ExpressionLanguageSyntaxValidatorTest.php b/src/Symfony/Component/Validator/Tests/Constraints/ExpressionLanguageSyntaxValidatorTest.php index 677660f6c3..8cca54ed8a 100644 --- a/src/Symfony/Component/Validator/Tests/Constraints/ExpressionLanguageSyntaxValidatorTest.php +++ b/src/Symfony/Component/Validator/Tests/Constraints/ExpressionLanguageSyntaxValidatorTest.php @@ -23,7 +23,7 @@ class ExpressionLanguageSyntaxValidatorTest extends ConstraintValidatorTestCase return new ExpressionLanguageSyntaxValidator(new ExpressionLanguage()); } - public function testExpressionValid(): void + public function testExpressionValid() { $this->validator->validate('1 + 1', new ExpressionLanguageSyntax([ 'message' => 'myMessage', @@ -33,7 +33,7 @@ class ExpressionLanguageSyntaxValidatorTest extends ConstraintValidatorTestCase $this->assertNoViolation(); } - public function testExpressionWithoutNames(): void + public function testExpressionWithoutNames() { $this->validator->validate('1 + 1', new ExpressionLanguageSyntax([ 'message' => 'myMessage', @@ -42,7 +42,7 @@ class ExpressionLanguageSyntaxValidatorTest extends ConstraintValidatorTestCase $this->assertNoViolation(); } - public function testExpressionWithAllowedVariableName(): void + public function testExpressionWithAllowedVariableName() { $this->validator->validate('a + 1', new ExpressionLanguageSyntax([ 'message' => 'myMessage', @@ -52,7 +52,7 @@ class ExpressionLanguageSyntaxValidatorTest extends ConstraintValidatorTestCase $this->assertNoViolation(); } - public function testExpressionIsNotValid(): void + public function testExpressionIsNotValid() { $this->validator->validate('a + 1', new ExpressionLanguageSyntax([ 'message' => 'myMessage', From a80409af2520ef6379f580dafb3b36a6000e0d3f Mon Sep 17 00:00:00 2001 From: Oskar Stark Date: Thu, 17 Dec 2020 20:28:02 +0100 Subject: [PATCH 6/6] [Notifier] Fix parsing Dsn with empty user/password --- .../Notifier/Tests/Transport/DsnTest.php | 123 ++++++++++++++++++ .../Component/Notifier/Transport/Dsn.php | 4 +- 2 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php diff --git a/src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php b/src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php new file mode 100644 index 0000000000..fc7072dee6 --- /dev/null +++ b/src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php @@ -0,0 +1,123 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Notifier\Tests\Transport; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\Notifier\Exception\InvalidArgumentException; +use Symfony\Component\Notifier\Transport\Dsn; + +final class DsnTest extends TestCase +{ + /** + * @dataProvider fromStringProvider + */ + public function testFromString(string $string, Dsn $expectedDsn) + { + $actualDsn = Dsn::fromString($string); + + $this->assertSame($expectedDsn->getScheme(), $actualDsn->getScheme()); + $this->assertSame($expectedDsn->getHost(), $actualDsn->getHost()); + $this->assertSame($expectedDsn->getPort(), $actualDsn->getPort()); + $this->assertSame($expectedDsn->getUser(), $actualDsn->getUser()); + $this->assertSame($expectedDsn->getPassword(), $actualDsn->getPassword()); + $this->assertSame($expectedDsn->getPath(), $actualDsn->getPath()); + $this->assertSame($expectedDsn->getOption('from'), $actualDsn->getOption('from')); + + $this->assertSame($string, $actualDsn->getOriginalDsn()); + } + + public function fromStringProvider(): iterable + { + yield 'simple dsn' => [ + 'scheme://localhost', + new Dsn('scheme', 'localhost', null, null, null, [], null), + ]; + + yield 'simple dsn including @ sign, but no user/password/token' => [ + 'scheme://@localhost', + new Dsn('scheme', 'localhost', null, null), + ]; + + yield 'simple dsn including : sign and @ sign, but no user/password/token' => [ + 'scheme://:@localhost', + new Dsn('scheme', 'localhost', null, null), + ]; + + yield 'simple dsn including user, : sign and @ sign, but no password' => [ + 'scheme://user1:@localhost', + new Dsn('scheme', 'localhost', 'user1', null), + ]; + + yield 'simple dsn including : sign, password, and @ sign, but no user' => [ + 'scheme://:pass@localhost', + new Dsn('scheme', 'localhost', null, 'pass'), + ]; + + yield 'dsn with user and pass' => [ + 'scheme://u$er:pa$s@localhost', + new Dsn('scheme', 'localhost', 'u$er', 'pa$s', null, [], null), + ]; + + yield 'dsn with user and pass and custom port' => [ + 'scheme://u$er:pa$s@localhost:8000', + new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', [], null), + ]; + + yield 'dsn with user and pass, custom port and custom path' => [ + 'scheme://u$er:pa$s@localhost:8000/channel', + new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', [], '/channel'), + ]; + + yield 'dsn with user and pass, custom port, custom path and custom options' => [ + 'scheme://u$er:pa$s@localhost:8000/channel?from=FROM', + new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', ['from' => 'FROM'], '/channel'), + ]; + } + + /** + * @dataProvider invalidDsnProvider + */ + public function testInvalidDsn(string $dsn, string $exceptionMessage) + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($exceptionMessage); + Dsn::fromString($dsn); + } + + public function invalidDsnProvider(): iterable + { + yield [ + 'some://', + 'The "some://" notifier DSN is invalid.', + ]; + + yield [ + '//slack', + 'The "//slack" notifier DSN must contain a scheme.', + ]; + + yield [ + 'file:///some/path', + 'The "file:///some/path" notifier DSN must contain a host (use "default" by default).', + ]; + } + + public function testGetOption() + { + $options = ['with_value' => 'some value', 'nullable' => null]; + $dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel'); + + $this->assertSame('some value', $dsn->getOption('with_value')); + $this->assertSame('default', $dsn->getOption('nullable', 'default')); + $this->assertSame('default', $dsn->getOption('not_existent_property', 'default')); + } +} diff --git a/src/Symfony/Component/Notifier/Transport/Dsn.php b/src/Symfony/Component/Notifier/Transport/Dsn.php index c18bc600e2..a8c556e8f6 100644 --- a/src/Symfony/Component/Notifier/Transport/Dsn.php +++ b/src/Symfony/Component/Notifier/Transport/Dsn.php @@ -54,8 +54,8 @@ final class Dsn throw new InvalidArgumentException(sprintf('The "%s" notifier DSN must contain a host (use "default" by default).', $dsn)); } - $user = isset($parsedDsn['user']) ? urldecode($parsedDsn['user']) : null; - $password = isset($parsedDsn['pass']) ? urldecode($parsedDsn['pass']) : null; + $user = '' !== ($parsedDsn['user'] ?? '') ? urldecode($parsedDsn['user']) : null; + $password = '' !== ($parsedDsn['pass'] ?? '') ? urldecode($parsedDsn['pass']) : null; $port = $parsedDsn['port'] ?? null; $path = $parsedDsn['path'] ?? null; parse_str($parsedDsn['query'] ?? '', $query);