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

Remove readonly modifier from EntityManager #11472

Merged
merged 1 commit into from
May 23, 2024

Conversation

nicolas-grekas
Copy link
Member

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 symfony/symfony#54228 for more detailed explanations.

@nicolas-grekas nicolas-grekas changed the title Remove reaonly modifier from EntityManager Remove readonly modifier from EntityManager May 23, 2024
@nicolas-grekas nicolas-grekas force-pushed the no-readonly branch 2 times, most recently from d259d40 to b6dbe52 Compare May 23, 2024 08:56
@nicolas-grekas
Copy link
Member Author

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

derrabus
derrabus previously approved these changes May 23, 2024
@derrabus derrabus requested a review from greg0ire May 23, 2024 09:18
@derrabus
Copy link
Member

derrabus commented May 23, 2024

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

I will review this after I find time to read #5933

tests/Tests/EntityManagerTest.php Outdated Show resolved Hide resolved
src/EntityManager.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

greg0ire commented May 23, 2024

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.

@derrabus derrabus changed the base branch from 3.1.x to 3.2.x May 23, 2024 12:04
@derrabus derrabus dismissed their stale review May 23, 2024 12:04

The base branch was changed.

@derrabus derrabus added this to the 3.2.0 milestone May 23, 2024
@nicolas-grekas
Copy link
Member Author

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 🚀 :shipit: 🚢

@curry684
Copy link

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

My mailbox highly appreciates this 😉

image

@greg0ire greg0ire merged commit 37946d3 into doctrine:3.2.x May 23, 2024
64 checks passed
@stof
Copy link
Member

stof commented May 23, 2024

@derrabus the feature request to move this feature to the ORM itself exists since 2016: #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
Copy link
Member

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.

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