DDC-1680: Id is null in postRemove events

[doctrinebot]

Jira issue originally created by user redemption:

When using the postRemove event, $this->id (where id is the annotated @Id of the Entity) is null.

All other fields have the correct data (eg if you have an Entity with name, email, address, and id as fields, the id field is the only one that is null).

[doctrinebot]

Comment created by @beberlei:

This is expected behavior, otherwise when you merge that entity back it will be detected as "known", although its actually deleted. You can save the id in preRemove and then reuse it in postRemove if you need it.

[doctrinebot]

Comment created by redemption:

Is there any chance of providing a native premade value (or method) to store the deleted id, for instance "deletedId"? It seems logical to me that postRemove events will need to know the Id that was removed, and hacking it in with preRemove events all the time is less elegant.

[doctrinebot]

Comment created by umed:

yeah I also think that we need something native. This hack with preremove looks really ugly.

[doctrinebot]

Comment created by umed:

Here is https://groups.google.com/forum/#!topic/doctrine-user/bTukzq0QrSE a very good comment left by Pavel Horal

[doctrinebot]

Comment created by @ocramius:

[~umed] I'm fine with reverting this, but it needs a new and well exposed issue.

[doctrinebot]

Comment created by ahmadnazir:

There there another issue created for this problem? If not, then I would like to help.

What do you mean by a well exposed issue?

[doctrinebot]

Comment created by @ocramius:

[~ahmadnazir] I mean that we could need a functional test case

[doctrinebot]

Comment created by pawelbaranski:

Hi,

I'd also see this issue solved. I have a case where I'm storing an entity both in MySQL and part of it in Elasticsearch. When I delete an entity from MySQL I'd like it to be deleted from ES as well. I would not want to remove it from ES too soon so postRemove event would be what I need.

I will use the hack Benjamin presented, but it is like an unwanted child

[doctrinebot]

Issue was closed with resolution "Invalid"

[ebuildy]

Why this is closed?

[lcobucci]

@ebuildy apparently nobody bothered to create a "new and well exposed issue" (which means it has a clear explanation and a functional test) as @Ocramius asked. Would you be wiling to do that?

You can find examples of functional tests on https://github.com/doctrine/doctrine2/tree/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket

[dkarlovi]

@lcobucci if I'm creating a test case, it's a PR against master or where?

[lcobucci]

@dkarlovi you can use 2.6 as base.

[dkarlovi]

Until I create the test, it get's reviewed, accepted/rejected/fixed/released, I need the described workaround.

Where exactly am I supposed to store the identifier in the preRemove handler? On the event? But it's not the same object in preRemove and postRemove. I imagine we can abuse PHP's synchronous nature and store as a property inside my handler and count on things not getting out of order, but that's just too messy for comfort, TBH.

IMO if this state is the solution for

when you merge that entity back it will be detected as "known", although its actually deleted

then this exact problem needed a better solution. For example, mark the entity as "deleted" or "unmergeable" or something.

[Ocramius]

Where exactly am I supposed to store the identifier in the preRemove handler?

I usually record everything I need to "keep" in onFlush, which right after the changes in the UnitOfWork were computed.

The id being null is currently by design, so we'd break BC for a scenario like this if we reverted it:

$entity = new Foo(); // has generated id
$em->persist($entity);
$em->flush();
$em->remove($entity);
$em->flush();
$em->persist($entity); // if we revert the change, this one would fail due to duplicate identifier
$em->flush();

[dkarlovi]

@Ocramius thanks, makes sense.

The id being null is currently by design, so we'd break BC for a scenario like this if we reverted it: if we revert the change, this one would fail due to duplicate identifier

I know, that was my point with

mark the entity as "deleted" or "unmergeable" or something.

so, when you try to merge it back, it just throws. This allows for correct behavior in both cases (entity which is deleted is a orphaned entity you still have a reference to, you still don't get to reuse it just like that).

[Ocramius]

->merge() will no longer be improved, and is deprecated (and removed in 3.x

[dkarlovi]

Sorry, "merge" is a lapsus, I meant "when you try to treat it as a regular entity in any way" (persist, update, etc).

[Ocramius]

Ah, sorry. No, there is no way to do that. After flush(), memory usage must stay constant in the ORM, and adding a registry that marks removed entities would be a big issue.

[dkarlovi]

Fair point.

Would a better workaround be to introduce a new event type, PostRemoveEventArgs which would have getIdentifier()? Then Doctrine can do this for you and I wouldn't be too bad.

[Ocramius]

@dkarlovi we can improve this in 3.x by adding the identifier to PostRemoveEventArgs or by breaking BC as I described in https://github.com/doctrine/doctrine2/issues/2326#issuecomment-360719911

[ismail1432]

PostRemoveEventArgs which would have getIdentifier()

One year later, I think it's a good idea :+1:

[Nathanael-Shermett]

PostRemoveEventArgs which would have getIdentifier()

One year later, I think it's a good idea 👍

Another two years later, and I also think it's a good idea.

It doesn't make sense to modify an entity class (or perhaps every entity class), complete with getters and setters, just to store a historical ID. It's hacky, and it subverts the entity's integrity. With that said, I have a hacky solution that is at least self-contained:

public function preRemove(LifecycleEventArgs $args): void {
    $entity = $args->getObject();
    $entity->historicalId = $entity->getId(); // $historicalId is not defined or referenced anywhere but here
}

public function postRemove(LifecycleEventArgs $args): void {
    $entity = $args->getObject(); // the old ID is accessible via $entity->historicalId
}

I'm sure some purists on here will say this is gross. I agree. However, this limitation shouldn't exist in the first place, and a man's gotta eat somehow.

[Warxcell]

I suggest just to move https://github.com/doctrine/orm/blob/2.8.x/lib/Doctrine/ORM/UnitOfWork.php#L1244-L1246 after https://github.com/doctrine/orm/blob/2.8.x/lib/Doctrine/ORM/UnitOfWork.php#L1248-L1250.

That will bring the ID in postRemove events. I think it's not convenient to have getIdentifier in event, for examples on places like that: https://github.com/Warxcell/files/blob/master/src/NamingStrategy/IdToPathStrategy.php

+1 for BC-break, It would be nice to have the ID even after postRemove. The use case is, when/if https://github.com/doctrine/dbal/pull/4622 is accepted, I will be able to refactor my listener on postRemove it will collect entities for removal, and on onTransactionCommit I can loop over pending entities for removal and call $fileManager->remove($entity) which could use ID to determine the path to the file. That will be impossible if IDs are gone after postRemove.

[Nathanael-Shermett]

FYI, my above fix (using a dynamic property to store the entity ID before it is lost during the delete process) will be deprecated in PHP 8.2 and will throw an error in PHP 9.0.

You can read more here.

[justin-oh]

For anyone searching in the future, I had to do something like the following to maintain an ID-based value (which was a directory path).

Before:

public function getDirectory(): string
{
    return __DIR__.'/../../entity-uploads/'.$this->id;
}

After:

private string $directory;

public function getDirectory(): string
{
    return $this->directory ?? __DIR__.'/../../entity-uploads/'.$this->id;
}

/**
 * For preRemove lifecycle callback because $this->id is NULL in postRemove.
 */
public function setDirectory(): static
{
    $this->directory = $this->getDirectory();

    return $this;
}

[orangevinz]

Using a trait can be convenient as well

trait StoreIdAfterDeletionEntity
{
    private ?int $storedId;

    public function getStoredId(): ?int
    {
        return $this->storedId;
    }

    #[ORM\PreRemove]
    public function setStoredId(): static
    {
        $this->storedId = $this->getId();

        return $this;
    }
}

#[ORM\Entity]
#[ORM\HasLifecycleCallbacks]
class MyEntity
{
    use StoreIdAfterDeletionEntity;
}

[Richardds]

@orangevinz Unfortunately, the trait methods won't get registered as Lifecycle callbacks.