fixes #PHPCR-78 #158

Merged
merged 8 commits into from Aug 30, 2012

Conversation

Projects
None yet
4 participants
Contributor

pitpit commented Aug 10, 2012

This is a bugfix for http://www.doctrine-project.org/jira/browse/PHPCR-78 but I'm really not sure to did it the best way....

This pull request passes (merged 3f72527 into fc47314).

Member

lsmith77 commented Aug 16, 2012

i wonder if in the node type definition we can find the protected properties so that we do not need to hardcode them. @dbu is on vacation but maybe @adou600 knows this too?

Member

dbu commented Aug 27, 2012

yes, that would be the most generic way. the code would look about this:

if ($node->hasProperty($class->fieldMappings[$fieldName]['name']) {
    $property = $node->getProperty($class->fieldMappings[$fieldName]['name']);
    $definition = $property->getDefinition();
    if ($definition->isProtected()) {
        continue;
    }

@pitpit can you try this? would be awesome if you can add a test too, to make sure it actually works. i.e. manually changing the create date.

hm, one question: why is the date changed in the first place? do we want to silently ignore users trying to change a protected property or should this throw an error so the user may learn it is not possible to change that property?

Contributor

pitpit commented Aug 27, 2012

Hi,
I updated the isssue http://www.doctrine-project.org/jira/browse/PHPCR-78 and I'm working on a reliable test to make it clear.
@dbu I'll try your code asap

Contributor

pitpit commented Aug 28, 2012

I added in my last commit a functional test to reproduce the bug.

@dbu The code you provide does not work because in this case, Jackalope\Property::getDefinition() throws a \Jackalope\NotImplementedException

here's the exact code I tested (from L1457 to L1472 in Doctrine\ODM\PHPCR\UnitOfWOrk):

               if ($class->fieldMappings[$fieldName]['multivalue']) {
                    $value = $fieldValue === null ? null : $fieldValue->toArray();
                    $node->setProperty($class->fieldMappings[$fieldName]['name'], $value, $type);
                } else if ($node->hasProperty($class->fieldMappings[$fieldName]['name'])) {
                    $property = $node->getProperty($class->fieldMappings[$fieldName]['name']);
                    try { //TODO: remove this try-catch when Jackalope\Property::getDefinition() will be able to acquire definition 
                        $definition = $property->getDefinition();
                        if (!$definition->isProtected()) {
                            $node->setProperty($class->fieldMappings[$fieldName]['name'], $fieldValue, $type);
                        }
                    } catch (\Jackalope\NotImplementedException $e) {
                        $node->setProperty($class->fieldMappings[$fieldName]['name'], $fieldValue, $type);
                    }
                } else {
                    $node->setProperty($class->fieldMappings[$fieldName]['name'], $fieldValue, $type);
                }

The other way could be to simply ignore ConstraintViolationException replacing L1461:

$node->setProperty($class->fieldMappings[$fieldName]['name'], $fieldValue, $type);

by:

$node->setProperty($class->fieldMappings[$fieldName]['name'], $fieldValue, $type, false);

@lsmith77 what do you thing about that ?

This pull request passes (merged a7e1ba8 into 11b721d).

This pull request fails (merged 8df754a into 11b721d).

Contributor

pitpit commented Aug 28, 2012

Travis seems to fails because of a timeout...

Member

dbu commented Aug 28, 2012

the false parameter to not throw exceptions is an internal thing of jackalope and not part of phpcr, so no option.

i try to implement the Property::getDefinition() method, should be able to push tomorrow.

what about the issue that this will silently ignore changes to protected properties? this might create hard to debug confusing situations for users not aware that they try to change a protected property.

Contributor

pitpit commented Aug 28, 2012

The bug we are facing only happens when we map a reference (@referencemany or @referenceone) field into a document. We could gently ignore the constraint violation to avoid it but -like you said- users won't be notified anymore if they try to change a protected property...
The problem is that I don't find why phpcr-odm try to update protected properties only when a reference field is mapped in metadata...
@lsmith77 any idea ?

Member

lsmith77 commented Aug 29, 2012

I dont think its an issue that we silently ignore such changes. If we ever get logging integrated we could log this, but otherwise I would hope that someone that expects these to change will test and find that they dont get updated.

As for why this issue only happens if there is a reference mapping, I dont know.

Member

dbu commented Aug 29, 2012

ok, Property::getDefinition() is now implemented in jackalope. can you try if it works for you and if so, remove the try-catch statement around it?

Contributor

pitpit commented Aug 29, 2012

Okay, I found the bug.

When there's an association field is mapped in a document, UnitOfWork schedules an update (L1418) on the whole document even if the document is new.

When executeUpdates() is called with scheduled association updates (L1270), every fields are considered as updated fields because they all appear in $this->documentChangesets[$oid]['fields'] (L1476) .

Protected fields like 'jcr:created' are set to null. So phpcr-odm try to push a null value to a protected field in Jackrabbit.

Then we got the "Can't remove the protected property" exception....

I pushed a commit that should fix this bug.

Sorry @dbu we won't need Property::getDefinition() for now

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1427,6 +1434,10 @@ private function executeInserts($documents)
$this->evm->dispatchEvent(Event::postPersist, new LifecycleEventArgs($document, $this->dm));
}
}
+
+ foreach($associationChangesets as $oid => $changeset) {
@lsmith77

lsmith77 Aug 29, 2012

Member

missing a space after foreach

@pitpit

pitpit Aug 29, 2012

Contributor

fixed in d0a07da

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1415,6 +1417,11 @@ private function executeInserts($documents)
}
} elseif (isset($class->associationsMappings[$fieldName])) {
$this->scheduledAssociationUpdates[$oid] = $document;
+
+ //populate $associationChangesets to force executeUpdates($this->scheduledAssociationUpdates) to only update association fields
+ $data = isset($associationChangesets[$oid]['fields'])?$associationChangesets[$oid]['fields']:array();
@lsmith77

lsmith77 Aug 29, 2012

Member

can you add some spaces around ? and :?

@pitpit

pitpit Aug 29, 2012

Contributor

fixed in d0a07da

Member

dbu commented Aug 29, 2012

@pitpit: np, much better that you found the actual root of the problem and fixed it! probably would create all sorts of weird issues in other situations as well. and the getDefinition is now there in case anybody ever needs it.

This pull request passes (merged 1b17705 into ba2b815).

This pull request passes (merged d0a07da into ba2b815).

Member

lsmith77 commented Aug 29, 2012

@pitpit before i merge this a quick question, is there a specific reason why you need this intermediate array rather than writing straight to $this->documentChangesets[$oid] ?

Contributor

pitpit commented Aug 29, 2012

@lsmith77 yes, because I didn't want to interact with the loop https://github.com/pitpit/phpcr-odm/blob/d0a07da49a24422735f94384de79aeffadf0e72e/lib/Doctrine/ODM/PHPCR/UnitOfWork.php#L1387 in where the association changeset is builded

This pull request passes (merged db60354e into ba2b815).

This pull request passes (merged 6957703 into ba2b815).

lsmith77 added a commit that referenced this pull request Aug 30, 2012

@lsmith77 lsmith77 merged commit 059f3de into doctrine:master Aug 30, 2012

Member

lsmith77 commented Aug 30, 2012

thx .. merged

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