search

doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.86k stars 2.5k forks source link

Remove readonly modifier from EntityManager #11472

Closed nicolas-grekas closed 1 month ago

nicolas-grekas commented 1 month ago

The readonly modifier has been added as if it had no consequences, but in reality it breaks the possibility to reset the EM, while this is an important capability when using it in long-running processes.

This has the same reasoning as why @final is used and not the real final modifier.

Please see the discussion in https://github.com/symfony/symfony/issues/54228 for more detailed explanations.

nicolas-grekas commented 1 month ago

I added a test case that breaks if the unitOfWork property is made readonly or if the class is made final.

derrabus commented 1 month ago

The readonly modifier has been added as if it had no consequences, but in reality it breaks the possibility to reset the EM, while this is an important capability when using it in long-running processes.

FTR: Resetting the EM is a feature that the ORM neither advertised nor documented. Reopening an EM is explicitly not supported. I'm fine merging this change to unblock downstream projects. But this reset feature should be implemented properly in this repository. We cannot maintain or support features that are monkey-patched onto our code.

greg0ire commented 1 month ago

I will review this after I find time to read https://github.com/doctrine/orm/issues/5933

greg0ire commented 1 month ago

This does not fix a bug with Doctrine, and as such should target 3.2.x. We may release 3.2.0 quickly after this is merged as a courtesy to Symfony users.

nicolas-grekas commented 1 month ago

This does not fix a bug with Doctrine, and as such should target 3.2.x. We may release 3.2.0 quickly after this is merged as a courtesy to Symfony users.

Fine to me :rocket: :shipit: :ship:

curry684 commented 1 month ago

We may release 3.2.0 quickly after this is merged as a courtesy to Symfony users.

My mailbox highly appreciates this 😉

image

stof commented 1 month ago

@derrabus the feature request to move this feature to the ORM itself exists since 2016: https://github.com/doctrine/orm/issues/5933 Being able to reopen an EntityManager instance (after clearing it to remove the broken state of managed entities) is the only way you can use the ORM in a long running queue consumer as you don't want to kill your long-running process each time an exception happen during the handling of a message.

derrabus commented 1 month ago

I think, we both know the difference beware requesting a feature and implementing it. I'm aware that the request exists, bit somebody needs to do it.