ORM 4 should not set IDs of removed entities to null anymore

[beberlei]

We have regular problems in the reflection code with setting null of ids on remove since readonly and property hooks were introduced into PHP. See #12401 for a recent example.

Since that is just a convention, we could modify the ORM to not unset these properties to null aynmore. Because IDs cannot/shouldn't be reused anyways.

Users would only need to be careful with merging the removed entities back into the UoW as that behavior would change (ID signals that a row exists on merge). So that is a BC break.

In 3.x introduce a Configuration flag onRemoveEntitySetIdentifierNull which defaults to true (current behavior).
Deprecation is thrown once if flag is true during construction of the UnitOfWork.

Flag is checked here to disable setting null: https://github.com/doctrine/orm/blob/3.6.x/src/UnitOfWork.php#L1184

When merged up to 4.x we can make the flag a no-op, remove the code in UnitOfWork. Potentially also remove a lot of code in the PRopertyAccessors that handle this case (readonly, propertyhooks).

This probably needs a documentation update. Maybe AI can help with locating the documentation places and also to find out of the linked UnitOfWork code is the only case that needs to be adressed or if there are others.

[greg0ire]

Cc @isaackaara, in case you would like to give it a try.

[MorganNextroad]

@greg0ire As far as I can understand the code in the UnitOfWork, the identifier is set to null when only if the identifier is not natural (= has a generator), or the PropertyAcessor is a ReadonlyAccessor.
The second case is, by design, irrelevant, as hooked properties cannot be readonly, so at this point it would require us to remove our generator, and define the identifier out of the generator logic ?
(Yes I'm going through this issue 😅 )

[Amoifr]

@greg0ire it's been a few months without news here, so if this is up for grabs I'd be happy to take it: Configuration::setOnRemoveEntitySetIdentifierNull() defaulting to true, a one-time deprecation when the flag is left to true at UnitOfWork construction, the flag guarding the identifier reset (UnitOfWork L1184), tests for both modes and the docs update. Targeting 3.7.x, with the 4.x no-op/cleanup as a follow-up. Of course if @isaackaara still wants it, it's theirs.

[isaackaara]

@Amoifr let me take it, sorry for the silence @greg0ire

[isaackaara]

PR is up: #12578

Follows the shape from the issue and @Amoifr's refinement - Configuration::isOnRemoveEntitySetIdentifierNull() / setOnRemoveEntitySetIdentifierNull(), default true, one-time deprecation at UnitOfWork construction when left enabled, the identifier reset gated behind the flag, targeting 3.7.x with the 4.x cleanup as a follow-up.

Full suite passes locally (3649 tests, 0 failures) and PHPStan is clean. Flagged a few things worth a second look in the PR description itself, mainly that the deprecation firing from UnitOfWork::__construct() rather than a Configuration getter/setter is a deviation from this file's usual pattern, and that it ends up on by default for every 3.7 user rather than opt-in-triggered - happy to adjust either if you'd rather it work differently.