Skip to content
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

Allow to typehint ConnectionRegistry #936

Closed
wants to merge 1 commit into from
Closed

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Mar 24, 2019

This would allow to typehint the ConnectionRegistry interface to be able to get a connection.

@@ -64,6 +64,7 @@
<argument>%doctrine.default_entity_manager%</argument>
</service>
<service id="Doctrine\Common\Persistence\ManagerRegistry" alias="doctrine" public="false" />
<service id="Doctrine\Common\Persistence\ConnectionRegistry" alias="doctrine" public="false" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much against adding more interfaces from doctrine/persistence and tying them to ORM. Existing aliases will be dropped in 2.0, see #870. Do we have other options here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another bundle? :/ What's the reasoning behind not having interfaces from doctrine/persistence in this bundle? 🤔 If ORM changes and doctrine is not anymore the service holding the ConnectionRegistry anymore, we "just" have to release a new version of this bundle isn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ORM isn’t the only mapper. The surrounding interfaces will be removed in 2.0 as the assumption that any interface from persistence should be owned by ORM goes out the door once another mapper (e.g. MongoDB ODM) is used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We solved this in general up until now by having an sub-interface that’s ORM-only iirc.

But if that doesn’t exist, it is tricky: not using these interfaces for autowiring optimizes for the 1% that use ORM and ODM simultaneously.

We could use the named autowiring thing that was added in 4.2 to make it ConnectionRegistry $dbalRegistry (where you need the type + arg name to match)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the named autowiring thing that was added in 4.2 to make it ConnectionRegistry $dbalRegistry (where you need the type + arg name to match)

But it wouldn't be a good DX for the 99% :/

@Majkl578
Copy link
Contributor

Besides what @alcaeus said, I'd rather prefer deprecating ConnectionRegistry than giving it more meaning.

@weaverryan
Copy link
Contributor

Could the user instead already use the RegistryInterface from the Symfony Bridge to get the connection?

@alcaeus
Copy link
Member

alcaeus commented Mar 28, 2019

Yes, the ConnectionRegistry interface is extended by the ManagerRegistry interface, so this shouldn’t be too much of a problem.

@sroze did you have a particular use-case for adding this?

@sroze
Copy link
Contributor Author

sroze commented Mar 28, 2019

Just wanted to get a connection by its name so thought I had to inject ConnectionRegistry. I'm happy if ManagerRegistry has the methods I need... but it wasn't straight-forward to me.

@alcaeus
Copy link
Member

alcaeus commented Mar 28, 2019

Good to see it worked. I'll take a look at ConnectionRegistry and how to proceed with it. Closing here in the meantime.

@alcaeus alcaeus closed this Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants