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

[DoctrineBridge] Added EntityCollectionType for better collection handling. #1745

Closed
wants to merge 1 commit into from

Conversation

marcw
Copy link
Contributor

@marcw marcw commented Jul 20, 2011

Hi everyone,

I worked on this patch in order to fix issue #1540. This does not affect PropertyPath but create a new EntityCollectionType.

Keep in mind that this solution is not perfect, but might work on a large number of case. The good news is that the default behavior can be overridden to fit your need.

Known limitations are :

  • It won't work if you have on one entity two relations to the same other entity.

*/
protected function processPersistentCollection(PersistentCollection $collection)
{
foreach ($collection->getInsertDiff() as $k => $data) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$k is unused here.

@stof
Copy link
Member

stof commented Jul 20, 2011

You don't solve the issue of collections on the owning side, and you break performances for the owning side by removing the relations in the DB and adding them again. Thus you remove entities that should not be deleted which breaks the code.

The issue with Doctrine collections was that you have to call a method on the parent to add or remove elements safely to allow handing the inversed side instead of adding them directly in the collection object.
And this impacts the collection type but also the entity type when using with multiple=true

@marcw
Copy link
Contributor Author

marcw commented Jul 20, 2011

@stof I don't remove relations from DB to add them again. I only create new entities when you add new items in the form. These items are passed as array inside the PersistentCollection and thus, an entity have to be instancied for each of these new items.

In the end, do you think the whole approach is flawed or shall I make modifications in order to only call methods on parent objects. If so, I guess these methods name should be defined in the options. What do you think ?

@stof
Copy link
Member

stof commented Jul 20, 2011

@marcw You do remove entities from the DB as you use $em->remove().

You are not fixing the issue of inversed side at all, as the only way to fix it is to call the addXXX and removeXXX methods of the parent object to use the additional logic contained in them to update the owning side when updating the inversed side (logic that is not generated by the EntityGenerator which is why a generated entity is not fully usable when using bidirectional relations).

Now that you talk about it, there is indeed another issue with the collection type as it should be able to create an instance of a class when adding new items, but this is not only for Doctrine collections (it should be done in the CollectionType itself by adding an option to specify the class) and it is not the issue you claim to fix in the description of the PR.

@stof
Copy link
Member

stof commented Jul 20, 2011

I reported the other issue you highlighted here as #1746

@marcw
Copy link
Contributor Author

marcw commented Jul 20, 2011

Just saw that.

I'm closing this pull request and will work at a better solution for these problems.

@marcw marcw closed this Jul 20, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants