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

#968 - Fix the child deletion issue in many-to-one form #1027

Merged
merged 1 commit into from Dec 10, 2015

Conversation

pimpreneil
Copy link
Contributor

Here is a fix for the Child deletion issue in many-to-one form issue.

Because this issue is caused mainly by Symfony not comparing the collection objects like Propel is made for, I unfortunately did not manage to provide any tests, though this fix does not break any existing ones...

@marcj
Copy link
Member

marcj commented Oct 30, 2015

Thanks! Can you also please add a failing unit tests that covers your changes? I'm not sure if this fixes really that issue. I think we had a issue with this approach (performance/memory) and changed it a bit. Not sure why it isn't working at the moment.

@pimpreneil
Copy link
Contributor Author

Thanks @marcj , unfortunately, as mentioned I really am not capable to write the failing test (I did try!).
This PR does fix the issue, one of my projects was stuck because of this issue, and this fix really sorted the things out.
There must indeed be a reason for this code to be removed in Propel2.

As detailed in the related issue #968 , the problem originates from an object comparison in Symfony2 that fails when using Propel, and succeeds when using Doctrine

@pimpreneil
Copy link
Contributor Author

@marcj Sorry, I fucked up a bit and pushed my modifs twice (forgot to merge master before pushing my branch)...
Anyway, here is the requested failing test.

@Maxooo
Copy link

Maxooo commented Nov 6, 2015

Good test, thank you @pimpreneil 👍

@pimpreneil
Copy link
Contributor Author

If you don't want to merge it, maybe change the tag? :)

@marcj marcj removed the Tests Needed label Dec 9, 2015
@marcj
Copy link
Member

marcj commented Dec 9, 2015

Very nice, could you please squash your commits into one? :)

@pimpreneil
Copy link
Contributor Author

@marcj Done. Thx 😀

marcj added a commit that referenced this pull request Dec 10, 2015
#968 - Fix the child deletion issue in many-to-one form
@marcj marcj merged commit 8003d93 into propelorm:master Dec 10, 2015
@marcj
Copy link
Member

marcj commented Dec 10, 2015

Awesome, thanks =)

@ktounet ktounet mentioned this pull request Mar 8, 2016
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

3 participants