Skip to content

Merge support #205

Merged
merged 1 commit into from Jan 8, 2013

5 participants

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

it basically works. the tests are not yet sufficient and there are some cases not yet covered. also there is some redundant code that needs to be refactored.

open questions:
1) should he have special handling to prevent overwriting child properties with null?
2) should we have special handling to prevent overwriting node/nodename/version name/version created/locale with null?

http://doctrine-project.org/jira/browse/PHPCR-13

@lsmith77 lsmith77 referenced this pull request Jan 4, 2013
Closed

Merge support #201

@dantleech

Hi, I'm having a problem with this PR.

daniel@dan-x220 ~/www/DCMS :( $ ./app/console cache:warmup --env=prod 
Warming up the cache for the prod environment with debug false
PHP Catchable fatal error:  Argument 1 passed to Doctrine\ODM\PHPCR\Mapping\Driver\AnnotationDriver::getCascadeMode() must be of the type array, integer given, called in /home/daniel/www/DCMS/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php on line 134 and defined in /home/daniel/www/DCMS/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.$hp on line 211

Basically, getCascadeMode(array $arrayList) is being passed 0 - which is the cascade mode integer when there is no cascade defined, I'm not 100% sure how it gets here -- but it comes from AbstractFile. So I think there is some inheritance stuff going on which is incompatible with oevrriding the array of cascade modes in the annotation with an integer ...

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

hmm i dont remember having fixed that issue .. but it doesnt seem to be there now. i did do a few rebases of this PR.

@dantleech

I think it should still be there, unless its fixed by the merge stuff, but I don't see how it could be, might be due to a nuance in my setup. I will investigate further tomorrow ...

@dantleech

yeah, still the same error, now at line 214

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

hmm this all works nicely for me .. the annotation parser takes care of splitting the values into an array. does the test suite run through for you? what version of doctrine common are you using? maybe this is an issue in the annotation parser version .. ?

@dantleech

Ah yes, I bumped Doctrine\Common to latest and that works nicely. cheers.

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

hmm which means we may need to bump our dependency ..

@rat4m3n
rat4m3n commented Jan 4, 2013

Yeah, I got line 214 complaining too.

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

i guess we need to find out which version of common is required and decide if we want to bump the requirement or add code here to ensure that we have an array. @beberlei do you have an idea?

@rat4m3n
rat4m3n commented Jan 4, 2013

i have checked and kept downgrading common in my composer and actually none of the previous versions work... including attempt for dev-master.

@lsmith77
Doctrine member
lsmith77 commented Jan 4, 2013

it works fine for me with Doctrine Common 2.3.0 (doctrine/common@bb0aebb)

@rat4m3n
rat4m3n commented Jan 5, 2013

OK. 2.3.0 still failing for me .. but aditional check:

    if (!$cascadeList) {
        return 0;
    }

Has fixed this for me there. Not sure if this is 100% correct fix (probably not).

@lsmith77
Doctrine member
lsmith77 commented Jan 5, 2013

so the tests are failing for you?

@rat4m3n
rat4m3n commented Jan 5, 2013

Well my composer / application is failing even with doctrine/common 2.3.0.

Unless I add this condition before the loop in AnnotationDriver.php around the line 214.

@lsmith77
Doctrine member
lsmith77 commented Jan 5, 2013

i am asking because i would like to know if your application is simply doing something different than we currently have covered in the test suite. if so it would be good if you could create a failing test case.

@rat4m3n
rat4m3n commented Jan 5, 2013

This actually makes me think... I dont set titles..... for my SimpleBlock nodes...

@lsmith77
Doctrine member
lsmith77 commented Jan 5, 2013

i just pushed some additional tests to master (64dfdc7) and i cannot reproduce the issue. if the issue persists with your code, can you please open a new ticket ideally with either a snippet of your code pasted or with a failing test?

@rat4m3n
rat4m3n commented Jan 5, 2013

OK... Its failing on having ParentDocument instance:

Catchable Fatal Error: Argument 1 passed to Doctrine\ODM\PHPCR\Mapping\Driver\AnnotationDriver::getCascadeMode() must be of the type array, integer given, called
in [...]/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php on line 134
and defined
in [...]/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php line 211

@lsmith77
Doctrine member
lsmith77 commented Jan 5, 2013

like i said .. i cannot reproduce this issue with doctrine common 2.3.0 :-/

@lsmith77 lsmith77 and 1 other commented on an outdated diff Jan 7, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ $mapping['filter']
+ );
+ $prop->setValue($managedCopy, $managedCol);
+ $this->originalData[$managedOid][$name] = $managedCol;
+ }
+ if ($mapping['cascade'] & ClassMetadata::CASCADE_MERGE > 0) {
+ $managedCol->initialize();
+ if (!$managedCol->isEmpty()) {
+ // clear managed collection, in casacadeMerge() the collection is filled again.
+ $managedCol->unwrap()->clear();
+ $managedCol->setDirty(true);
+ }
+ }
+ } elseif ($class->parentMapping === $name) {
+ $this->doMergeSingleDocumentProperty($managedCopy, $document, $prop, $class->parentMappingData);
+ } elseif (isset($class->localeMapping[$name])
@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

prevent setting locale, version name, version created, node or nodename to null if it was set in the managed copy already .. does this make sense?

@dbu
Doctrine member
dbu added a note Jan 7, 2013

do we merge 2 objects or just update the db from a non-tracked object? version name and version created are readonly fields, node too. if nodename is null this means we have an id set, otherwise we would not even find the right document to merge with? if so then yes, name can be kept. locale is the most dangerous one. keeping the one from the db makes probably sense, unless specified explicitly it would be request locale, which would be the default anyways.

@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

just FYI

there are basically 3 scenarios:
1) the supplied document with an existing version
a) the existing version was fetched from the db
b) the existing version was not fetched from the db (ie. it may have been flushed to the db, but it was created in this request)
2) the supplied document was never previously persisted and therefore its effectively just a persist call

