Possible bug in object relation code? #603

Closed
marcj opened this Issue Feb 15, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@marcj
Member

marcj commented Feb 15, 2013

I've created a testcase that should prove the bahaviour.
https://github.com/MArcJ/Propel/blob/0fe5e3eacc04c9d715ea03e5cb6a6ae214590626/test/testsuite/generator/builder/om/GeneratedObjectMoreRelationTest.php

TestSuite output:

$ phpunit test/testsuite/misc/MoreRelationTest.php
PHPUnit 3.7.13 by Sebastian Bergmann.

Configuration read from /Users/marc/Propel/phpunit.xml.dist

F

Time: 0 seconds, Memory: 18.75Mb

There was 1 failure:

1) MoreRelationTest::testContentsDeletion
We assigned a collection of only one item.
Failed asserting that 4 matches expected 1.

/Users/marc/Propel/test/testsuite/misc/MoreRelationTest.php:97

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.

Is this a desired behaviour? If I apply the following patch, then the testcase is OK.

diff --git a/generator/lib/builder/om/PHP5ObjectBuilder.php b/generator/lib/builder/om/PHP5ObjectBuilder.php
index 1fba18f..ad3c6b6 100644
--- a/generator/lib/builder/om/PHP5ObjectBuilder.php
+++ b/generator/lib/builder/om/PHP5ObjectBuilder.php
@@ -3824,7 +3824,7 @@ abstract class ".$this->getClassname()." extends ".$parentClass." ";
     {
         \${$inputCollection}ToDelete = \$this->get{$relatedName}(new Criteria(), \$con)->diff(\${$inputCollection});

-        \$this->{$inputCollection}ScheduledForDeletion = unserialize(serialize(\${$inputCollection}ToDelete));
+        \$this->{$inputCollection}ScheduledForDeletion = \${$inputCollection}ToDelete;

         foreach (\${$inputCollection}ToDelete as \${$inputCollectionEntry}Removed) {
             \${$inputCollectionEntry}Removed->set{$relCol}(null);

I believe through this unserialize(serialize( call the next call Removed->set{$relCol}(null); doesn't have any effect, does it?

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Feb 15, 2013

Member

Here's the commit, that brought in this unserialize(serialize( call:
b489a97

Member

marcj commented Feb 15, 2013

Here's the commit, that brought in this unserialize(serialize( call:
b489a97

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Feb 15, 2013

Member

After applying the patch above, which removes the unserialize(serialize( call, one assertions fails:

1) GeneratedObjectTest::testReplace_RelationWithCompositePK
Only 1 BookOpinion; the new one got inserted and the previously associated one got deleted
Failed asserting that 2 matches expected 1.

/Users/marc/Propel/test/testsuite/generator/builder/om/GeneratedObjectTest.php:737

This test is also brought in by the same commit that brought in the unserialize(serialize( call. So either my test case is based on a false assumption or the test case of b489a97.

Member

marcj commented Feb 15, 2013

After applying the patch above, which removes the unserialize(serialize( call, one assertions fails:

1) GeneratedObjectTest::testReplace_RelationWithCompositePK
Only 1 BookOpinion; the new one got inserted and the previously associated one got deleted
Failed asserting that 2 matches expected 1.

/Users/marc/Propel/test/testsuite/generator/builder/om/GeneratedObjectTest.php:737

This test is also brought in by the same commit that brought in the unserialize(serialize( call. So either my test case is based on a false assumption or the test case of b489a97.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Feb 15, 2013

Member

Ok, these two test cases differs. Mine has only one PK and the other has a composite PK. Between this two ways of having a PK the save method of the collection ScheduledForDeletion differs, too. So the idea behind the patch of b489a97 is that he 'unlinks' the ScheduledForDeletion collection so the set<RelationName>(null) does not affect ScheduledForDeletion, so the bookOpinionsScheduledForDeletion->getPrimaryKeys later has the correct PK values. Mh, lemme see if I can fix this, because this behaviour is only needed for composite PKs.

Member

marcj commented Feb 15, 2013

Ok, these two test cases differs. Mine has only one PK and the other has a composite PK. Between this two ways of having a PK the save method of the collection ScheduledForDeletion differs, too. So the idea behind the patch of b489a97 is that he 'unlinks' the ScheduledForDeletion collection so the set<RelationName>(null) does not affect ScheduledForDeletion, so the bookOpinionsScheduledForDeletion->getPrimaryKeys later has the correct PK values. Mh, lemme see if I can fix this, because this behaviour is only needed for composite PKs.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Feb 15, 2013

Member

more exact: this behaviour of unlinking the ScheduledForDeletion array is only needed for PKs that are at the same times FKs. If we set the FK to NULL we also set then the PK to null and have therefore no possibility to delete the item later. So $fk->isLocalPrimaryKey() instead of isComposite() is more sensible here, and then make a backup of the PK values through unserialize(serialize( if it returns true.

Member

marcj commented Feb 15, 2013

more exact: this behaviour of unlinking the ScheduledForDeletion array is only needed for PKs that are at the same times FKs. If we set the FK to NULL we also set then the PK to null and have therefore no possibility to delete the item later. So $fk->isLocalPrimaryKey() instead of isComposite() is more sensible here, and then make a backup of the PK values through unserialize(serialize( if it returns true.

marcj added a commit to marcj/Propel that referenced this issue Feb 15, 2013

Fixed #603.
Added a test set that proves the bug.

@marcj marcj referenced this issue Feb 15, 2013

Closed

Fix #603 #604

marcj added a commit to marcj/Propel that referenced this issue Feb 15, 2013

willdurand added a commit that referenced this issue Feb 15, 2013

@havvg havvg referenced this issue Apr 11, 2013

Merged

deleting objects #656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment