diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feature #39585 [Notifier] Change Dsn api (OskarStark)

Browse Source 
This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Notifier] Change Dsn api

| Q             | A
| ------------- | ---
| Branch?       | 5.x, BC BREAK, but experimental
| Bug fix?      | yes
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #39533
| License       | MIT
| Doc PR        | ---

This PR

* [x] adds a new `getOptions()` method, which could be helpful and also improves testability instead of dealing with reflections
* [x] makes the `__construct()` method accept only a dsn as string
* [x] removes `fromString()` method
* [x] afterwards you can always rely on `getOriginalDsn()` method, like described by its return type, before it returned null when Dsn class was instantiated via the constructor and a `TypeError` was thrown
* [x] refactored the tests

This should be done for the Mailer Dsn class too, but this class is not experimental anymore... 🤔

Commits
-------

44e8ca164f [Notifier] Change Dsn api
This commit is contained in:
Fabien Potencier 2021-01-14 08:54:42 +01:00
commit 1eb849dfb4
11 changed files with 183 additions and 96 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/Symfony/Component/Notifier/Bridge/Esendex/CHANGELOG.md
View File

@ -5,10 +5,10 @@ CHANGELOG
-----
* The bridge is not marked as `@experimental` anymore
* [BC BREAK] Changed signature of `EsendexTransport::__construct()` method from:
`public function __construct(string $token, string $accountReference, string $from, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
to:
`public function __construct(string $email, string $password, string $accountReference, string $from, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
* [BC BREAK] Change signature of `EsendexTransport::__construct()` method from:
`public function __construct(string $token, string $accountReference, string $from, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
to:
`public function __construct(string $email, string $password, string $accountReference, string $from, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
5.2.0
-----

2
src/Symfony/Component/Notifier/Bridge/LinkedIn/CHANGELOG.md
View File

@ -5,7 +5,7 @@ CHANGELOG
-----
* The bridge is not marked as `@experimental` anymore
* [BC BREAK] `LinkedInTransportFactory` is now final
* [BC BREAK] `LinkedInTransportFactory` is now final
5.2.0
-----

8
src/Symfony/Component/Notifier/Bridge/Mattermost/CHANGELOG.md
View File

@ -5,10 +5,10 @@ CHANGELOG
-----
* The bridge is not marked as `@experimental` anymore
* [BC BREAK] Changed signature of `MattermostTransport::__construct()` method from:
`public function __construct(string $token, string $channel, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null, string $path = null)`
to:
`public function __construct(string $token, string $channel, ?string $path = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
* [BC BREAK] Change signature of `MattermostTransport::__construct()` method from:
`public function __construct(string $token, string $channel, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null, string $path = null)`
to:
`public function __construct(string $token, string $channel, ?string $path = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
5.1.0
-----

2
src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php
View File

@ -52,7 +52,7 @@ final class SlackTransportFactoryTest extends TransportFactoryTestCase
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Support for Slack webhook DSN has been dropped since 5.2 (maybe you haven\'t updated the DSN when upgrading from 5.1).');
$factory->create(Dsn::fromString('slack://default/XXXXXXXXX/XXXXXXXXX/XXXXXXXXXXXXXXXXXXXXXXXX'));
$factory->create(new Dsn('slack://default/XXXXXXXXX/XXXXXXXXX/XXXXXXXXXXXXXXXXXXXXXXXX'));
}
public function supportsProvider(): iterable

4
src/Symfony/Component/Notifier/Bridge/Zulip/CHANGELOG.md
View File

@ -5,8 +5,8 @@ CHANGELOG
-----
* The bridge is not marked as `@experimental` anymore
* [BC BREAK] `ZulipTransport` is now final
* [BC BREAK] `ZulipTransportFactory` is now final
* [BC BREAK] `ZulipTransport` is now final
* [BC BREAK] `ZulipTransportFactory` is now final
5.2.0
-----

5
src/Symfony/Component/Notifier/CHANGELOG.md
View File

@ -5,6 +5,11 @@ CHANGELOG
-----
* The component is not marked as `@experimental` anymore
* [BC BREAK] Change signature of `Dsn::__construct()` method from:
`public function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null)`
to:
`public function __construct(string $dsn)`
* [BC BREAK] Remove `Dsn::fromString()` method
* [BC BREAK] Changed the return type of `AbstractTransportFactory::getEndpoint()` from `?string` to `string`
* Added `DSN::getRequiredOption` method which throws a new `MissingRequiredOptionException`.

186
src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php
View File

@ -19,79 +19,136 @@ use Symfony\Component\Notifier\Transport\Dsn;
final class DsnTest extends TestCase
{
/**
* @dataProvider fromStringProvider
* @dataProvider constructProvider
*/
public function testFromString(string $string, Dsn $expectedDsn)
public function testConstruct(string $dsnString, string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null)
{
$actualDsn = Dsn::fromString($string);
$dsn = new Dsn($dsnString);
$this->assertSame($dsnString, $dsn->getOriginalDsn());
$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());
$this->assertSame($scheme, $dsn->getScheme());
$this->assertSame($host, $dsn->getHost());
$this->assertSame($user, $dsn->getUser());
$this->assertSame($password, $dsn->getPassword());
$this->assertSame($port, $dsn->getPort());
$this->assertSame($path, $dsn->getPath());
$this->assertSame($options, $dsn->getOptions());
}
public function fromStringProvider(): iterable
public function constructProvider(): iterable
{
yield 'simple dsn' => [
'scheme://localhost',
new Dsn('scheme', 'localhost', null, null, null, [], null),
'scheme',
'localhost',
];
yield 'simple dsn including @ sign, but no user/password/token' => [
'scheme://@localhost',
new Dsn('scheme', 'localhost', null, null),
'scheme',
'localhost',
];
yield 'simple dsn including : sign and @ sign, but no user/password/token' => [
'scheme://:@localhost',
new Dsn('scheme', 'localhost', null, null),
'scheme',
'localhost',
];
yield 'simple dsn including user, : sign and @ sign, but no password' => [
'scheme://user1:@localhost',
new Dsn('scheme', 'localhost', 'user1', null),
'scheme',
'localhost',
'user1',
];
yield 'simple dsn including : sign, password, and @ sign, but no user' => [
'scheme://:pass@localhost',
new Dsn('scheme', 'localhost', null, 'pass'),
'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),
'scheme',
'localhost',
'u$er',
'pa$s',
];
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),
'scheme',
'localhost',
'u$er',
'pa$s',
8000,
];
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'),
'scheme',
'localhost',
'u$er',
'pa$s',
8000,
[],
'/channel',
];
yield 'dsn with user and pass, custom port, custom path and custom option' => [
'scheme://u$er:pa$s@localhost:8000/channel?from=FROM',
'scheme',
'localhost',
'u$er',
'pa$s',
8000,
[
'from' => 'FROM',
],
'/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'),
'scheme://u$er:pa$s@localhost:8000/channel?from=FROM&to=TO',
'scheme',
'localhost',
'u$er',
'pa$s',
8000,
[
'from' => 'FROM',
'to' => 'TO',
],
'/channel',
];
yield 'dsn with user and pass, custom port, custom path and custom options and custom options keep the same order' => [
'scheme://u$er:pa$s@localhost:8000/channel?to=TO&from=FROM',
'scheme',
'localhost',
'u$er',
'pa$s',
8000,
[
'to' => 'TO',
'from' => 'FROM',
],
'/channel',
];
}
/**
* @dataProvider invalidDsnProvider
*/
public function testInvalidDsn(string $dsn, string $exceptionMessage)
public function testInvalidDsn(string $dsnString, string $exceptionMessage)
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage($exceptionMessage);
Dsn::fromString($dsn);
new Dsn($dsnString);
}
public function invalidDsnProvider(): iterable
@ -112,38 +169,75 @@ final class DsnTest extends TestCase
];
}
public function testGetOption()
/**
* @dataProvider getOptionProvider
*/
public function testGetOption($expected, string $dsnString, string $option, ?string $default = null)
{
$options = ['with_value' => 'some value', 'nullable' => null];
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
$dsn = new Dsn($dsnString);
$this->assertSame('some value', $dsn->getOption('with_value'));
$this->assertSame('default', $dsn->getOption('nullable', 'default'));
$this->assertSame('default', $dsn->getOption('not_existent_property', 'default'));
$this->assertSame($expected, $dsn->getOption($option, $default));
}
public function testGetRequiredOptionGetsOptionIfSet()
public function getOptionProvider(): iterable
{
$options = ['with_value' => 'some value'];
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
yield [
'foo',
'scheme://localhost?with_value=foo',
'with_value',
];
$this->assertSame('some value', $dsn->getRequiredOption('with_value'));
yield [
'',
'scheme://localhost?empty=',
'empty',
];
yield [
'0',
'scheme://localhost?zero=0',
'zero',
];
yield [
'default-value',
'scheme://localhost?option=value',
'non_existent_property',
'default-value',
];
}
public function testGetRequiredOptionGetsOptionIfValueIsZero()
/**
* @dataProvider getRequiredOptionProvider
*/
public function testGetRequiredOption(string $expectedValue, string $options, string $option)
{
$options = ['timeout' => 0];
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
$dsn = new Dsn(sprintf('scheme://localhost?%s', $options));
$this->assertSame(0, $dsn->getRequiredOption('timeout'));
$this->assertSame($expectedValue, $dsn->getRequiredOption($option));
}
public function getRequiredOptionProvider(): iterable
{
yield [
'value',
'with_value=value',
'with_value',
];
yield [
'0',
'timeout=0',
'timeout',
];
}
/**
* @dataProvider getRequiredOptionThrowsMissingRequiredOptionExceptionProvider
*/
public function testGetRequiredOptionThrowsMissingRequiredOptionException(string $expectedExceptionMessage, array $options, string $option)
public function testGetRequiredOptionThrowsMissingRequiredOptionException(string $expectedExceptionMessage, string $options, string $option)
{
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
$dsn = new Dsn(sprintf('scheme://localhost?%s', $options));
$this->expectException(MissingRequiredOptionException::class);
$this->expectExceptionMessage($expectedExceptionMessage);
@ -155,20 +249,14 @@ final class DsnTest extends TestCase
{
yield [
'The option "foo_bar" is required but missing.',
['with_value' => 'some value'],
'with_value=value',
'foo_bar',
];
yield [
'The option "with_empty_string" is required but missing.',
['with_empty_string' => ''],
'with_empty_string=',
'with_empty_string',
];
yield [
'The option "with_null" is required but missing.',
['with_null' => null],
'with_null',
];
}
}

4
src/Symfony/Component/Notifier/Tests/Transport/NullTransportFactoryTest.php
View File

@ -41,14 +41,14 @@ class NullTransportFactoryTest extends TestCase
{
$this->expectException(UnsupportedSchemeException::class);
$this->nullTransportFactory->create(new Dsn('foo', ''));
$this->nullTransportFactory->create(new Dsn('foo://localhost'));
}
public function testCreate()
{
$this->assertInstanceOf(
NullTransport::class,
$this->nullTransportFactory->create(new Dsn('null', ''))
$this->nullTransportFactory->create(new Dsn('null://null'))
);
}
}

14
src/Symfony/Component/Notifier/Tests/TransportFactoryTestCase.php
View File

@ -13,8 +13,8 @@ namespace Symfony\Component\Notifier\Tests;
use PHPUnit\Framework\TestCase;
use Symfony\Component\Notifier\Exception\IncompleteDsnException;
use Symfony\Component\Notifier\Exception\UnsupportedSchemeException;
use Symfony\Component\Notifier\Exception\MissingRequiredOptionException;
use Symfony\Component\Notifier\Exception\UnsupportedSchemeException;
use Symfony\Component\Notifier\Transport\Dsn;
use Symfony\Component\Notifier\Transport\TransportFactoryInterface;
@ -68,7 +68,7 @@ abstract class TransportFactoryTestCase extends TestCase
{
$factory = $this->createFactory();
$this->assertSame($expected, $factory->supports(Dsn::fromString($dsn)));
$this->assertSame($expected, $factory->supports(new Dsn($dsn)));
}
/**
@ -77,7 +77,7 @@ abstract class TransportFactoryTestCase extends TestCase
public function testCreate(string $expected, string $dsn)
{
$factory = $this->createFactory();
$transport = $factory->create(Dsn::fromString($dsn));
$transport = $factory->create(new Dsn($dsn));
$this->assertSame($expected, (string) $transport);
}
@ -89,7 +89,7 @@ abstract class TransportFactoryTestCase extends TestCase
{
$factory = $this->createFactory();
$dsn = Dsn::fromString($dsn);
$dsn = new Dsn($dsn);
$this->expectException(UnsupportedSchemeException::class);
if (null !== $message) {
@ -106,7 +106,7 @@ abstract class TransportFactoryTestCase extends TestCase
{
$factory = $this->createFactory();
$dsn = Dsn::fromString($dsn);
$dsn = new Dsn($dsn);
$this->expectException(IncompleteDsnException::class);
if (null !== $message) {
@ -119,11 +119,11 @@ abstract class TransportFactoryTestCase extends TestCase
/**
* @dataProvider missingRequiredOptionProvider
*/
public function testMissingRequiredOptionException(string $dsn, string $message = null): void
public function testMissingRequiredOptionException(string $dsn, string $message = null)
{
$factory = $this->createFactory();
$dsn = Dsn::fromString($dsn);
$dsn = new Dsn($dsn);
$this->expectException(MissingRequiredOptionException::class);
if (null !== $message) {

4
src/Symfony/Component/Notifier/Transport.php
View File

@ -112,7 +112,7 @@ class Transport
return new RoundRobinTransport($this->createFromDsns($dsns));
}
return $this->fromDsnObject(Dsn::fromString($dsn));
return $this->fromDsnObject(new Dsn($dsn));
}
public function fromDsnObject(Dsn $dsn): TransportInterface
@ -133,7 +133,7 @@ class Transport
{
$transports = [];
foreach ($dsns as $dsn) {
$transports[] = $this->fromDsnObject(Dsn::fromString($dsn));
$transports[] = $this->fromDsnObject(new Dsn($dsn));
}
return $transports;

42
src/Symfony/Component/Notifier/Transport/Dsn.php
View File

@ -16,6 +16,7 @@ use Symfony\Component\Notifier\Exception\MissingRequiredOptionException;
/**
* @author Fabien Potencier <fabien@symfony.com>
* @author Oskar Stark <oskarstark@googlemail.com>
*/
final class Dsn
{
@ -24,23 +25,14 @@ final class Dsn
private $user;
private $password;
private $port;
private $options;
private $path;
private $dsn;
private $options;
private $originalDsn;
public function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null)
public function __construct(string $dsn)
{
$this->scheme = $scheme;
$this->host = $host;
$this->user = $user;
$this->password = $password;
$this->port = $port;
$this->options = $options;
$this->path = $path;
}
$this->originalDsn = $dsn;
public static function fromString(string $dsn): self
{
if (false === $parsedDsn = parse_url($dsn)) {
throw new InvalidArgumentException(sprintf('The "%s" notifier DSN is invalid.', $dsn));
}
@ -48,21 +40,18 @@ final class Dsn
if (!isset($parsedDsn['scheme'])) {
throw new InvalidArgumentException(sprintf('The "%s" notifier DSN must contain a scheme.', $dsn));
}
$this->scheme = $parsedDsn['scheme'];
if (!isset($parsedDsn['host'])) {
throw new InvalidArgumentException(sprintf('The "%s" notifier DSN must contain a host (use "default" by default).', $dsn));
}
$this->host = $parsedDsn['host'];
$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);
$dsnObject = new self($parsedDsn['scheme'], $parsedDsn['host'], $user, $password, $port, $query, $path);
$dsnObject->dsn = $dsn;
return $dsnObject;
$this->user = '' !== ($parsedDsn['user'] ?? '') ? urldecode($parsedDsn['user']) : null;
$this->password = '' !== ($parsedDsn['pass'] ?? '') ? urldecode($parsedDsn['pass']) : null;
$this->port = $parsedDsn['port'] ?? null;
$this->path = $parsedDsn['path'] ?? null;
parse_str($parsedDsn['query'] ?? '', $this->options);
}
public function getScheme(): string
@ -104,6 +93,11 @@ final class Dsn
return $this->options[$key];
}
public function getOptions(): array
{
return $this->options;
}
public function getPath(): ?string
{
return $this->path;
@ -111,6 +105,6 @@ final class Dsn
public function getOriginalDsn(): string
{
return $this->dsn;
return $this->originalDsn;
}
}