"preRemove" given wrong entity

[justin-nl]

Bug Report

Q	A
BC Break	no?
Version	2.4.8 (and 2.5)

Summary

I'm working in Symfony 2.8 (and 3.4), with an Invoice entity that has a OneToOne with a File entity (for a PDF upload). I'm cascading "all" from Invoice to File.

The File entity is implementing the PostRemove callback for deleting the physical file after the DB is updated. Sometimes, this is called on the wrong entity, resulting in the wrong file being deleted.

Current behavior

The PostRemove callback is called on the wrong File entity. This results in the wrong physical file being removed. The correct parent Invoice and child File entities are removed from the DB.

It seems related to recent/subsequent entity creation. I would create a few invoices. Delete one. Load another, and find that the file could not be found.

Oddly enough, once I implemented the "PreRemove" (simply for debugging which entity I was working with), the "PostRemove" suddenly worked as expected. Basically, an empty "PreRemove" function "magically" made things work.

My ultimate work-around was to store the filename in a class variable in the "PreRemove" callback, and use that variable to determine which file is deleted in the "PostRemove" callback.

How to reproduce

In the Invoice entity:

/**
 * @ORM\OneToOne(targetEntity="File", cascade={"all"})
 * @ORM\JoinColumn(name="pdf_id", referencedColumnName="id")
 */
protected $pdf;

In the File entity:

/**
 * @ORM\PostRemove()
 */
public function postRemove()
{
    // "getAbsolutePath()" uses the "path" variable from the file object
    // and prepends the local system path
    $file = $this->getAbsolutePath();
    if (null !== $file) {
        // i did a `var_dump($file); exit;` to show the wrong filepath

        // "fs()" returns a new `Symfony\Component\Filesystem\Filesystem`
        $this->fs()->remove($file);
    }
}

Simple deletion using the Entity Manager:

// i verified here that the associations were correct
$em->remove($invoice);
// i verified here that the associations were still correct
$em->flush();
// i never got here because of the `exit;` I had implemented above

Expected behavior

I expect if I'm cascading from a parent object, the resulting PostRemove should be called on the correct child object.

[lcobucci]

@justin-nl unfortunately, v2.4 and v2.5 aren't maintained anymore (only security fixes are backported). Can you check if that still happens in v2.6.3?

[RobinHoutevelts]

@lcobucci We seem to be hitting this as well in latest v2.8.2 with a cascading delete.

The title of this issue is wrong though, it happens in the "PostRemove" callback.

---

The File entity is a Proxy whose values aren’t hydrated ( uninitialised ). After persisting the deletion, when the entity passes Doctrine/ORM/UnitOfWork.php#L1248 it loses its identifier.

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 1244 to 1249 in 3358ccd

	// Entity with this $oid after deletion treated as NEW, even if the $oid
	// is obtained by a new entity because the old one went out of scope.
	//$this->entityStates[$oid] = self::STATE_NEW;
	if (! $class->isIdentifierNatural()) {
	$class->reflFields[$class->identifier[0]]->setValue($entity, null);
	}

A few lines further the PostRemove event is invoked.
Anyone that listens for that event and tries to get a value from the File proxy will trigger a hydration. However because the identifier is gone, the loadById call thats being made in the ProxyFactory is passing an empty array as the $criteria.

// The entity's identifier has been unset in UnitOfWork::executeDeletions() after the deletion has been persisted
$identifier = $classMetadata->getIdentifierValues($proxy);

// $identifier now contains an empty array

orm/lib/Doctrine/ORM/Proxy/ProxyFactory.php

Lines 147 to 152 in 3358ccd

	$identifier = $classMetadata->getIdentifierValues($proxy);
	if ($entityPersister->loadById($identifier, $proxy) === null) {
	$proxy->__setInitializer($initializer);
	$proxy->__setCloner($cloner);
	$proxy->__setInitialized(false);

This causes a SELECT * FROM file query to be run in

orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

Lines 725 to 735 in 3358ccd

	public function load(array $criteria, $entity = null, $assoc = null, array $hints = [], $lockMode = null, $limit = null, ?array $orderBy = null)
	{
	$this->switchPersisterContext(null, $limit);
	$sql = $this->getSelectSQL($criteria, $assoc, $lockMode, $limit, null, $orderBy);
	[$params, $types] = $this->expandParameters($criteria);
	$stmt = $this->conn->executeQuery($sql, $params, $types);
	if ($entity !== null) {
	$hints[Query::HINT_REFRESH] = true;
	$hints[Query::HINT_REFRESH_ENTITY] = $entity;

Which loads all(!) File entities. The first one is being returned..

Which causes the $proxy to be hydrated with a random entity.

---

Just like @justin-nl noticed we can fix this by making sure the entity is hydrated in a “PreRemove” event.

[akasake]

I can confirm that @RobinHoutevelts solution works.
By adding a PreRemoveSubscriber that hydrates the entity, I can access the correct values of the removed entity in the postRemove event.

Important to note is that you can still not access the previous id in postRemove (cause the entity does not exist anymore).

<?php

namespace App\EventListener;

use Doctrine\Common\EventSubscriber;
use Doctrine\ORM\Events;
use Doctrine\Persistence\Event\LifecycleEventArgs;
use Doctrine\Common\Proxy\Proxy;

class PreRemoveHydratorSubscriber implements EventSubscriber
{
    public function getSubscribedEvents(): array
    {
        return [
            Events::preRemove,
        ];
    }

    public function preRemove(LifecycleEventArgs $args): void
    {
        // TODO remove when issue https://github.com/doctrine/orm/issues/7806 is fixed
        $entity = $args->getObject();
        if (!$entity instanceof Proxy) {
            // is not recommended to do this
            // this would mean always initializing every entity that you are about to remove
            // could also make a HasPostRemoveInterface and decide this way which entity you want to do this for
            return;
        }

        // hydrate values of entity proxy,
        // so we can access the values in postRemove
        // is no-op if entity is already initialized
        $entity->__load();
    }
}