refacto: use Weakmap for UnitOfWork internal storage instead of uid i…

[vgreb]

Q	A
Bug fix?	yes/no
New feature?	yes/no
BC breaks?	yes/no
Deprecations?	yes/no
Tests pass?	yes
Licence	Apache-2.0
Fixed tickets	#1234

I proposed to add this feature in v3.10.0 and not wait the release of v4.0

[Ngob]

Are we sure about this change?

If we do that:

$entity = $ting->getById(12);
$entity->setSomething('foobaz');
$entity = null;

Or:

$entity = new MyEntity();
$ting->save($entity);
$entity = null;

Won't it be removed from the weakmap so the change wont be persisted?

[xavierleune]

For the first case you're right, it's expected. As the app doesn't retain the entity, no need for ting to keep working with it.
For the second use case, no, as save processes the changes immediately.
But if you use the UnitOfWork, there is indeed a bug. Good catch !
Like this:

$myEntities = $ting->getBy(['awesome' => true]);

foreach ($myEntities as $entity) {
    $entity->setAwesome = false;
    $unitOfWork->pushSave($entity);
    // After this loop the entity will be destroyed by GC, because the refcount will drop to 0, except for the last one
}
$unitOfWork->process();

I think entities that have been sent to UnitOfWork should have a real reference, not a weak one.

[Ngob]

Thanks for the fast reply @xavierleune

If my understanding is correct, it means we should preserve $entitiesShouldBePersisted as an array

[xavierleune]

@Ngob Indeed, I think that's correct

[vgreb]

@Ngob @xavierleune I've switch back $entitiesShouldBePersisted to an array, however this may have a impact in performances.
Let me explain, UnitofWork::process() loops over the property $entitiesShouldBePersisted and forward the uuid to methods process*(). In order to retrieve the corresponding entity, I need to iterate on the WeakMap $entities.
I don't kwow about big of an impact this this for performance.

Any thoughts ?

[xavierleune]

Actually you should keep the entity in $entitiesShouldBePersisted, if not we are back to the same issue. No tingUuid should exists anymore.
You can index $entitiesShouldBePersisted with spl_oject_hash if you need to have a unique identifier (note sure if needed). Sounds good ?

[vgreb]

Something bugles me, why change $entitiesShouldBePersisted to an array ? It only stores the persistence state. However, the entity is actually stored in $entityChanged (cf methods processManaged, processNew, processDelete).
Am I wrong ?

[xavierleune]

@vgreb follow up on this, here is a patch after a few tests on unitofwork:
unitofwork.PATCH

[vgreb]

@xavierleune, I applied your patch. It's will avoid performance issues I mentionned earlier.