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

Drop support for EntityManager#flush($something) #6118

Closed
Ocramius opened this issue Nov 5, 2016 · 15 comments
Closed

Drop support for EntityManager#flush($something) #6118

Ocramius opened this issue Nov 5, 2016 · 15 comments

Comments

@Ocramius
Copy link
Member

Ocramius commented Nov 5, 2016

EntityManager#flush($something) is a problematic operation, since it leads to consumers potentially storing incomplete in-memory graphs into the DB.

In general, flush($something) should be used as an optimization, but dealing with it from within the ORM has only led to bugs and massive headaches so far, whereas keeping a smaller UnitOfWork is trivial from the consumer perspective.

Therefore, I suggest dropping this ability completely, while consumers should instead approach batch-processing problems with something like:

@chalasr
Copy link

chalasr commented Nov 5, 2016

👍 I too often seen changes made on the associations of an entity not took into account because of the use of EntityManager#flush($entity).
On the other hand it's often scaring to don't specify the entity, because some other entities may be unexpectedly impacted, even if having entities in invalid states should never occur...

create new EntityManager instances, and keep them small

Not always easy e.g. in symfony where the manager is default a shared service which is injected everywhere, that is imho the cause of lots of issues.

use EntityManager#clear() more often

Looks like a good solution, it's more explicit what you're doing.

@Jean85
Copy link
Contributor

Jean85 commented Nov 5, 2016

In my team, we use a lot of this, to avoid flushing something else that got fetched from the DB and I don't want to change right now. This is an issue when the code base is large and there are a lot of (Symfony) events and listeners, and it's hard to keep track of everything. I agree that clear() is the right solution with batch operations, and in fact we use that a lot, since it has the important benefit of freeing memory.

I'm interested in the suggestions for the alternative approaches, I will look more into it on Monday with more code at hand.

@pablomoreno61
Copy link

I've had unexpected entities persisted because of not using $entity inside the flush operation, so nowadays I use it in all cases.

Your alternatives look interesting, I will investigate them

@Ocramius
Copy link
Member Author

Ocramius commented Nov 5, 2016

The EntityManager#flush() method makes no guarantees about what ia being
flushed: it just reduces computed changesets

On 5 Nov 2016 15:55, "Pablo Moreno" notifications@github.com wrote:

I've had unexpected entities persisted because of not using $entity inside
the flush operation, so nowadays I use it in all cases.

Your alternatives look interesting, I will investigate them


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#6118 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakPHQ2PM2AzqBJWmUaGtZiVTq7P1Uks5q7JjVgaJpZM4KqNo8
.

@baptistemanson
Copy link

We use it in a ZF2 daemon listening to a queue. It is the only way we found to get rid of all the in memory objects to retrieve them from the db regularly.

@yannickl88
Copy link
Contributor

yannickl88 commented Nov 5, 2016

We do the same as @Jean85 within our company. So I'm really interested in the alternatives. A way to create a "sub-entity manager" (same connection, same config, new UnitOfWork) would pretty much solve the issue since each listener can use its own entity manager. Thus it cannot influence the other state.

@alcaeus
Copy link
Member

alcaeus commented Nov 5, 2016

I'm interested in the suggestions for the alternative approaches, I will look more into it on Monday with more code at hand.

The most obvious one would be to use explicit change tracking instead of the default implicit change tracking. In a nutshell, when you use explicit change tracking and you call flush(), no entities will be updated. Every entity you want to be updated has to be marked for update with persist() (as if it were a new entity). Note that change tracking can be specified on a per-entity basis, so you could actually store some entities the old-fashioned way.

When it comes to event subscribers that write entities to the database, you should most likely use a separate EntityManager. Even when calling flush($entity), the UnitOfWork would still write new documents to the database. All you're doing is circumventing the implicit change tracking mentioned above.

I'm all for removing this - the comments in this thread as well as on Twitter show quite well that many developers misunderstand what the feature does.

@drola
Copy link

drola commented Nov 6, 2016

After this change happens we need to make sure that flush() method still checks its argument count and throws an exception if there is an argument given. Ignoring the argument silently will lead in hard to catch bugs in people's codebases.

@Ocramius
Copy link
Member Author

Ocramius commented Nov 6, 2016

Very good point, Matjaž!

On 6 Nov 2016 11:10, "Matjaž Drolc" notifications@github.com wrote:

After this change happens we need to make sure that flush() method still
checks its argument count and throws an exception if there is an argument
given. Ignoring the argument silently will lead in hard to catch bugs in
people's codebases.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#6118 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakJHB1IQUKYahmukladdiW5pI54tCks5q7aeFgaJpZM4KqNo8
.

@sstok
Copy link

sstok commented Nov 6, 2016

@yannickl88 If I'm not mistaken you "can" configure multiple entity-managers in a Symfony application, but I have not seen many developers use it. It would make sense to give each bounded context it's own EntityManager rather then sharing this with other contexts.

@yannickl88
Copy link
Contributor

@sstok Yes you can, but it will require a bunch of extra config. Think of configurations which use auto-mapping and such. Also, this is even more difficult in shared Symfony bundles. Having a way to simply do something like $sub_manager = $entity_manager->createSubManager() would be so much easier.

@Jean85
Copy link
Contributor

Jean85 commented Nov 7, 2016

@yannickl88 yeah that would be lovely, but it's not feasible without a lot of dark magic, since you can be inside a transaction and would lead to a mess... How would you control it? Who would have the responsibility of committing the transaction?

toooni added a commit to toooni/awesome-doctrine that referenced this issue Feb 13, 2017
There are currently plans to drop support for `flush($entity)`. So a recommendation would lead people to implement something they have to refactor pretty soon: doctrine/orm#6118
@ihorsamusenko
Copy link

@Jean85 aren't we allowed to have nested transactions? The "subManager" would open his own transaction and commit it.

@sstok
Copy link

sstok commented Mar 26, 2017

A nested transaction is actually a safe point, and not all vendors (couch (older) MySQL versions) support this properly.

lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Jul 28, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Jul 28, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Jul 28, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
lcobucci pushed a commit to lcobucci/doctrine2 that referenced this issue Dec 31, 2017
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
maglnet pushed a commit to maglnet/doctrine2 that referenced this issue Oct 10, 2018
@greg0ire greg0ire removed this from the 3.0.0 milestone Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests