Entity attach should be called after onLoad

[Mikulas]

Attaching before hydrating is done may cause unexpected states. For example, I'm attaching a finite state machine service, but the state is not initialized yet.

In other words, onAttach handlers may require complete entity, but onLoad must rely on the fetched data and should not require attached services.

Attach during fetch was introduced in and the behaviour was not changed since: https://github.com/nextras/orm/commit/1c57c79d46a29a59b370b309499f830adb669147#diff-e26a575a236aa861c0d14711806aac2c

I propose this patch:

@@ src/Repository/IdentityMap.php
+       $entity->fireEvent('onLoad', [$data]);
        $this->repository->attach($entity);
-       $entity->fireEvent('onLoad', [$data]);

[Mikulas]

Well, seems this is duplicate of https://github.com/nextras/orm/pull/132 which I opened 2 years ago ¯\(ツ)/¯

[hrach]

And it also contains the reason why it's bad PR :D The attach is not suitable for doing initial set up. Does this case have something different why we should reconsider? :)

[Mikulas]

No it doesn't contain the reason why. I strongly believe Orm should not leak entities in incomplete state and I'm fairly convinced this is a design issue.

What are the downsides of swapping those two events?

Would you agree with this?

onAttach handlers may require complete entity, but onLoad must rely on the fetched data and should not require attached services.

=> onAttach may require data from onLoad, onLoad requires nothing. Current implementation does not support this.

[hrach]

onAttach may require data from onLoad

This is antipattern and bad design. You should not do anything with data in on attach event, or better, you cannnot know what state the entity is in, therefore it is wrong. onAttach event purpose is for some work connected to a repository, not the data in the entity itself.

I strongly believe Orm should not leak entities in incomplete state

This is nonsese: Orm does not anyhow leak incomplete entity - becasuse there is nothing as "complete" neither "incomplete" state. You may do (and actually, in pre 2.0 it was recommended) $e = new Entity; $repository->attach($e); $e->property = val;

[Mikulas]

I don't concede any of the points. The fix is trivial, solves IMO rather major issue and whats worse, current implementation leads to hard to solve bugs. Nevertheless, next time I will just remember these issues.

[hrach]

If you open this issue once more, I will remove onAttach event, because you are clearly misusing it.