the tricky topic is really only case 1) .. and here usually only a) is relevant for stuff like node/version name/version created ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jan 7, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ $managedCopy,
+ $mapping['filter'],
+ $mapping['fetchDepth']
+ );
+ $prop->setValue($managedCopy, $managedCol);
+ $this->originalData[$managedOid][$name] = $managedCol;
+ }
+ if ($mapping['cascade'] & ClassMetadata::CASCADE_MERGE > 0) {
+ $managedCol->initialize();
+ if (!$managedCol->isEmpty()) {
+ // clear managed collection, in casacadeMerge() the collection is filled again.
+ $managedCol->unwrap()->clear();
+ $managedCol->setDirty(true);
+ }
+ }
+ } elseif (isset($class->referrersMappings[$name])) {
@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

lots of redundant code .. maybe pass a closure to build the collection to a generic method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jan 7, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ );
+ $prop->setValue($managedCopy, $managedCol);
+ $this->originalData[$managedOid][$name] = $managedCol;
+ }
+ if ($mapping['cascade'] & ClassMetadata::CASCADE_MERGE > 0) {
+ $managedCol->initialize();
+ if (!$managedCol->isEmpty()) {
+ // clear managed collection, in casacadeMerge() the collection is filled again.
+ $managedCol->unwrap()->clear();
+ $managedCol->setDirty(true);
+ }
+ }
+ }
+ } elseif (isset($class->childMappings[$name])) {
+ $this->doMergeSingleDocumentProperty($managedCopy, $document, $prop, $class->childMappings[$name]);
+ } elseif (isset($class->childrenMappings[$name])) {
@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

lots of redundant code .. maybe pass a closure to build the collection to a generic method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 and 1 other commented on an outdated diff Jan 7, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ $managedCopy = $class->newInstance();
+ $class->setIdentifierValue($managedCopy, $id);
+ $persist = true;
+ }
+ }
+
+ $managedOid = spl_object_hash($managedCopy);
+
+ // Merge state of $document into existing (managed) document
+ foreach ($class->reflFields as $name => $prop) {
+ if ( isset($class->associationsMappings[$name])) {
+ $mapping = $class->associationsMappings[$name];
+ if ($mapping['type'] & ClassMetadata::TO_ONE) {
+ $this->doMergeSingleDocumentProperty($managedCopy, $document, $prop, $mapping);
+ } else {
+ $mergeCol = $prop->getValue($document);
@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

lots of redundant code .. maybe pass a closure to build the collection to a generic method?

@dbu
Doctrine member
dbu added a note Jan 7, 2013

guess its a good place to use a closure. will make the method logic more understandable too i guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jan 7, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1231,6 +1231,247 @@ private function doRefresh($document, &$visited)
return $document;
}
+ public function merge($document)
+ {
+ $visited = array();
+ return $this->doMerge($document, $visited);
+ }
+
+ private function doMergeSingleDocumentProperty($managedCopy, $document, \ReflectionProperty $prop, array $mapping)
+ {
+ $other = $prop->getValue($document);
+ if (null === $other) {
+ $prop->setValue($managedCopy, null);
@lsmith77
Doctrine member
lsmith77 added a note Jan 7, 2013

this behavior makes sense for ReferenceOne and ParentDocument too i guess .. but it doesnt really make sense for Child imho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Doctrine member
lsmith77 commented Jan 7, 2013

ok .. after some more refactoring i think i cut out the redundant code sufficiently. now the question is just about the special handling for null values in the passed document is left as an open question. see the description of the ticket for details.

@dantleech

ok. im still getting the "array/integer getCascadeMode" problem, and the worst thing is that I cannot reproduce it in a unit test -- it only happens in prod mode. I am using Doctrine Commons latest, and I don't understand why it was working for me earlier. But I think I see the problem, maybe:

  1. Folder and File extend AbstractFile
  2. Metadata for Folder is loaded, the reflection property "parent" on abstract class is set to the cascade mode of the Folder.
  3. The File metadata is loaded, and here the value of the reflection property "parent" is evaluated. It evaluates to an integer.

It seems to be that the File property for $parent is the same instance as that of Folder. I.e. the property comes from the same AbstractFile instance. So, when the metadata for Folder is set, it sets the instance associated with AbstractFile, which is used by the two child classes.

In prod mode the Annotation reader is FileCacheReader, whilst in dev it is AnnotationReader. I'm guessing the problem is coming from there, but havn't confirmed it yet. Anyway, have been working on this for a few hours now, so giving up for today.

@lsmith77 can you confirm if you have the problem in prod ? i.e. with the FileCacheReader ?

@lsmith77
Doctrine member
lsmith77 commented Jan 7, 2013

yes .. i can confirm that there is an issue in prod mode .. will have a look

@stof
Doctrine member
stof commented Jan 7, 2013

@lsmith77 the issue is that your are modifying the annotation object to replace the array by an integer: https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php#L134

If the annotation is cached with the integer, it will become invalid

@lsmith77
Doctrine member
lsmith77 commented Jan 7, 2013

yeah .. i just found out the same thing .. how about #209?

@stof
Doctrine member
stof commented Jan 7, 2013

I think it should work

@dbu
Doctrine member
dbu commented Jan 7, 2013

alright, so do we merge now? or is this still missing something?

@lsmith77
Doctrine member
lsmith77 commented Jan 7, 2013

i am fine with the current state of this PR .. not 100% sure yet about the 2 questions in the descriptions but willing to take the risk that we later discover that we need to change those cases :)

@lsmith77 lsmith77 merged commit e61d766 into master Jan 8, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.