Skip to content

Commit

Permalink
Fix broken changeset computation for entities loaded through fetch=EA…
Browse files Browse the repository at this point in the history
…GER + using inheritance (#10884)

#10880 reports a case where the changes from #10785 cause entity updates to be missed.

Upon closer inspection, this change seems to be causing it:

https://github.com/doctrine/orm/pull/10785/files#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4L2990-L3003

The code was changed to use `registerManaged()` instead, which basically does the same things, but (since #10785) also includes an additional check against duplicate entity instances.

But, one detail slipped through tests and reviews: `registerManaged()` also updates `\Doctrine\ORM\UnitOfWork::$originalEntityData`, which is used to compute entity changesets. An empty array `[]` was passed for $data here.

This will make the changeset computation assume that a partial object was loaded and effectively ignore all field updates here:

https://github.com/doctrine/orm/blob/a616914887ea160db4158d2c67752e99624f7c8a/lib/Doctrine/ORM/UnitOfWork.php#L762-L764

I think that, effectively, it is sufficient to call `registerManaged()` only in the two cases where a proxy was created.

Calling `registerManaged()` with `[]` as data for a proxy object is consistent with e. g. `\Doctrine\ORM\EntityManager::getReference()`.

In the case that a full entity has to be loaded, we need not call `registerManaged()` at all, since that will already happen inside `EntityManager::find()` (or, more specifically, `UnitOfWork::createEntity()` called inside it).

Note that the test case has to make some provisions so that we actually reach this case:
* Load an entity that uses `fetch="EAGER"` on a to-one association
* That association being against a class that uses inheritance (why's that?)
  • Loading branch information
mpdude authored Aug 9, 2023
1 parent a616914 commit 440b244
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 7 deletions.
9 changes: 2 additions & 7 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -3048,6 +3048,7 @@ public function createEntity($className, array $data, &$hints = [])
// We are negating the condition here. Other cases will assume it is valid!
case $hints['fetchMode'][$class->name][$field] !== ClassMetadata::FETCH_EAGER:
$newValue = $this->em->getProxyFactory()->getProxy($assoc['targetEntity'], $normalizedAssociatedId);
$this->registerManaged($newValue, $associatedId, []);
break;

// Deferred eager load only works for single identifier classes
Expand All @@ -3056,20 +3057,14 @@ public function createEntity($className, array $data, &$hints = [])
$this->eagerLoadingEntities[$targetClass->rootEntityName][$relatedIdHash] = current($normalizedAssociatedId);

$newValue = $this->em->getProxyFactory()->getProxy($assoc['targetEntity'], $normalizedAssociatedId);
$this->registerManaged($newValue, $associatedId, []);
break;

default:
// TODO: This is very imperformant, ignore it?
$newValue = $this->em->find($assoc['targetEntity'], $normalizedAssociatedId);
break;
}

if ($newValue === null) {
break;
}

$this->registerManaged($newValue, $associatedId, []);
break;
}

$this->originalEntityData[$oid][$field] = $newValue;
Expand Down
119 changes: 119 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH10880Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\ORM\Mapping as ORM;
use Doctrine\Tests\OrmFunctionalTestCase;

use function reset;

class GH10880Test extends OrmFunctionalTestCase
{
protected function setUp(): void
{
parent::setUp();

$this->setUpEntitySchema([
GH10880BaseProcess::class,
GH10880Process::class,
GH10880ProcessOwner::class,
]);
}

public function testProcessShouldBeUpdated(): void
{
$process = new GH10880Process();
$process->description = 'first value';

$owner = new GH10880ProcessOwner();
$owner->process = $process;

$this->_em->persist($process);
$this->_em->persist($owner);
$this->_em->flush();
$this->_em->clear();

$ownerLoaded = $this->_em->getRepository(GH10880ProcessOwner::class)->find($owner->id);
$ownerLoaded->process->description = 'other description';

$queryLog = $this->getQueryLog();
$queryLog->reset()->enable();
$this->_em->flush();

$this->removeTransactionCommandsFromQueryLog();

self::assertCount(1, $queryLog->queries);
$query = reset($queryLog->queries);
self::assertSame('UPDATE GH10880BaseProcess SET description = ? WHERE id = ?', $query['sql']);
}

private function removeTransactionCommandsFromQueryLog(): void
{
$log = $this->getQueryLog();

foreach ($log->queries as $key => $entry) {
if ($entry['sql'] === '"START TRANSACTION"' || $entry['sql'] === '"COMMIT"') {
unset($log->queries[$key]);
}
}
}
}

/**
* @ORM\Entity
*/
class GH10880ProcessOwner
{
/**
* @ORM\Id
* @ORM\GeneratedValue
* @ORM\Column(type="integer")
*
* @var int
*/
public $id;

/**
* fetch=EAGER is important to reach the part of \Doctrine\ORM\UnitOfWork::createEntity()
* that is important for this regression test
*
* @ORM\ManyToOne(targetEntity="GH10880Process", fetch="EAGER")
*
* @var GH10880Process
*/
public $process;
}

/**
* @ORM\Entity()
* @ORM\InheritanceType("SINGLE_TABLE")
* @ORM\DiscriminatorColumn(name="type", type="string")
* @ORM\DiscriminatorMap({"process" = "GH10880Process"})
*/
abstract class GH10880BaseProcess
{
/**
* @ORM\Id
* @ORM\GeneratedValue
* @ORM\Column(type="integer")
*
* @var int
*/
public $id;

/**
* @ORM\Column(type="text")
*
* @var string
*/
public $description;
}

/**
* @ORM\Entity
*/
class GH10880Process extends GH10880BaseProcess
{
}

0 comments on commit 440b244

Please sign in to comment.