PersistenceManager::isNewObject broken for all intents and purposes

[kdambekalns]

Description

(This was reported by @kitsunet in https://neos-project.slack.com/archives/C050KKBEB/p1644399771569339)

\Neos\Flow\Persistence\Doctrine\PersistenceManager::isNewObject currently is broken for all intents and purposes. Unless the object IS already loaded by Doctrine, this method will return true in all cases, regardless if the object is actually new or not.

The reason is that (presumably to avoid DB calls) we call this with $assume set to the NEW state, but as can be seen that will return the assumption if the state is unknown to the current unit of work.

Excerpt from Doctrine getEntityState:

    public function getEntityState($entity, $assume = null)
    {
        $oid = spl_object_hash($entity);

        if (isset($this->entityStates[$oid])) {
            return $this->entityStates[$oid];
        }

        if ($assume !== null) {
            return $assume;
        }

I guess we just need to drop the assumption and live with the performance hit induced (because then it will progress further and check the DB for that entity).

Affected Versions

Flow: 2.0 and later (our code was not changed since then)

[kdambekalns]

Some feedback from the Slack thread on this:

@fcool:

Well, I would not be as harsh. It does work in the case you present a loaded or a just instanciated entity.

I often thought, that now where general persistence is near "dead" (may be a thing to remove?) and doctrine itself makes no difference between new and updated entities, we could drop the differentiation between "add" and "update" in the DoctrineRepository alltogether? But that's completely new discussion, so probably thats a topic for another day.

Back to topic: In theory the unserializer should have loaded all Flow entities during deserialization, so the EntityManager should know their state. So my guess is, that we have to search for the guilty there... Serialization is one of my "favourite" topics :smile:

@kitsunet:

just instanciated entity

only if you already know you just instantiated it

and that is the whole point, if you already know, you don't need this functoin, and if you don't know, well you have a problem because this doesn't really tell you

in many regular contexts this will probably do what you wannt because the entity will already be loaded. But in the case of eg. thumbnails or anything with an idenity defined by it's properties you might have a problem figuring out if this identity already exists

@fcool:

"you" know, thats true. But the repositorys "add" and "update" methods use this check to throw exceptions if an object in "for them" unexpected state arrives. And they does not know and for them the check is nearly 100% accurate.

That said I'm still not convinced they should check at all - as doctrine's interface dont care anyway.

Aaaah. But of course... ValueObjects are "different" in this context. And yes, probably we should handle that different.

@albe:

agree to basically everything you said above :smile: getting rid of update/add difference would be nice, though in some cases an application might need to differenciate, so isNew should work work reliably (even if we might not depend on it ourselves in the future)

[kitsunet]

created PR towards supported LTS.

[fcool]

One problem now is that a quite common use case, using classes which derived from Account will break the session persistence.

If you have an account derived class, which never gets persisted, it breaks after authentication, because it tries to load metadata(which is not there, because the class is not (yet) annotated with @entity). If you fix this, it breaks, because the table is not there. If you fix this, it generates invalid SQL because Account has no Inheritance - Type defined.