[Notifier] Change Dsn api #39585
[Notifier] Change Dsn api #39585
Conversation
| yield [ | ||
| '', | ||
| 'scheme://localhost?empty=', | ||
| 'empty', | ||
| ]; |
There was a problem hiding this comment.
I would prefer to return null in case of an empty string, wdyt?
There was a problem hiding this comment.
@fabpot thoughts?
a809ab7 to
6afcbc0
Compare
|
I would love to get your feedback on this @fabpot 👍 |
|
|
||
| public function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null) | ||
| private function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, array $options = [], ?string $path = null) |
There was a problem hiding this comment.
I don't see why we should forbid the usage of the constructor.
There was a problem hiding this comment.
Otherwise you would end up with the same problem.... original DSN is not set, but used heavily in exceptions.
See explanation in the referenced issue please.
There was a problem hiding this comment.
Instead, would it possible to create the "DSN as string" from all the parts given in the constructor?
There was a problem hiding this comment.
Yes it would, but it is not the "original"/"provided" one, so let's say we can't parse anything or have magic chars or .... we will not use the provided one in the exception message, but a generated one which is not what I would expect 🤷♂️
So this would be more a "toString" instead of a "original dsn"...
Besides this, its a class to deal with DSN's which are strings per design, so I would go with strings only, another positive aspect I encountered while reworking all tests for the bridges: the dsn is much more readable as a string ❗
There was a problem hiding this comment.
I get it now :) So, what about removing the fromString() method instead and pass the DSN (a string) as the only argument to the constructor?
There was a problem hiding this comment.
@OskarStark Have you seen my comment above?
There was a problem hiding this comment.
Didn't see the comment, will have a look later or tomorrow 👍🏻
There was a problem hiding this comment.
I get it now :) So, what about removing the fromString() method instead and pass the DSN (a string) as the only argument to the constructor?
Yes we can 👍 Will do
e5568cd to
c807a08
Compare
|
Can you have a look at the failing tests? |
|
All green now @fabpot 👍 |
|
I also streamlined some changelog files. @fabpot I would love to hear your feedback on:
and #39585 (comment) |
| `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)` | ||
| * The bridge is not marked as `@experimental` anymore. |
There was a problem hiding this comment.
Instead of going back and forth on this, I've documented the conventions in the docs: https://github.com/symfony/symfony-docs/pull/14830/files
| { | ||
| return $this->options; | ||
| } | ||
|
|
||
| public function getPath(): ?string | ||
| { | ||
| return $this->path; | ||
| } | ||
|
|
||
| public function getOriginalDsn(): string |
There was a problem hiding this comment.
Nitpick: What about renaming it to __toString()?
There was a problem hiding this comment.
I would avoid that, because that would be yet another BC break
|
Thank you @OskarStark. |
This PR
getOptions()method, which could be helpful and also improves testability instead of dealing with reflections__construct()method accept only a dsn as stringfromString()methodgetOriginalDsn()method, like described by its return type, before it returned null when Dsn class was instantiated via the constructor and aTypeErrorwas thrownThis should be done for the Mailer Dsn class too, but this class is not experimental anymore... 🤔