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

[GH-8327] Deprecate EntityManagerHelper for a provider abstraction. #8524

Merged
merged 18 commits into from
Apr 18, 2021

Conversation

beberlei
Copy link
Member

@beberlei beberlei commented Mar 6, 2021

Deprecates the EntityManagerHelper in favor of a new EntityManagerProvider interface that comes with two implementation.

  • A BC abstraction wrapping the Console HelperSet HelpersetManagerProvider
  • a new SingleManagerProvider that provides access to a single entity manager.

In addition an implementation for the DBAL ConnectionProvider is provided that is based on any EntityManagerProvider and calling getConnection() on the EntityManager.

It is expected for the DoctrineBundle to create a new EntityManagerProvider that provides access to their multiple ORM entity managers via the Symfony DI Container.

Two deprecation messages are added:

  • When the HelperSetManagerProvider must be used from within ConsoleRunner, because a HelperSet is passed.
  • When an ORM command is used without explicitly passing the EntityManagerProvider as a dependency.

Todos:

Fixes #8327

@beberlei beberlei requested a review from greg0ire March 6, 2021 02:37
@beberlei beberlei added this to the 2.9.0 milestone Mar 6, 2021
public function getManager(string $name = 'default'): EntityManagerInterface
{
if ($name !== 'default') {
throw UnknownManagerException::unknownManager($name, ['default']);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that people define several managers but end up using SingleManagerProvider? If yes, that exception will be confusing I think.

Copy link
Member

Choose a reason for hiding this comment

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

Also, let's test this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No the SingleManagerProvider is for non Symfony context, where usually you have only one and you don't care about any of this. :)

@beberlei beberlei requested a review from greg0ire April 16, 2021 18:34
greg0ire
greg0ire previously approved these changes Apr 17, 2021
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

LGTM. The code suggestions I added are just that: suggestions

@dominikzogg
Copy link

@beberlei adding em option probably breaks every glue code out there similar to the doctrine bundle. This change should be in 3.0 not 2.9

@beberlei
Copy link
Member Author

@dominikzogg we speifically added code to prevent breaking in doctrine Bundle

@beberlei
Copy link
Member Author

@dominikzogg
Copy link

dominikzogg commented May 25, 2021

@beberlei as mentioned: there is code outside the symfony ecosystem out there

https://github.com/chubbyphp/chubbyphp-laminas-config-doctrine/pull/3/files and i am not even done yet => will lead to a major release on my side and when orm 3.0 gets released another major cause i am sure there will be (expected) bc's

@beberlei
Copy link
Member Author

Adding any parameter could have caused this, others might use entity-manager instead of em. Not sure how this could have been prevented, we won't change this now. Sorry to cause extra work on your end.

@dominikzogg
Copy link

dominikzogg commented May 26, 2021

Not sure how this could have been prevented

do it in 3.0, so nobody get forced to update code immediately

@beberlei
Copy link
Member Author

That ship has sailed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli-config.php compatibility with DBAL
3 participants