Orm: PHP 7.4 Typed Ids musts be nullable to remove an entity

[leongersen]

Bug Report

| Q | A
|------------ | ------
| BC Break | no
| Version | 2.7.0

Summary

This is closely related to #7940, which reports the UnitOfWork accessing properties before initialization. This was resolved in doctrine/persistence 1.3.6.

With that update, Ids must still be nullable to be able to remove the entity, as the UnitOfWork will be setting the id value back to null in \Doctrine\ORM\UnitOfWork::executeDeletions:

// 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);
}

Adding a non-nullable type to a property and trying to remove it results in the following error:

Typed property App\Entity\MyEntity::$id must be int, null used

I'm not sure this necessarily qualifies as a bug in orm, but I'm not sure what else to call it. A potential solution would be to check if the reflField allows null (->getType()->allowsNull()).

[beberlei]

Checking for allows null seems like the sensible approach.

[beberlei]

Fix should be in doctrine/persistence TypedNoDefaultReflectionProperty class with a new setValue overwrite that handles the special case of setting to NULL by unitializing with unset().

[enumag]

@beberlei isn't this fixed by https://github.com/doctrine/reflection/releases/tag/1.2.0?

[Alexandre-T]

@enumag I still have this bug, I use doctrine/reflection 1.2.1. The bug appears as soon as I added a ManyToMany relation between the same entity.

[enumag]

@Alexandre-T What is the version of your doctrine/persistence? If it is 1.3.6+, can you post the exact error message and relevant part of stack trace? It seems your case is elsewhere than what was fixed.

[Alexandre-T]

@enumag doctrine/persistence 1.3.7
I'm using a Symfony5 backend application with api-platform.
My standard User entity had a non-null Collection property stations which is initialized in the constructor. My backend was working. As soon as I had a ManyToOne relation to the same entity User, the serialization process failed because it accessed the initial property before initialization (in __sleep). I remove the @ before ORM to temporary remove the self ManyToOne relation, there is no more error.
Here is the error:

1) DirectorCest: Edit user
Test testsapiSecurityDirectorCest.php:editUser

[Error] Typed property Proxies__CG__AppEntityUser::$ must not be accessed before initialization (in __sleep)

Scenario Steps:

1. $I->sendPUT("/api/users/203",{"email":"[email protected]","familyName":"foo","givenName":"foo","phone":"0000000042","plainPassword":"foo-bar-team","createdAt":"2018-01-20T13:58:21+00:00","updatedAt":"2018-01-20T13:58:21+00...}) at testsapiSecurityDirectorCest.php:153
2. $I->haveHttpHeader("accept","application/json") at testsapiSecurityDirectorCest.php:151
3. $I->haveHttpHeader("content-type","application/json") at testsapiSecurityDirectorCest.php:150
4. $I->grabEntityFromRepository("AppEntityUser",{"email":"[email protected]"}) at testsapiSecurityDirectorCest.php:147

And here is the simplified code of my User entity:

//...
class User implements //....
{
  //...
  /**
     * @ORM\ManyToMany(targetEntity="App\Entity\Station", inversedBy="users")
     *
     * @var Collection|Station[]
     *
     * @Groups({"owner:user:read", "animator:user:read"})
     */
  private Collection $stations;

    /**
     * Referent of this user. 
     *
     * @ORM\ManyToOne(targetEntity="App\Entity\User")
     */
    private ?User $referent = null;

   /**
     * User constructor.
     */
    public function __construct()
    {
        $this->stations = new ArrayCollection();
    }
}

[enumag]

@Alexandre-T Ah, actually that's a different issue. See https://github.com/doctrine/common/issues/886.

[Ciloe]

Hi, there are any news about this issue ? I have the same problem when I want to delete an entity....

    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     **/
    public int $id;

The version of packages :
doctrine/reflection: 1.2.1
doctrine/persistence: 1.3.7

Typed property App\Entity\MyEntity::$id must be int, null used

in RuntimePublicReflectionProperty.php line 47
--
at ReflectionProperty->setValue(object(MyEntity), null)in RuntimePublicReflectionProperty.php line 47
at RuntimePublicReflectionProperty->setValue(object(MyEntity), null)in UnitOfWork.php line 1213
at UnitOfWork->executeDeletions(object(ClassMetadata))in UnitOfWork.php line 423

EDIT

It works in private property id. Because the PR https://github.com/doctrine/reflection/pull/35/files is only on private properties. Thanks to add this code on public properties ?

[oojacoboo]

Honestly, after going through things with this for a while now, we've decided that allowing null for an auto_increment dbms generated value is the ideal functionality.

Firstly, it allows for us to tell if the entity has been persisted. Relying on the EntityManager is not sufficient. And, in a state where an object has been created and not persisted, that value being null, and accessible, is the most sane state/type IMO. If there was a more direct way to check if a value has been initialized, maybe that'd be better, but I'm not aware of a clean way of doing that right now (reflection isn't an option IMO).

[joaojacome]

I've added a fix for public properties here: https://github.com/doctrine/persistence/pull/103

Just need a review :)

[elzozo13]

I also encountered this issue and read the previous posts.

I strongly disagree with @oojacoboo opinion saying the best solution is to make them nullable for the following reasons (I will refer to symfony 5 projects because letely that's what I've been working on, but I'm sure is rather general):

1. When I do something in my code I check for all possible values, and treat all possible cases (it is a good practice and strongly suggest all devs to do this). I waited years for PHP to finally get strong types in order to be able to do that right and now I would love to be able to use them. If I see that an entity has a nullable property, somewhere in my code the following line will appear

if (is_null($entity->getProperty)) { // do something }

Making ids (or any other property for that matter but ids are affected mostly) nullable will pollute my code with thousands of useless checks making it less readable and less maintainable. Also a little bit slower but that is negligible in 99.99% of real life cases so that is not my main issue, but the useless code is.

1. I'm using annotations to generate migrations (I know this practice is debatable but that's how we do it for a series of reasons I won't get in). Right now I have a column set as not nullable in annotations (and in db), and the entity property set as nullable. It is clear that is a discrepancy between the db and the code, and the whole point of the orm is to map that values in objects.

2. I cannot use any code quality tools at their full capabilities (like https://github.com/rectorphp/rector or https://github.com/phpstan/phpstan) because they fail on all entities because annotations say is not nullable while the typehint sais it is in order to be able to delete.

3. There are other minor reasons why this is bad but I think those 3 should suffice to make it clear that the current way it works needs to be improved. PHP 7+ gave us so many great improvements and would be a shame not to take advantage of them in code.

Fix suggestion:
Why not mimic the id property internally in em, and when deleting setting that as null, and check that? Is a simple fix imo. The only time when this issue appears is when a totally new entity is created, and the id hasn't been yet set (but that is not a problem because is never requested hence the exception is not thrown), and when an entity is deleted. A new method for related entities would also need to be added but again... should not be that complicated (at least from what I see in code). Would you be interested in a solution like this?

[oojacoboo]

@elzozo13 I’d like to clarify that we almost always assume an auto generated “id” exists. If you’re frequently dealing with objects where that may not be the case, I’d agree and not advise you to make your id nullable