test case for ticket 1323

Added another testcase which is more "real life" for me

One cause here is a fundamental flaw in the core relation code. When you create a many-many association, i.e. User-UserGroups?-Groups the following happens:

- A one-to-many relation from User to UserGroups? is created, named "UserGroups?" ($user->UserGroups?, the name of the association class)
- A one-to-many relation from Groups to UserGroups? is created, named "UserGroups?" ($group->UserGroups?, the name of the association class)
- A one-to-one relation is created (if it is not defined) from UserGroups? to User
- A one-to-one relation is created (if it is not defined) from UserGroups? to Group

In your nest relation you have 2 many-many between the same class basically. That means Doctrine would create two relations $user->UserReference? (one time for many-many to Children, one time for many-many to Parents), obviously this isnt possible.

Another problem is that the local/foreign columns need to be the other way around, so it seems the documentation is wrong. Take a look at this test model:  http://trac.phpdoctrine.org/browser/branches/1.0/tests/models/NestTest.php .

I do have patch ready but im not too happy with it. It introduces an option 'refClassRelation' that allows you to "rename" the one-many relation to the association table. That, in addition to switching the columns seems to make it work with the patch ( http://pastie.org/258871). Especially, take a look at your (revised) testcase. I would appreciate some feedback on that patch.

Replying to romanb:

Another problem is that the local/foreign columns need to be the other way around, so it seems the documentation is wrong. Take a look at this test model:  http://trac.phpdoctrine.org/browser/branches/1.0/tests/models/NestTest.php .

Sorry, my fault: I mixed up the names of the parent/child - relations.

I do have patch ready but im not too happy with it. It introduces an option 'refClassRelation' that allows you to "rename" the one-many relation to the association table. That, in addition to switching the columns seems to make it work with the patch ( http://pastie.org/258871). Especially, take a look at your (revised) testcase. I would appreciate some feedback on that patch.

I have applied your patch and tested my modified testcase (1323 with 'refClassRelation' and non-mixed up relation names) and got no error anymore: Looks like everything is fine... But unfortunately testcase 1323b is still failing, although I changed the relations of the models in this case to the new 'refClassRelation'-option. I attached the modified 1323b-testcase to this ticket under the name 1323b2: Can you please look for it too? This is the one which brings down my application because I cannot update any data of my concepts without messing their relations...

modified testcase 1323b to the new refClassRelation

While further investigating and debugging testcase 1323b2 i found out that calling of Collection->getInsertDiff() [by running '$oRecord->save();' in line 92] results in differences between the before/after-state of the records' relations ['stacktrace' is: Collection->getInsertDiff() -> UnitOfWork?->saveAssociations() --> UnitOfWork?->saveGraph() ---> Record->save() ]. This happens although i never change any relation but only a 'direct' attribute of my record [$oRecord->identifier = "MySurfaceworking?"; line 91]. In my opinion the reason for the differences is a missing snapshot in Collection->getInsertDiff().

Therefore i changed the method Record->set() which is called when running the $oRecord->identifier = "MySurfaceworking?"; on line 91. My new Record->set()-method now takes a snapshot for all relations when Record->_state changes from CLEAN to DIRTY and the relation has no snapshot yet. I dont know if this is the right place for taking these snapshots but it works for me: Testcase 1323b2 is now running...

changed code is:

[/lib/Doctrine/Record.php]

[...]
public function set($fieldName, $value, $load = true)
  [...]
  switch ($this->_state) {
      case Doctrine_Record::STATE_CLEAN:
      case Doctrine_Record::STATE_PROXY:
          $this->_state = Doctrine_Record::STATE_DIRTY;
          /* FIXME Ticket 1323b2  */
          foreach ($this->_references as $ref) {
          	if (get_class($ref) == "Doctrine_Collection") {
          		if (count($ref->getSnapshot()) == 0) $ref->takeSnapshot();
          	}
          }
          /* end 1323b2  */
          break;
      case Doctrine_Record::STATE_TCLEAN:
          $this->_state = Doctrine_Record::STATE_TDIRTY;
          break;
  }
  [...]
}
[...]

Do you have any idea if this is the right place to look for the problem?

Thanks for your help. You gave me a good hint, takeSnapshot() is the right idea but in the wrong place. Snapshots of collections are there in order to be able to determine the changes that happened to the collection since it was fetched/created. Your patch basically suppresses all change detection in collections because you take a fresh snapshot after each modification.

What i did is to take a snapshot after the collection has been saved, seems this was not done previously in UnitOfWork?. That means the association collection gets saved but no new snapshot taken, hence Doctrine still thinks it needs to insert them on the next save. This really becomes visible when you remove the single "id" primary key from BaseConceptRelation? and use a composite key instead (concept_id, parent_concept_id). I would recommend to do this. Then you get an error about a PK violation because Doctrine tries to insert the same id pair in the association table twice (because it does not take a snapshot after saving).

So do the following:

1) remove the id PK from BaseConceptRelation?, make a composite key out of the foreign keys (primary => true on both columns)

2) run your testcase again, you should get a primary key violation

3) Apply the following patch:  http://pastie.org/261078 and run your testcase again. It should work fine.

If this works well for you i will consider getting the whole patch into 1.0 HEAD.

Replying to romanb:

If this works well for you i will consider getting the whole patch into 1.0 HEAD.

First of all: Thanks for your patch. It works fine: Testcase 1323b2 is not failing anymore.

BUT: When i execute tests/run.php the testcase 'Doctrine_Query_Limit_TestCase' (tests/Query/LimitTestCase.php) fails in 'testLimitWithManyToManyColumnAggInheritanceLeftJoin()' and 'testLimitAttribute()'. With my patch in Doctrine_Record (comment 7) these tests were working. May you please take a look after this new problem?

Sure, i will not commit before getting the tests to the same state as before. Please test your relation setup further and add any new issues here.

Turns out this patch revealed another issue (wrong behavior) that is fixed with the patch. The tests were just wrong. So from my side everything looks good now. I would just like to get a final OK from you, are there any more issues with these relations?

Replying to romanb:

So from my side everything looks good now. I would just like to get a final OK from you, are there any more issues with these relations?

Im sorry, but I have no OK for you. I got another issue, which I unfortunately cannot repeat in a testcase till now. The new problem is: when I try to add a new child to a record which already has a child and a parent, the old child is overwritten with the new one and not added as it is intended. This error only appears in the Concept-Record of my real application which is more complex than the 1323b2-Testcase.

Unfortunately I cannot look for the problem because I am already much too late here. My current failing lines of code in my application are:

situation: 967 has one parent and one child, 1921 shall be added as child:

$concept = Doctrine::getTable('Concept')->find(967);
$c2 = Doctrine::getTable('Concept')->find(1921);
$concept->narrowerConcepts[] = $c2;
$concept->save();

result: 1 parent, 2 childs (as intended)

$concept = Doctrine::getTable('Concept')->find(967);
//the different line follows: lets assume i need $concept->broaderConcepts later...
$broader = $concept->broaderConcepts;
$c2 = Doctrine::getTable('Concept')->find(1921);
$concept->narrowerConcepts[] = $c2;
$concept->save();

result: 1 parent, 1 child (1921) which is obviously wrong.

Replying to floriank:

I got another issue, which I unfortunately cannot repeat in a testcase till now. The new problem is: when I try to add a new child to a record which already has a child and a parent, the old child is overwritten with the new one and not added as it is intended.

Since i could not reproduce this error i think the problem is solved for now. Saving attributes of my models works as well as adding new and unlinking old childs in single transactions, so i think you can patch in the new refClassRelation and getSnapshot(). Please don't forget to change the import- and export-classes for generating models from yaml and vice versa.

Thanks for your good work!

We will patch this properly together with yaml/import/export support for the new attribute in the 1.0.1 bugfix release which will come shortly after 1.0. We rather want to patch this properly instead of throwing it into 1.0 half-finished.

(In [4889]) Fixed #1323. Final adjustments to come from jon.

(In [4890]) fixes #1323 Renaming to refClassRelationAlias and adding support to yaml

Importing from yaml doesn't support the new refClassRelationAlias-attribute properly (Doctrine_Import_Builder).

I have attached a patch which you can add to the current build.

fix for unsupported yaml-import