-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge entity associated to versioned entity #1573
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1866,7 +1866,7 @@ private function doMerge($entity, array &$visited, $prevManagedCopy = null, $ass | |
} | ||
} | ||
|
||
if ($class->isVersioned) { | ||
if ($class->isVersioned && !($this->isNotInitializedProxy($managedCopy) || $this->isNotInitializedProxy($entity))) { | ||
$reflField = $class->reflFields[$class->versionField]; | ||
$managedCopyVersion = $reflField->getValue($managedCopy); | ||
$entityVersion = $reflField->getValue($entity); | ||
|
@@ -1904,6 +1904,18 @@ private function doMerge($entity, array &$visited, $prevManagedCopy = null, $ass | |
return $managedCopy; | ||
} | ||
|
||
/** | ||
* Tests if an entity is a non initialized proxy class | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it may be clearer to flip this function and rewrite it as a positive-question, where you're asking if the entity has any data to work with.
Other possible names that come to mind are |
||
* | ||
* @param $entity | ||
* | ||
* @return bool | ||
*/ | ||
private function isNotInitializedProxy($entity) | ||
{ | ||
return $entity instanceof Proxy && !$entity->__isInitialized(); | ||
} | ||
|
||
/** | ||
* Sets/adds associated managed copies into the previous entity's association field | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<?php | ||
|
||
namespace Doctrine\Tests\Models\VersionedOneToMany; | ||
|
||
use Doctrine\Common\Collections\ArrayCollection; | ||
|
||
/** | ||
* @Entity | ||
* @Table(name="article") | ||
*/ | ||
class Article | ||
{ | ||
/** | ||
* @Id | ||
* @Column(name="id", type="integer") | ||
* @GeneratedValue(strategy="AUTO") | ||
*/ | ||
public $id; | ||
|
||
/** | ||
* @Column(name="name") | ||
*/ | ||
public $name; | ||
|
||
/** | ||
* @ManyToOne(targetEntity="Category", inversedBy="category", cascade={"merge", "persist"}) | ||
*/ | ||
public $category; | ||
|
||
/** | ||
* Version column | ||
* | ||
* @Column(type="integer", name="version") | ||
* @Version | ||
*/ | ||
public $version; | ||
|
||
/** | ||
* Category constructor. | ||
*/ | ||
public function __construct() | ||
{ | ||
$this->tags = new ArrayCollection(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need a |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<?php | ||
|
||
namespace Doctrine\Tests\Models\VersionedOneToMany; | ||
|
||
use Doctrine\Common\Collections\ArrayCollection; | ||
|
||
/** | ||
* @Entity | ||
* @Table(name="category") | ||
*/ | ||
class Category | ||
{ | ||
/** | ||
* @Id | ||
* @Column(name="id", type="integer") | ||
* @GeneratedValue(strategy="AUTO") | ||
*/ | ||
public $id; | ||
|
||
/** | ||
* @OneToMany(targetEntity="Article", mappedBy="category", cascade={"merge", "persist"}) | ||
*/ | ||
public $articles; | ||
|
||
/** | ||
* @Column(name="name") | ||
*/ | ||
public $name; | ||
|
||
/** | ||
* Version column | ||
* | ||
* @Column(type="integer", name="version") | ||
* @Version | ||
*/ | ||
public $version; | ||
|
||
/** | ||
* Category constructor. | ||
*/ | ||
public function __construct() | ||
{ | ||
$this->articles = new ArrayCollection(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
<?php | ||
|
||
namespace Doctrine\Tests\ORM\Functional; | ||
|
||
use Doctrine\ORM\OptimisticLockException; | ||
use Doctrine\ORM\ORMException; | ||
use Doctrine\Tests\Models\VersionedOneToMany\Article; | ||
use Doctrine\Tests\Models\VersionedOneToMany\Category; | ||
|
||
/** | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, remove this empty line |
||
* @group MergeVersionedOneToMany | ||
*/ | ||
class MergeVersionedOneToManyTest extends \Doctrine\Tests\OrmFunctionalTestCase | ||
{ | ||
protected function setUp() | ||
{ | ||
parent::setUp(); | ||
|
||
try { | ||
$this->_schemaTool->createSchema( | ||
[ | ||
$this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToMany\Category'), | ||
$this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToMany\Article'), | ||
] | ||
); | ||
} catch (ORMException $e) { | ||
} | ||
} | ||
|
||
/** | ||
* This test case asserts that a detached and unmodified entity could be merge without firing | ||
* OptimisticLockException. | ||
*/ | ||
public function testSetVersionOnCreate() | ||
{ | ||
$category = new Category(); | ||
$category->name = 'Category'; | ||
|
||
$article = new Article(); | ||
$article->name = 'Article'; | ||
$article->category = $category; | ||
|
||
$this->_em->persist($article); | ||
$this->_em->flush(); | ||
$this->_em->clear(); | ||
|
||
$articleMerged = $this->_em->merge($article); | ||
|
||
$articleMerged->name = 'Article Merged'; | ||
|
||
$this->_em->flush(); | ||
$this->assertEquals(2, $articleMerged->version); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In conjunction with my other note, I think this conditional would be clearer as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree ! It awfully clearer !
I made the changes