fix: Do not change readonly id when deleting (#9538)

[robske110]

Fixes #9538 by not resetting the ID of objects which have a readonly identifier to null.
This will cause those to not be seen as new objectes (will be STATE_DETACHED). However, this is in my understanding not a problem as entities with readonly ID cannot be reused anyways.
If those need to be seen as NEW for some practical reason, we could also persist the NEW state in entityStates (which the commented line just above the resetting does).

Additional notes:
From the commit commenting that line out, it is not entirely clear to me why that change was made (setting id to null rather than persisting the state). Persisting the state instead of setting the id to null does not seem to cause any trouble with all tests passing (apart from maybe a slight memory leak).

This does not touch the issues #9505.

[greg0ire]

From the commit commenting that line out

Link for the lazy: 4212b88

it is not entirely clear to me why that change was made (setting id to null rather than persisting the state)

I asked my LLM, it says that it's

because the state is now fully unset() instead, which is safer against spl_object_hash reuse. The getEntityState() method can dynamically infer STATE_NEW when needed, so explicitly storing it was both unnecessary and a potential source of bugs.

And indeed, in the diff above, you can see that $this->entityStates[$oid] is included in a call to unset(). Having it unset leads to this piece of code, which correctly assume that it was either new or detached

orm/src/UnitOfWork.php

Line 1629 in 580a95c

// State can only be NEW or DETACHED, because MANAGED/REMOVED states are known.

[robske110]

To answer to your comment regarding my additional notes:
(All of this is not really relevant to this PR specifically, but still an interesting dicussion and prompted me to look at all of this a bit closer.)

I think the reason the LLM provided doesn't really make sense.

because the state is now fully unset() instead, which is safer against spl_object_hash reuse

We're actually using spl_object_id here (but it has the same reuse behavior). However, reuse can in my opinion not lead to additional bugs, as if the object id will be reused, that object obviously should also be initially in STATE_NEW - and that's now just still cached. Now re-reading the comments above the commented line, it actually explictly mentions this.
After some more digging: Technically this would however prevent the new object from being seen as DETACHED until it became managed before. But to reproduce this, oid reuse isn't even necesarry! (You can simply re-set an id on a deleted entity.) So this might have been the actual reason the change was made. However, see at the end of this comment for more on this.

Also: Because this project now requires PHP 8 this could all be rewritten to use a WeakMap anyways if we for some reason that's not yet obvious to me need/want to start storing the state again. This also gets rid of multiple "memory leaks", and the reuse "issue", but it would not solve the DETACHED state not being possible.

And on that DETACHED state:
Actually it looks like that at this point the distinction between NEW and DETACHED is potentially not even really required anymore: Setting the value of STATE_DETACHED to 2 (STATE_NEW) doesn't result in any test failures, and with this you can even remove the ID to null change in executeDeletions and everything still passes. This would still be a somewhat breaking API change at the very least though.
And the test suite might also simply not be comprehensive enough here. (It definetly doesn't validate some things, for example the code path to throw an exception on attempting to delete detached entities is never tested.)

[robske110]

Let me know if this PR otherwise looks good now with the simplified check, then I can squash the two commits together.

[greg0ire]

It is looking good, yes