[Lock] add the DSN object #33200
[Lock] add the DSN object #33200
Conversation
900a181 to
253a2a3
Compare
253a2a3 to
27f35c7
Compare
|
If we go down this road, I'd like a full PR. For instance, it's not possible to configure some adapter via a DSN like the Doctrine one. Can you work on this so we get the full picture? |
|
Is this one related #24267 |
|
@OskarStark It was a first draft but we are going to implement in the lock as an exemple to see what will be the good path |
| private $path; | ||
| private $options; | ||
|
|
||
| public function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, ?string $path = null, array $options = []) |
There was a problem hiding this comment.
should be private to be immutable
| $options); | ||
| } | ||
|
|
||
| public function getScheme(): string |
There was a problem hiding this comment.
| public function getScheme(): string | |
| public function scheme(): string |
I am 👍 for no get-prefix. Same for the others
Except getOption(...)
| $this->options = $options; | ||
| } | ||
|
|
||
| public static function isValid(string $dsn) |
There was a problem hiding this comment.
not sure this is a good idea, by having this method the Dsn object could be instantiated and be in an invalid state.
Why not throw an exception instead?
There was a problem hiding this comment.
Ah I saw this is using a parameter and to the objects state... Then why using it? You can wrap your code in try-catch-block to achieve the same goal, right?
| private $options; | ||
|
|
||
| public function __construct(string $scheme, string $host, ?string $user = null, ?string $password = null, ?int $port = null, ?string $path = null, array $options = []) | ||
| { |
There was a problem hiding this comment.
You should assert, that the string values are not empty IMO
|
|
||
| parse_str($parsedDsn['query'] ?? '', $options); | ||
|
|
||
| return new self($parsedDsn['scheme'], |
There was a problem hiding this comment.
| return new self($parsedDsn['scheme'], | |
| return new self( | |
| $parsedDsn['scheme'], |
|
I think I'm 👎 here. We don't want to cover this with the BC policy. |
|
Instead, I'd suggest improving StoreFactory to make it able to create PdoStore and ZookeeperStore (check that all supported stores can be created by it actually) |
|
@nicolas-grekas So you are -1 with adding DSN object per component ? What can we do to improve the parsing of DSN ? this is not only the lock but messenger too see #32566 as another exemple. |
|
Closing as we won't merge it only for Lock. Still not convinced this is something we want/need. |
For the lock component we are using dsn for flock and for testing that we have DSN for redis and for memcache directly via the cache component, we could go one step futher and pass the dsn to the adapter directly. but that could be done in another PR.