Skip to content

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

Merged
beberlei merged 18 commits intodoctrine:2.9.xfrom
beberlei:GH-8327-cliconfig
Apr 18, 2021
Merged

[GH-8327] Deprecate EntityManagerHelper for a provider abstraction.#8524
beberlei merged 18 commits intodoctrine:2.9.xfrom
beberlei:GH-8327-cliconfig

Conversation

@beberlei
Copy link
Copy Markdown
Member

@beberlei beberlei commented Mar 6, 2021 •
edited
Loading

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
greg0ire
greg0ire reviewed Apr 2, 2021
View reviewed changes
lib/Doctrine/ORM/Tools/Console/EntityManagerProvider/SingleManagerProvider.php
public function getManager(string $name = 'default'): EntityManagerInterface
{
if ($name !== 'default') {
throw UnknownManagerException::unknownManager($name, ['default']);
Copy link
Copy Markdown
Member

@greg0ire greg0ire Apr 2, 2021

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
Copy Markdown
Member

@greg0ire greg0ire Apr 2, 2021

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
Copy Markdown
Member Author

@beberlei beberlei Apr 8, 2021

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. :)

greg0ire
greg0ire reviewed Apr 2, 2021
View reviewed changes
@beberlei beberlei requested a review from greg0ire April 16, 2021 18:34
greg0ire
greg0ire previously approved these changes Apr 17, 2021
View reviewed changes
Copy link
Copy Markdown
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

@beberlei beberlei merged commit dac87da into doctrine:2.9.x Apr 18, 2021
@beberlei beberlei deleted the GH-8327-cliconfig branch April 18, 2021 12:32
This was referenced Apr 18, 2021
Closed
Merged
@dominikzogg
Copy link
Copy Markdown

dominikzogg commented May 25, 2021

@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
Copy Markdown
Member Author

beberlei commented May 25, 2021

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

@beberlei
Copy link
Copy Markdown
Member Author

beberlei commented May 25, 2021

doctrine/DoctrineBundle@d6b3c37

@dominikzogg
Copy link
Copy Markdown

dominikzogg commented May 25, 2021 •
edited
Loading

@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
Copy Markdown
Member Author

beberlei commented May 25, 2021

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
Copy Markdown

dominikzogg commented May 26, 2021 •
edited
Loading

Not sure how this could have been prevented

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

@beberlei
Copy link
Copy Markdown
Member Author

beberlei commented May 26, 2021

That ship has sailed :)

@derrabus derrabus mentioned this pull request Apr 10, 2022
Merged
@dmaicher dmaicher mentioned this pull request Apr 20, 2022
Closed
@franmomu franmomu mentioned this pull request Dec 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greg0ire greg0ire greg0ire left review comments

Assignees

No one assigned

Labels

Deprecation

Projects

None yet

Milestone

2.9.0

Development

Successfully merging this pull request may close these issues.

cli-config.php compatibility with DBAL

3 participants

@beberlei @dominikzogg @greg0ire