[Dsn] New component #24267

jderusse · Sep 19, 2017

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	NA
License	MIT
Doc PR	TODO

A new Component to handle DSN and retrieve a parameterized connection.
Such logic is actually duplicated in Cache and Lock component.

Other DSN could be handled in the future like pdo

When merged, the next steps are (can not be done in this PR because coimpoent is not released and not available in packagist => could not be used as a dependency)

use this component in cache and lock
deprecate createConnection from Cache adapters

TODO

find usage of other Dsn in the project
add an option class in MemcachedFactory too

javiereguiluz · Sep 21, 2017

@jderusse thanks for submitting this proposal!

I have a quick question about the long-term vision of this component. Would it also handle the other DSN used in modern Symfony apps: Doctrine DBAL DSN and Swiftmailer DSN? And would it also be useful for the new Queue/AMPQ components that will be included in the future?

jderusse · Sep 21, 2017

@javiereguiluz yes, the purpose of this component is to centralize usage of DSN parsing. It could be LDAP too

Moreover, with the usage of env variables over arameters.yml, Dsn would be a better alternative to declare endpoint

# before
doctrine:
    dbal:
        driver: "pdo_mysql"
        host: "%env(DATABASE_HOST)%"
        port: "%env(DATABASE_PORT)%"
        dbname: "%env(DATABASE_NAME)%"
        user: "%env(DATABASE_USER)%"
        password: "%env(DATABASE_PASSWORD)%"
        charset: UTF8
        server_version: 5.6


# after
doctrine:
    dbal:
        dsn:     "%env(DATABASE_DSN)%"
        charset:  UTF8
        server_version: 5.6

# + helper if you don't want to use env for the entier DSN
parameters:
  env(DATABASE_DSN): mysql://root:%env(DATABASE_PASSWORD)%@%env(DATABASE_HOST)%/my_app

This PR was about Redis & Memcached because code is already here, and is just about copy/paste.
And I planned to migrate other ConnectionFactories in dedicated PR to simplify the review (Wath do you think)

OskarStark · Sep 22, 2017

src/Symfony/Component/Dsn/Factory/MemcachedConnectionFactory.php

+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class MemcachedConnectionFactory


shouldn't they implement an interface?

each factory has a different return type, so I see no point creating an interface (the return type would have to be mixed

the input could also vary (string in redis, array or string in memchached).

OskarStark · Sep 22, 2017

src/Symfony/Component/Dsn/Exception/ExceptionInterface.php

+namespace Symfony\Component\Dsn\Exception;
+
+/**
+ * Interface for exceptions.


this comment is uselesse

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/ConnectionFactory.php

+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+class ConnectionFactory


please make it final

👎 for having everything static here

although we don't usually make things final by default, I think we should here, because there is no "type" to inherite from, only behavior to composer.
So 👍 for final.

👎 for non-static, there is no OOP here, just constructors.

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/ConnectionFactory.php

+        }
+        if (0 === strpos($dsn, 'memcached://')) {
+            return static::TYPE_MEMCACHED;
+        }


this implementation is hard to extend / maintainance
when we will want to have 3rd and 4th dsn type, you would just add more if's, but when one wants to use component from outside, he won't have nice way to add more guessers.

what about some indicator classes created and registered in factory class as a list, each of them specialized in recognizing only one type and answering "yes" or "no" ? then, here we would just iterate over the list

Instead of hardcoding a list of supported Factories, an other solution would be ot use instances (instead of static calls), definig a common interface with 2 methods supports and createConnection(mixed, array): mixed and use a strategy pattern to iterrate over a list of injected factories.

do we really need OOP in factories?

interfaces with mixed/mixed seems useless

then at least mark this class as final, maybe even @internal

Adding guesser on user's side is not needed: these are factories, they don't adhere to any contract, exactly like constructors. Instead, they ship behaviors, that users would then wire in their DI layer.
If they have more DSN factories, they'll use these factories directly. No need to create any OOP design here, that'd we completely useless and wouldn't solve any problem, just create new ones.

I red your comments from 15 minutes ago. and especially for that:

then at least mark this class as final, maybe even @internal

@internal would defeat the purpose of the component by stating "don't use it" to users.

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/ConnectionFactory.php

+class ConnectionFactory
+{
+    const TYPE_REDIS = 1;
+    const TYPE_MEMCACHED = 2;


if this would be logged anywhere for some reason, 2 is magic value. I advice to make value matching constant, like MEMCACHED here

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/Factory/MemcachedConnectionFactory.php

+/**
+ * Factory for Memcached connections.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>


it's new code. is he an author here indeed?

this is code extracted from the Cache component, so he is.

cool. then we shall use this in Cache component ;)
lack of that confused me

please read the PR description. It says it is the next step.

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/Factory/MemcachedConnectionFactory.php

+     *
+     * @throws \ErrorException When invalid options or servers are provided
+     */
+    public static function createConnection($dsns, array $options = array())


👎 for 100+ lines in one method. please decouple

keradus · Sep 22, 2017

src/Symfony/Component/Dsn/Tests/Factory/MemcachedConnectionFactoryTest.php

+    /**
+     * @dataProvider provideBadOptions
+     * @expectedException \ErrorException
+     * @expectedExceptionMessage constant(): Couldn't find constant Memcached::


using annotations is considered as bad practice for expectations.
please use ->expectException (or ->setExpectedException if lower phpunit is needed)

The issue is we would have to use both of them (as lower PHPUnit is needed for older PHP versions, but PHPUnit 6 removed expectException entirely).
The annotation works in all versions.

And we use them in most places in the Symfony testsuite btw.

PHPUnit 6 removed setExpectedException, not expectException.
Is there a need to support PHPUnit 4, 5, and 6 ?

yes, due to PHP versions being supported (PHPUnit 5 does not support PHP 5.5, which is our min version in Symfony 3.x, and so must be covered in CI)

nicolas-grekas · Sep 24, 2017

do we really need OOP in factories?

clearly no, these are factories, they belong to the same world as constructors: no contract there.

mvrhov · Sep 26, 2017

I'm against any kind of DSN, and I find it bad DX because every time I have to look up on how to build it up. If every property stands for itself, then I can simply call config:debug <bundle>

nicolas-grekas

LGTM, let's create this component asap to unlock the corresponding PRs on Cache and Lock

OskarStark · Sep 26, 2017

src/Symfony/Component/Dsn/Tests/ConnectionFactoryTest.php

+
+    /**
+     * @dataProvider provideInvalidDsn
+     * @expectedException InvalidArgumentException


should be FQCN

OskarStark · Sep 26, 2017

src/Symfony/Component/Dsn/Tests/ConnectionFactoryTest.php

+
+    /**
+     * @dataProvider provideInvalidDsn
+     * @expectedException InvalidArgumentException


should be FQCN

jderusse · Sep 28, 2017

I just merged PR #24323 with this one. It'll help to understand the problem that this new component try to solve.

nicolas-grekas · Sep 28, 2017

src/Symfony/Component/Dsn/README.md

+Dsn Component
+=============
+
+Provides utilities for handle Data Source Names.


for creating connections using DSN (Data Source Names) strings.

nicolas-grekas · Sep 28, 2017

About the name of the component, if "dsn" is not clear enough, what about "dsn-factories"? or "connection-factories"?

nicolas-grekas · Sep 28, 2017

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -77,6 +77,7 @@
    "suggest": {
        "ext-apcu": "For best performance of the system caches",
        "symfony/console": "For using the console commands",
+        "symfony/dsn": "For using the configuring locks and cache with Dsn",


I think this should be moved to "require", so that DSN works out of the box in fwb.

at least must be in require-dev, so that tests can pass

moved into require section

nicolas-grekas · Sep 28, 2017

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -1704,9 +1705,13 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
                        break;
                    case $usedEnvs || preg_match('#^[a-z]++://#', $storeDsn):
                        if (!$container->hasDefinition($connectionDefinitionId = $container->hash($storeDsn))) {
+                            if (!class_exists(ConnectionFactory::class)) {
+                                throw new LogicException('Lock stores cannot be configured as the Dsn component is not installed.');


should be removed once/if dsn is added as a requirement in composer.json

removed (same for CachePoolPass)

nicolas-grekas · Sep 28, 2017

src/Symfony/Component/Cache/Adapter/AbstractAdapter.php

+
+        switch ($type) {
+            case ConnectionFactory::TYPE_MEMCACHED:
+                return MemcachedConnectionFactory::createConnection($dsn, $options);


bad name, this has been renamed now

OskarStark · Sep 28, 2017

src/Symfony/Component/Dsn/README.md

@@ -0,0 +1,13 @@
+Dsn Component


- Dsn Component + DSN Component

The component's name is really Dsn Component. See @javiereguiluz comment

OskarStark · Sep 28, 2017

About the name of the component, if "dsn" is not clear enough, what about "dsn-factories"? or "connection-factories"?

👍 for dsn-factories

javiereguiluz · Sep 28, 2017

dsn-factories or connection-factories don't look like component names to me. But maybe it's because this doesn't look like a "real component" either? (similar to what Fabien said). It's very useful to avoid repeating code in Lock and Cache ... but is it useful outside of that?

nicolas-grekas · Sep 28, 2017

It's very useful to avoid repeating code in Lock and Cache ... but is it useful outside of that?

I personnaly it is, because Loch & Cache are not the only use cases of Redis/memcached connections, and this allows creating them easily using DSN, for any other use case. Just my 2cts :)

fabpot · Sep 28, 2017

Having the other PR here does not change my point of view. This is not about a DSN component. Changing the name might make it a bit better, but still, I've a hard time thinking about this "code sharing" between several component as being a component by itself. Still 👎 about this change, and certainly 👎 for rushing it just before code freeze.

nicolas-grekas · Sep 28, 2017

OK, so let's close this.
Instead, I propose to remove the DSN-factories from Lock altogether. We don't need them for the FrameworkBundle integration since the ones in Cache are just fine for Lock. Let use them for now, and maybe forever.
If someone has another idea to remove the code duplication, please share.

This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Use cache connection factories in lock | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no (feature removal) | BC breaks? | no (if merged in 3.4) | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | An alternative to #24267 to share code between cache and lock. Commits ------- 95358ac Share connection factories between cache and lock

This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Use cache connection factories in lock | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no (feature removal) | BC breaks? | no (if merged in 3.4) | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | An alternative to symfony/symfony#24267 to share code between cache and lock. Commits ------- 95358ac98f Share connection factories between cache and lock

This PR was merged into the 3.4 branch. Discussion ---------- [DoctrineBridge] Deprecate dbal session handler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no  | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20501 | License | MIT | Doc PR | The DbalSessionHandler misses all the improvements from the PdoSessionHandler (lock modes, delayed GC, configurable naming). The only advantage it had was the ability to work with non-pdo drivers. But since DBAL requires PDO now as well (doctrine/dbal@36df682) even that is not really relevant anymore. Stofs argument in #20501 (comment) sound like a new feature that can be implemented separately. Ref. #24267 Commits ------- ffc74eb [DoctrineBridge] Deprecate DbalSessionHandler

carsonbot added Status: Needs Review Feature labels Sep 19, 2017

jderusse force-pushed the dsn branch 3 times, most recently from 66d1891 to 6802f09 Compare September 19, 2017 21:58

jderusse changed the base branch from master to 3.4 September 19, 2017 21:59

jderusse force-pushed the dsn branch 3 times, most recently from 0a9c8f2 to 1beaa92 Compare September 19, 2017 22:14

nicolas-grekas added this to the 3.4 milestone Sep 20, 2017

jderusse force-pushed the dsn branch from 1beaa92 to 10ea36a Compare September 20, 2017 06:54

OskarStark reviewed Sep 22, 2017

View reviewed changes

keradus reviewed Sep 22, 2017

View reviewed changes

jderusse force-pushed the dsn branch 2 times, most recently from ad501b1 to df8a505 Compare September 22, 2017 16:44

jderusse force-pushed the dsn branch 2 times, most recently from a17f50e to 668c870 Compare September 24, 2017 15:46

nicolas-grekas approved these changes Sep 26, 2017

View reviewed changes

OskarStark reviewed Sep 26, 2017

View reviewed changes

jderusse force-pushed the dsn branch from d371004 to b0b2c49 Compare September 26, 2017 10:47

jderusse added 2 commits September 28, 2017 10:45

Rename factories names

d0c6907

Use Dsn component to create connections in cache

6c4d771

jderusse force-pushed the dsn branch from 1561f7f to 0546bab Compare September 28, 2017 08:47

nicolas-grekas reviewed Sep 28, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Sep 28, 2017

[Cache][Lock] Use Dsn component to create connections #24323

Closed

nicolas-grekas reviewed Sep 28, 2017

View reviewed changes

OskarStark reviewed Sep 28, 2017

View reviewed changes

jderusse force-pushed the dsn branch from 0546bab to 7134242 Compare September 28, 2017 09:35

Use DSN for lock configuration too

b0c2ee3

jderusse force-pushed the dsn branch from 7134242 to b0c2ee3 Compare September 28, 2017 10:06

nicolas-grekas closed this Sep 28, 2017

jderusse mentioned this pull request Sep 28, 2017

[Lock] Use cache connection factories in lock #24366

Merged

Tobion mentioned this pull request Oct 1, 2017

[DoctrineBridge] Deprecate dbal session handler #24389

Merged

jderusse mentioned this pull request Jul 11, 2019

[Messenger] Doctrine Transport: Support setting auto_setup from DSN #32392

Merged

Simperfit mentioned this pull request Jul 15, 2019

[DSN] Add a new DSN component #32546

Closed

jderusse deleted the dsn branch August 2, 2019 12:14

OskarStark mentioned this pull request Aug 16, 2019

[Lock] add the DSN object #33200

Closed

Nyholm mentioned this pull request May 28, 2020

New DSN component #36999

Closed

3 tasks

Search code, repositories, users, issues, pull requests...

Uh oh!

[Dsn] New component #24267

[Dsn] New component #24267

Uh oh!

Conversation

jderusse commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented Sep 21, 2017

Uh oh!

jderusse commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 24, 2017

Uh oh!

mvrhov commented Sep 26, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jderusse commented Sep 28, 2017

Uh oh!

jderusse commented Sep 19, 2017 •

edited

Loading

nicolas-grekas Sep 24, 2017 •

edited

Loading

keradus Sep 22, 2017 •

edited

Loading

nicolas-grekas Sep 28, 2017 •

edited

Loading