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

Snapshot persistent collection before clearing. #1034

Closed
wants to merge 5 commits into from
Closed

Snapshot persistent collection before clearing. #1034

wants to merge 5 commits into from

Conversation

dazorni
Copy link

@dazorni dazorni commented Feb 26, 2015

This PR aims to change the way a persistent collection is cleared.
Currently, calling clear() will clear the collection and take a snapshot, which leaves event subscribers unable to work with objects that would be removed from the collection (e.g. reference counting). To avoid deleting all elements of the collection one by one, and retain the original clear functionality, clear() no longer makes a snapshot after clearing the collection and instead makes one before. This way an event subscriber can work with all objects that are removed from the collection, while still providing the original functionality in getInsertDiff() and getDeleteDiff().

@sharpner
Copy link
Contributor

sharpner commented Mar 4, 2015

+1

@jwage
Copy link
Member

jwage commented Mar 28, 2015

I think this behavior change would be different from the ORM PersistentCollection. https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/PersistentCollection.php#L536

I am not sure if this fix is the correct one. Can you see how this problem is handled in the ORM or if it is broken in the same way there?

@dazorni
Copy link
Author

dazorni commented Mar 30, 2015

It's broken in the same way in the ORM. You also be unable to work with cleared collections in the event subscriber.

@alcaeus
Copy link
Member

alcaeus commented May 6, 2015

Ping @Ocramius: Just saw that there is an issue for ORM dealing with the same kind of issue: http://www.doctrine-project.org/jira/browse/DDC-3372.

I'm not sure whether this really is the best fix - if we want to provide a snapshot of the state of the collection before clearing it, it must be initialized which might be a big performance impact. I kinda like the compromise of only snapshotting if there was no snapshot upfront.

@malarzm malarzm added the Idea label Jun 11, 2015
@alcaeus alcaeus self-assigned this Jul 30, 2015
@malarzm malarzm added this to the 1.x milestone Oct 31, 2015
@malarzm malarzm assigned malarzm and unassigned alcaeus Oct 31, 2015
@malarzm
Copy link
Member

malarzm commented Nov 7, 2015

@dazorni @sharpner @alcaeus what do you guys think about flag for embed/ref many fields that would change clear behaviour? Default would be the same as is now and with flag turned on it could look like (without scheduling collection for deletion in UoW)

$this->initialize();
foreach ($this->coll as $elem) {
    $this->removeElement($elem);
}

This way developer will be in charge whether or not he needs to know what was deleted and even will be able to use getDeleteDiff. Of course this will fire multiple queries when using addToSet/pushAll but won't have any side effects for others using $set.

Offhand I'm not sure why can't this be implemented in userland instead of calling clear() ;)

@alcaeus
Copy link
Member

alcaeus commented Nov 21, 2015

We decided to take care of this in userland instead of calling clear. It's a complicated issue, since clearing the collection is done somewhere while an event subscriber needs to get accurate insert/delete diffs (which aren't possible when the collection has been cleared). Changing clear behavior by default isn't an option either since the collection in question might be huge and the delete diff might not even be necessary, in which case the collection would be initialized for nothing, costing lots of memory and time.

I'm closing this PR now since there's no nice way to implement this for now and it would change PersistentCollection behavior from what ORM is doing. I think we're better off letting users deal with this in custom collection classes (#1219).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants