Cascade detach should detach lazy collections too#11717
Cascade detach should detach lazy collections too#11717goetas wants to merge 2 commits intodoctrine:2.20.xfrom
Conversation
d7d1e4c to
4c3c8f0
Compare
4c3c8f0 to
56ce570
Compare
| if ($this->isInIdentityMap($entity)) { | ||
| $this->removeFromIdentityMap($entity); | ||
| } | ||
| $state = $this->getEntityState($entity, self::STATE_DETACHED); |
There was a problem hiding this comment.
I had to make this changes because "child" objects should be detached first, and then the parents (otherwise the IDs are not available for some DBs as sqlite)
There was a problem hiding this comment.
I am afraid this is going to break some people event listeners :)
We should add this the UPGRADE notes to warn people. We don't guarantee the order so its not a BC break, but a heads up is always nice.
There was a problem hiding this comment.
Your example is describing that there shouldn't be an SQL query triggered for addresses when detached. Shouldn't the test make sure that this is the case?
$adr1 = $user->addresses[0]; // no SQL triggered at this pointWhen I run $this->getQueryLog() after $ad = $user->addresses[0], I find the following SQL query in the logger:
SELECT t0.id AS id_1, t0.data AS data_2, t0.user_id AS user_id_3 FROM LazyEagerCollectionAddress t0 WHERE t0.user_id = ?If I understood the PR correctly we have two goals: detach the lazy stuff and prevent an SQL query for the lazy objects. Am I understanding it correctly that this query shouldn't happen in the test?
beberlei
left a comment
There was a problem hiding this comment.
You found a tricky problem, thanks for that!
but we should improve the implementation to be more performance sensitive imho, wdyt?
| if ($this->isInIdentityMap($entity)) { | ||
| $this->removeFromIdentityMap($entity); | ||
| } | ||
| $state = $this->getEntityState($entity, self::STATE_DETACHED); |
There was a problem hiding this comment.
I am afraid this is going to break some people event listeners :)
We should add this the UPGRADE notes to warn people. We don't guarantee the order so its not a BC break, but a heads up is always nice.
| $relatedEntities = $relatedEntities->unwrap(); | ||
| // break; is commented intentionally! | ||
|
|
||
| // in order to detach the entities in the collection, initialization is needed (no unwrap) |
There was a problem hiding this comment.
This should be documented in the docs, because it could lead to the operation becoming very expensive.
It should also be documented in upgrade notes.
You could also implement a shortcut here, if no entity of $assoc entity is in the identity map, then loading the collection from database will not yield any benefit.
For both lazy and extra lazy you could also make a query to load only the IDs, then check if they are in the identity map to detech. This would make the query + loading cheaper. The reaosn is that the entities loaded here are immediately going out of scope and garbage collected anyways.
|
@beberlei thanks for the review and i'm sorry for the late reply. I plan to continue working on this in the upcoming weeks |
|
There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
|
This pull request was closed due to inactivity. |
This is a follow up of #10065
When a collection is "LAZY", it is not being detached when
detachis called.In order to detach all the entities, the collection needs to be initialized (similarly to what is done for
remove()).To give a more real example:
Given this class:
before my change
after my change