-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DSN] Add a new DSN component #32546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
See #31946 for some idea how to implement DSN for Mailer. I have plans do same for Messenger and after that consider extract it as separate component |
@Koc That's what I have in mind but for all of symfony, if your PR gets merged before we statued on this, we could take some of your work to begin the |
-1 because there is no clear spec for DSNs, which are implementation-specific. More precisely, ODBC, DBD, JDBC, native drivers, etc. handle DSNs in a different manner, meaning that there is a huge number of edge cases. |
As a first step, perhaps having a |
Why not create the |
Well yes, as far as I understand it's not about creating one type of DSN but simply having a class that components can use to parse a DSN. Components that require a very specific DSN handle it on their own anyway as they are edge cases like you said. So it serves as a base, a quick way to implement DSN style configuration for components that benefit from it. Messenger and mailer are good examples. |
I agree with @fabpot's position: a Dsn class in each component should be good. What I meant is that I do not see the point in a Dsn component, living on its own. |
I guess it could live in the framework bundle? |
The interest of creating a small component is mainly to allow the reuse of this logic to facilitate the creation of services through a DSN (at least have a base, and add customization based on specific features). Given that there are at least 2 components (Mailer and Messenger) using the same logic, we should avoid duplicating code, but I understand very well that it is an additional component to maintain. There was also a class in the Form component for pluralizing strings, this one ended up in a new So concerning the creation or not of the DSN component, we have the same case, and as Symfony is moving more and more towards the DSN configuration, it would be interesting for all third-party components to have the same basis for also proposing the creation of service via a DSN configuration. It will also be beneficial for the bug fix, because for example, a correction has been done on the URL decoding of the user and password parameters in the Mailer component, but this problem exists in all other classes creating a service from a DSN ; example with the Messenger transports: AmqpExt and RedisExt. In the end, the bug fix for the Mailer component is not reflected on the other components, while if they use the same basic component, this problem would have been immediately corrected for all components (by the way, it would be necessary to create an issue concerning this problem in the transports of the Messenger component when the username and/or the password has special characters and are therefore encoded). @mdeboer If it is used by the Mailer and Messenger components, it will not be possible. |
Don't get me wrong, I'm the one who suggested to have such a DSN component in the first place. But before coding, I really think we need to check that we will be able to cover all use cases. |
@fabpot What do you think of starting it in the FrameworkBundle, using it in all components and check if we cover all our use case, if we do, then we can check if it can leave alone ? |
@Simperfit I don't think having the FrameworkBundle as a dependency in our components is the way to go. This would completely contradict the direction of dependencies as they are right now (i.e. introducing circular dependencies). |
@xabbuh I don't see how we could not duplicate code if we don't make it live in a single place and not in a self working component. |
Well configuration of components is done within the FrameworkBundle so I don't see why components would need to depend on the Framework bundle. In fact, they don't really need to know about a DSN component right now. So focus on adding DSN functionality for applications using the FrameworkBundle first then later move it to a component and make other components depend on that to bring functionality to component-only applications as well. |
Thank you for this suggestion. |
Yes @carsonbot, I would ;) |
As far as I can see, the ideas of this issue have been proposed in #36999. Ultimately, that PR was closed as the number of different DSNs is too big to benefit from a single abstraction. So I think it's best to close this issue (but of course, feel free to comment if you disagree after reading the PR discussion). |
Description
Following this comment from @fabpot: #32392 (comment) and searching for information in the symfony repository
I've seen an attempt from @jderusse #24267. I still think the proposal was a good idea and we need a DSN component, we could even imagine a DSN contract, let me explain why:
I've looked into all of the symfony components to look for DSN usage, it seems that we use lot's of DSN and even in client project we use more and more DSN.
Lot's of symfony-linked librairies (doctrine, flysystem) use a DSN too, so instead of rechecking and reparsing the DSN all the time, we could have a simple
dsn-parser
that would do the job and return a DSN parsed as a PHP array/object.If we talk about usage in symfony:
In the
Cache
💾 component where are parsing two time theredis
DSN:In the redis trait:
symfony/src/Symfony/Component/Cache/Traits/RedisTrait.php
Lines 84 to 90 in 9ab4f14
And in the adapter:
symfony/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
Lines 136 to 141 in 9ab4f14
We are also parsing a memcache DSN in the adapter.
Both of the redis check could be merged into one and an object could be passed between the two. The exception logic would be merge an a new exception would be thrown.
In the
Messenger
📮 component we are using DSN in connections classes and in the supports method, maybe a trait comming from the dns parser would help parsing this directly. I give you the exact lines of the code that each connection classes is doing but we know that's Doctrine AMQP, and redis. As an exemple we will only take doctrine.symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php
Lines 68 to 70 in 9ab4f14
In
HttpKernel
📟 we parse a dsn too, that's the file DSN, it could be reworked to avoid parsing it in here and just getting the value without using substr:symfony/src/Symfony/Component/HttpKernel/Profiler/FileProfilerStorage.php
Lines 37 to 39 in 9ab4f14
In multiple places we parse
PDO
connection DSN with a throw if it's not a DSN, this could be all in the same places it will ease the maintenance and creating new adapter based on PDO:symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php
Lines 181 to 182 in 9ab4f14
symfony/src/Symfony/Component/Cache/Traits/PdoTrait.php
Lines 57 to 59 in 9ab4f14
And the
PdoCache
it uses the PdoTraitIn
HttpFoundation
📡 we them found that big method that is parsing all kind of connection based sql DSN, this could totally be in the new DSN parser or in an dsn-sql-adapter ;).symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php
Lines 445 to 530 in 9ab4f14
In the
Lock
🔒component we parse a DSN too in order to see what store we will instantiate:symfony/src/Symfony/Component/Lock/Store/StoreFactory.php
Lines 54 to 64 in 9ab4f14
And the
Mailer
components parses DSN too, the most widely used config param of the component is a DSN, all of this could simply in the DSN parser and will throw better exception when something is wrongsymfony/src/Symfony/Component/Mailer/Transport.php
Lines 61 to 71 in 9ab4f14
As far I have looked into the code ans searched for DSN usages, I think we can really do something to avoid parsing all the DSN and having a system that will do it for us, to return the right types, the rights errors, the right scheme to have normalized error messages and interface on how we can parse DSN. It would avoid having the DSN parsed two times like in the RedisAdapter and in the RedisTrait.
Talking about symfony ecosystem (like doctrine for exemple) it will help too because since using DSN has been introduced widely when we use env var we can normalize on how we work with DSN.
Example
Like I said in the listing before, we could imagine the whole parsing logic being in the same place with a parser and them multiple connection like we've done with the mailer.
before would still be:
symfony/src/Symfony/Component/Cache/Traits/RedisTrait.php
Lines 84 to 90 in 9ab4f14
after it could looks like this (draaaft)
Either we inject the DsnParser or we use a static method that will return a Dsn Object:
Static version:
The object version is quite the same but we use the DependencyInjection to inject an instance of the DsnParser in the classe and to run parse on it.
Maybe we could even imagine a DSN component that contains all the connection that we have in symfony and all of them will be only in the DSN component, either we build a parser with different adapter either we build a whole connection object.
What do you think of this new component ?
The text was updated successfully, but these errors were encountered: