8000 feature #39585 [Notifier] Change Dsn api (OskarStark) · symfony/symfony@1eb849d · GitHub
[go: up one dir, main page]
Skip to content

Commit 1eb849d

Browse files
fabpotfabpot
committed
feature #39585 [Notifier] Change Dsn api (OskarStark)
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 ------- 44e8ca1 [Notifier] Change Dsn api
2 parents a608793 + 44e8ca1 commit 1eb849d

File tree

11 files changed

+183
-96
lines changed

11 files changed

+183
-96
lines changed

‎src/Symfony/Component/Notifier/Bridge/Esendex/CHANGELOG.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ CHANGELOG
55
-----
66

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

1313
5.2.0
1414
-----

‎src/Symfony/Component/Notifier/Bridge/LinkedIn/CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ CHANGELOG
55
-----
66

77
* The bridge is not marked as `@experimental` anymore
8-
* [BC BREAK] `LinkedInTransportFactory` is now final
8+
* [BC BREAK] `LinkedInTransportFactory` is now final
99

1010
5.2.0
1111
-----

‎src/Symfony/Component/Notifier/Bridge/Mattermost/CHANGELOG.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ CHANGELOG
55
-----
66

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

1313
5.1.0
1414
-----

‎src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportFactoryTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public function testCreateWithDeprecatedDsn()
5252
$this->expectException(InvalidArgumentException::class);
5353
$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).');
5454

55-
$factory->create(Dsn::fromString('slack://default/XXXXXXXXX/XXXXXXXXX/XXXXXXXXXXXXXXXXXXXXXXXX'));
55+
$factory->create(new Dsn('slack://default/XXXXXXXXX/XXXXXXXXX/XXXXXXXXXXXXXXXXXXXXXXXX'));
5656
}
5757

5858
public function supportsProvider(): iterable

‎src/Symfony/Component/Notifier/Bridge/Zulip/CHANGELOG.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ CHANGELOG
55
-----
66

77
* The bridge is not marked as `@experimental` anymore
8-
* [BC BREAK] `ZulipTransport` is now final
9-
* [BC BREAK] `ZulipTransportFactory` is now final
8+
* [BC BREAK] `ZulipTransport` is now final
9+
* [BC BREAK] `ZulipTransportFactory` is now final
1010

1111
5.2.0
1212
-----

‎src/Symfony/Component/Notifier/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ CHANGELOG
55
-----
66

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

‎src/Symfony/Component/Notifier/Tests/Transport/DsnTest.php

