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

Deprecate EntityManager::create() #9961

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Aug 4, 2022

Now that the entity manager's constructor is public, why not make it the only way to construct an EM?

The only additional feature that create() has is that you can pass the connection parameters instead of a real DBAL connection. However, I believe that bootstrapping the DBAL connection is a concern that belongs into the DBAL library and the DBAL's DriverManager class already does a fine job in that regard.

This is why I believe that we shouldn't need the create() method anymore.

@beberlei
Copy link
Member

beberlei commented Aug 4, 2022

Can you also check the docs for necesarry changes?

I am unsure about this. While your argument rings true, if we are on a greenfield project, we are not snd this method is not expensive to maintain, but will avoid breaking not only a lot of vode, but also tutorials, blog posts, books, ...

@derrabus derrabus marked this pull request as draft August 5, 2022 09:05
@derrabus
Copy link
Member Author

derrabus commented Aug 5, 2022

Marking as draft for now. The documentation indeed deserves an update.

breaking not only a lot of code,

I don't believe that this is the case, actually. Usually, the EM is initialized once at a central point in your application. This is the one piece of code that you need to apply a trivial fix to.

Even more though, in applications that use some kind of framework integration like DoctrineBundle. In that case, this integration code will be patched and people won't even notice.

but also tutorials, blog posts, books, ...

Those will all still work for ORM 2. Deprecated code is not broken code. ORM 3 will be a little different than ORM 2 and I think it's acceptable that blogposts and tutorials need to be adjusted for a new major version.

My rationale here is that the ORM and the DBAL are not as inseparable anymore as they maybe used to be. The ::create() method might not be hard to maintain right now, but it also means that the DBAL can't change its own bootstrap too much without breaking the ORM.

@derrabus derrabus modified the milestones: 2.13.0, 2.14.0 Aug 7, 2022
@derrabus derrabus changed the base branch from 2.13.x to 2.14.x August 7, 2022 18:22
@@ -156,15 +156,15 @@ class EntityManager implements EntityManagerInterface
* Creates a new EntityManager that operates on the given database connection
* and uses the given Configuration and EventManager implementations.
*/
public function __construct(Connection $conn, Configuration $config)
public function __construct(Connection $conn, Configuration $config, ?EventManager $eventManager = null)
Copy link
Member

Choose a reason for hiding this comment

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

The class-level phpdoc comment refers to the factory method your deprecated, please update it.

@SenseException
Copy link
Member

Is this still a draft or can it be changed into a PR?

@derrabus
Copy link
Member Author

Still a draft because I'm updating the docs atm.

@derrabus derrabus marked this pull request as ready for review October 23, 2022 20:54
@derrabus
Copy link
Member Author

Ready.

I chose to resurrect this PR because the latest deprecation in DBAL (removal of the EventManager integration) force us to change the create() method again. Let's remove the create() method, it's a burden.

@derrabus derrabus merged commit f3f4532 into doctrine:2.14.x Oct 23, 2022
@derrabus derrabus deleted the deprecate/em-create branch October 23, 2022 22:15
derrabus added a commit to derrabus/orm that referenced this pull request Oct 24, 2022
* 2.14.x:
  Deprecate EntityManager::create() (doctrine#9961)
  Address deprecation of SchemaDiff::toSaveSql()
  Address deprecation of SchemaDiff::toSql()
  Use error style for notifications
  Fix calls to AbstractSchemaManager::createSchema() (doctrine#10165)
  Fix build with DBAL 3.5 (doctrine#10163)
  Adjust comments (doctrine#10160)
  Deprecate methods related to the annotation driver
  Use correct link
  Deprecate annotations
  Remove trailing whitespace
  Migrate more documentation towards attributes
  Remove wrong sentence
  Do not export phpstan stuff (doctrine#10154)
@frsmoray
Copy link

Please update docs as official documentation on creating an EntityManager actually tells us to use the ::create() method. Not a single deprecation advice on that.

Thanks

@greg0ire
Copy link
Member

@frsmoray You are linking to the 2.13.x docs. This was perfectly valid in 2.13.x

Screenshot 2023-01-13 at 13-51-47 Advanced Configuration - Doctrine Object Relational Mapper (ORM)

Not a single deprecation advice on that.

Use a nicer tone, this is not OK.

@frsmoray
Copy link

frsmoray commented Jan 13, 2023

Sorry, mistakenly got pulled 2.14 onto my project. Expeting to be 2.13 as usual.

Use a nicer tone, this is not OK.

It's written text. Tone is a perception on spoken words. Apart from my mistake searching wrong docs, and 2.13 docs having nothing to do with any deprecation advice on that purpose, there is no deprecation notice at all there. If you think it twice, you may "find another tone" in these words

Not a single deprecation advice on that.

Thanks for the help :)

@greg0ire
Copy link
Member

Sorry, mistakenly got pulled 2.14 onto my project. Expeting to be 2.13 as usual.

2.13 is no longer maintained, so if you upgrade, that's not a bad things, really.

It's written text. Tone is a perception on spoken words.

Wanna nit-pick? From https://www.merriam-webster.com/dictionary/tone :

style or manner of expression in speaking or writing

(emphasis mine)

Note that I didn't have to discard several web pages until finding one that fits my argument, it's literally the first one I clicked.

there is no deprecation notice at all there

You're right, but what's your point? Our policy is to document those in UPGRADE.md, not in the docs themselves.

If you think it twice, you may "find another tone" in these words

I can do that, and on your end, you can be more careful when addressing people that provide you with software updates for free. I guess you meant well, but having to do that assumption for everyone can be a bit exhausting, don't you think?

@@ -127,7 +127,8 @@ the targetEntity resolution will occur reliably:
// Add the ResolveTargetEntityListener
$evm->addEventListener(Doctrine\ORM\Events::loadClassMetadata, $rtel);

$em = \Doctrine\ORM\EntityManager::create($connectionOptions, $config, $evm);
$connection = \Doctrine\DBAL\DriverManager::createConnection($connectionOptions, $config, $evm);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this supposed to be getConnection? There doesn't seem to be a createConnection function in this class

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

@SenseException SenseException Dec 4, 2023

Choose a reason for hiding this comment

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

Yes, it became getConnection by now. It's totally okay if you want to create a pull request with changes for that.

If you haven't done a pull request for Doctrine, you might find this guide helpful:
https://www.doctrine-project.org/contribute/index.html

Copy link
Contributor

Choose a reason for hiding this comment

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

See #11104

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.

6 participants