Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

improve error message when adding a document to an existing path #184

Merged
merged 2 commits into from

3 participants

@dbu
Collaborator

@beberlei lukas said that you probably have an opinion on this... do you think it is correct to report this instead of just "detached document"? it was very confusing for me until i figured out that i actually try to add 2 documents with the same id.

@lsmith77
Owner

if we change this then it should likely also be changed in all other ORM/ODMs

@dbu
Collaborator

travis fail is because of a midgard setup issue, jackrabbit passes

@Ocramius
Owner

Identifiers could also be objects, should consider that or it will fail in trying to call __toString

@dbu
Collaborator
dbu commented

ups, thanks for the warning. though i am not sure if this really is the case for phpcr-odm. maybe this is only relevant in the ORM?

do you agree with the general intention of this change?

@Ocramius
Owner

@dbu better exceptions (even if heavy) is always good IMO, that's self-documenting "facedesk" for the user :)

@dbu
Collaborator
dbu commented

ok, i handle the caes of an object with no __toString now. should we merge or is there anything else?

@Ocramius
Owner

:+1:

@dbu dbu merged commit e3c2eca into master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 22, 2012
  1. @dbu
Commits on Nov 8, 2012
  1. @dbu

    handle id object case

    dbu authored
This page is out of date. Refresh to see the latest.
View
8 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -525,7 +525,13 @@ private function doScheduleInsert($document, &$visited, $overrideIdGenerator = n
$this->setDocumentState($oid, self::STATE_MANAGED);
break;
case self::STATE_DETACHED:
- throw new \InvalidArgumentException('Detached document passed to persist(): '.self::objToStr($document, $this->dm));
+ $class = $this->dm->getClassMetadata(get_class($document));
+ $id = $class->getIdentifierValue($document);
+ // ids might be objects too
+ if (is_object($id) && ! method_exists($id, '__toString')) {
+ $id = 'Class: ' . get_class($id);
+ }
+ throw new \InvalidArgumentException('Detached document or new document with already existing id passed to persist(): '.self::objToStr($document, $this->dm)." ($id)");
}
$this->cascadeScheduleInsert($class, $document, $visited);
View
24 tests/Doctrine/Tests/ODM/PHPCR/Functional/BasicCrudTest.php
@@ -79,6 +79,28 @@ public function testInsert()
$this->assertEquals($user->numbers->toArray(), $userNew->numbers->toArray());
}
+ /**
+ * @expectedException \InvalidArgumentException
+ *
+ * This should probably lead to a merge when http://www.doctrine-project.org/jira/browse/PHPCR-13 is implemented
+ */
+ public function testInsertTwice()
+ {
+ $user = new User();
+ $user->username = "test";
+ $user->numbers = array(1, 2, 3);
+ $user->id = '/functional/test';
+ $this->dm->persist($user);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $user = new User();
+ $user->username = "toast";
+ $user->numbers = array(1, 2, 3);
+ $user->id = '/functional/test';
+ $this->dm->persist($user);
+ }
+
public function testFindByClass()
{
$user = $this->node->addNode('userWithAlias');
@@ -92,7 +114,7 @@ public function testFindByClass()
$this->assertEquals('dbu', $userWithAlias->username);
}
- public function testFindNonPersisted()
+ public function testFindNonFlushed()
{
$user = new User();
$user->username = "test";
Something went wrong with that request. Please try again.