Lines changed: 137 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -19,79 +19,136 @@
1919
final class DsnTest extends TestCase
2020
{
2121
/**
22-
* @dataProvider fromStringProvider
22+
* @dataProvider constructProvider
2323
*/
24-
public function testFromString(string $string, Dsn $expectedDsn)
24+
public function testConstruct(string $dsnString, string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null)
2525
{
26-
$actualDsn = Dsn::fromString($string);
26+
$dsn = new Dsn($dsnString);
27+
$this->assertSame($dsnString, $dsn->getOriginalDsn());
2728

28-
$this->assertSame($expectedDsn->getScheme(), $actualDsn->getScheme());
29-
$this->assertSame($expectedDsn->getHost(), $actualDsn->getHost());
30-
$this->assertSame($expectedDsn->getPort(), $actualDsn->getPort());
31-
$this->assertSame($expectedDsn->getUser(), $actualDsn->getUser());
32-
$this->assertSame($expectedDsn->getPassword(), $actualDsn->getPassword());
33-
$this->assertSame($expectedDsn->getPath(), $actualDsn->getPath());
34-
$this->assertSame($expectedDsn->getOption('from'), $actualDsn->getOption('from'));
35-
36-
$this->assertSame($string, $actualDsn->getOriginalDsn());
29+
$this->assertSame($scheme, $dsn->getScheme());
30+
$this->assertSame($host, $dsn->getHost());
31+
$this->assertSame($user, $dsn->getUser());
32+
$this->assertSame($password, $dsn->getPassword());
33+
$this->assertSame($port, $dsn->getPort());
34+
$this->assertSame($path, $dsn->getPath());
35+
$this->assertSame($options, $dsn->getOptions());
3736
}
3837

39-
public function fromStringProvider(): iterable
38+
public function constructProvider(): iterable
4039
{
4140
yield 'simple dsn' => [
4241
'scheme://localhost',
43-
new Dsn('scheme', 'localhost', null, null, null, [], null),
42+
'scheme',
43+
'localhost',
4444
];
4545

4646
yield 'simple dsn including @ sign, but no user/password/token' => [
4747
'scheme://@localhost',
48-
new Dsn('scheme', 'localhost', null, null),
48+
'scheme',
49+
'localhost',
4950
];
5051

5152
yield 'simple dsn including : sign and @ sign, but no user/password/token' => [
5253
'scheme://:@localhost',
53-
new Dsn('scheme', 'localhost', null, null),
54+
'scheme',
55+
'localhost',
5456
];
5557

5658
yield 'simple dsn including user, : sign and @ sign, but no password' => [
5759
'scheme://user1:@localhost',
58-
new Dsn('scheme', 'localhost', 'user1', null),
60+
'scheme',
61+
'localhost',
62+
'user1',
5963
];
6064

6165
yield 'simple dsn including : sign, password, and @ sign, but no user' => [
6266
'scheme://:pass@localhost',
63-
new Dsn('scheme', 'localhost', null, 'pass'),
67+
'scheme',
68+
'localhost',
69+
null,
70+
'pass',
6471
];
6572

6673
yield 'dsn with user and pass' => [
6774
'scheme://u$er:pa$s@localhost',
68-
new Dsn('scheme', 'localhost', 'u$er', 'pa$s', null, [], null),
75+
'scheme',
76+
'localhost',
77+
'u$er',
78+
'pa$s',
6979
];
7080

7181
yield 'dsn with user and pass and custom port' => [
7282
'scheme://u$er:pa$s@localhost:8000',
73-
new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', [], null),
83+
'scheme',
84+
'localhost',
85+
'u$er',
86+
'pa$s',
87+
8000,
7488
];
7589

7690
yield 'dsn with user and pass, custom port and custom path' => [
7791
'scheme://u$er:pa$s@localhost:8000/channel',
78-
new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', [], '/channel'),
92+
'scheme',
93+
'localhost',
94+
'u$er',
95+
'pa$s',
96+
8000,
97+
[],
98+
'/channel',
7999
];
80100

81-
yield 'dsn with user and pass, custom port, custom path and custom options' => [
101+
yield 'dsn with user and pass, custom port, custom path and custom option' => [
82102
'scheme://u$er:pa$s@localhost:8000/channel?from=FROM',
83-
new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', ['from' => 'FROM'], '/channel'),
103+
'scheme',
104+
'localhost',
105+
'u$er',
106+
'pa$s',
107+
8000,
108+
[
109+
'from' => 'FROM',
110+
],
111+
'/channel',
112+
];
113+
114+
yield 'dsn with user and pass, custom port, custom path and custom options' => [
115+
'scheme://u$er:pa$s@localhost:8000/channel?from=FROM&to=TO',
116+
'scheme',
117+
'localhost',
118+
'u$er',
119+
'pa$s',
120+
8000,
121+
[
122+
'from' => 'FROM',
123+
'to' => 'TO',
124+
],
125+
'/channel',
126+
];
127+
128+
yield 'dsn with user and pass, custom port, custom path and custom options and custom options keep the same order' => [
129+
'scheme://u$er:pa$s@localhost:8000/channel?to=TO&from=FROM',
130+
'scheme',
131+
'localhost',
132+
'u$er',
133+
'pa$s',
134+
8000,
135+
[
136+
'to' => 'TO',
137+
'from' => 'FROM',
138+
],
139+
'/channel',
84140
];
85141
}
86142

87143
/**
88144
* @dataProvider invalidDsnProvider
89145
*/
90-
public function testInvalidDsn(string $dsn, string $exceptionMessage)
146+
public function testInvalidDsn(string $dsnString, string $exceptionMessage)
91147
{
92148
$this->expectException(InvalidArgumentException::class);
93149
$this->expectExceptionMessage($exceptionMessage);
94-
Dsn::fromString($dsn);
150+
151+
new Dsn($dsnString);
95152
}
96153

97154
public function invalidDsnProvider(): iterable
@@ -112,38 +169,75 @@ public function invalidDsnProvider(): iterable
112169
];
113170
}
114171

115-
public function testGetOption()
172+
/**
173+
* @dataProvider getOptionProvider
174+
*/
175+
public function testGetOption($expected, string $dsnString, string $option, ?string $default = null)
116176
{
117-
$options = ['with_value' => 'some value', 'nullable' => null];
118-
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
177+
$dsn = new Dsn($dsnString);
119178

120-
$this->assertSame('some value', $dsn->getOption('with_value'));
121-
$this->assertSame('default', $dsn->getOption('nullable', 'default'));
122-
$this->assertSame('default', $dsn->getOption('not_existent_property', 'default'));
179+
$this->assertSame($expected, $dsn->getOption($option, $default));
123180
}
124181

125-
public function testGetRequiredOptionGetsOptionIfSet()
182+
public function getOptionProvider(): iterable
126183
{
127-
$options = ['with_value' => 'some value'];
128-
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
184+
yield [
185+
'foo',
186+
'scheme://localhost?with_value=foo',
187+
'with_value',
188+
];
189+
190+
yield [
191+
'',
192+
'scheme://localhost?empty=',
193+
'empty',
194+
];
129195

130-
$this->assertSame('some value', $dsn->getRequiredOption('with_value'));
196+
yield [
197+
'0',
198+
'scheme://localhost?zero=0',
199+
'zero',
200+
];
201+
202+
yield [
203+
'default-value',
204+
'scheme://localhost?option=value',
205+
'non_existent_property',
206+
'default-value',
207+
];
208+
}
209+
210+
/**
211+
* @dataProvider getRequiredOptionProvider
212+
*/
213+
public function testGetRequiredOption(string $expectedValue, string $options, string $option)
214+
{
215+
$dsn = new Dsn(sprintf('scheme://localhost?%s', $options));
216+
217+
$this->assertSame($expectedValue, $dsn->getRequiredOption($option));
131218
}
132219

133-
public function testGetRequiredOptionGetsOptionIfValueIsZero()
220+
public function getRequiredOptionProvider(): iterable
134221
{
135-
$options = ['timeout' => 0];
136-
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
222+
yield [
223+
'value',
224+
'with_value=value',
225+
'with_value',
226+
];
137227

138-
$this->assertSame(0, $dsn->getRequiredOption('timeout'));
228+
yield [
229+
'0',
230+
'timeout=0',
231+
'timeout',
232+
];
139233
}
140234

141235
/**
142236
* @dataProvider getRequiredOptionThrowsMissingRequiredOptionExceptionProvider
143237
*/
144-
public function testGetRequiredOptionThrowsMissingRequiredOptionException(string $expectedExceptionMessage, array $options, string $option)
238+
public function testGetRequiredOptionThrowsMissingRequiredOptionException(string $expectedExceptionMessage, string $options, string $option)
145239
{
146-
$dsn = new Dsn('scheme', 'localhost', 'u$er', 'pa$s', '8000', $options, '/channel');
240+
$dsn = new Dsn(sprintf('scheme://localhost?%s', $options));
147241

148242
$this->expectException(MissingRequiredOptionException::class);
149243
$this->expectExceptionMessage($expectedExceptionMessage);
@@ -155,20 +249,14 @@ public function getRequiredOptionThrowsMissingRequiredOptionExceptionProvider():
155249
{
156250
yield [
157251
'The option "foo_bar" is required but missing.',
158-
['with_value' => 'some value'],
252+
'with_value=value',
159253
'foo_bar',
160254
];
161255

162256
yield [
163257
'The option "with_empty_string" is required but missing.',
164-
['with_empty_string' => ''],
258+
'with_empty_string=',
165259
'with_empty_string',
166260
];
167-
168-
yield [
169-
'The option "with_null" is required but missing.',
170-
['with_null' => null],
171-
'with_null',
172-
];
173261
}
174262
}

‎src/Symfony/Component/Notifier/Tests/Transport/NullTransportFactoryTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,14 @@ public function testCreateThrowsUnsupportedSchemeException()
4141
{
4242
$this->expectException(UnsupportedSchemeException::class);
4343

44-
$this->nullTransportFactory->create(new Dsn('foo', ''));
44+
$this->nullTransportFactory->create(new Dsn('foo://localhost'));
4545
}
4646

4747
public function testCreate()
4848
{
4949
$this->assertInstanceOf(
5050
NullTransport::class,
51-
$this->nullTransportFactory->create(new Dsn('null', ''))
51+
$this->nullTransportFactory->create(new Dsn('null://null'))
5252
);
5353
}
5454
}

0 commit comments

Comments
 (0)
0