Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entity onLoad container #132

Closed
wants to merge 4 commits into from

Conversation

Mikulas
Copy link
Contributor

@Mikulas Mikulas commented Oct 23, 2015

Updates #131

@Mikulas
Copy link
Contributor Author

Mikulas commented Oct 23, 2015

What it comes down to is IdentityMap::createEntity() calling attach before data is filled

$entity = $this->entityReflections[$entityClass]->newInstanceWithoutConstructor();
$this->repository->attach($entity);
$entity->fireEvent('onLoad', [$data]);

swapping those would not work (onLoad requires metadata set in attach) and also would break back compatibility with onLoad.

I propose adding new event as it's least invasive method and least likely to break anything.

Another solution might be setting metadata via different mechanism than attach. That would also be good for consistency, as setting metadata via attach is only used for entities loaded from storage, new entities have metadata set in constructor (and setting metadata again in attach is redundant).

Are entity events supposed to be overwritten? Either way, the on* methods should list all entity states in which they might be called. I would be happy to send a pr for that.

btw I'm pretty sure there is currently no way to tell what state an entity is in onAttach; an obvious way would be isPersisted(), but that always returns FALSE as id was not yet populated from $data.

@hrach
Copy link
Member

hrach commented Oct 23, 2015

Let's discuss it on Telegram!

@Mikulas
Copy link
Contributor Author

Mikulas commented Oct 23, 2015

solved: replaced onAttach with onCreate+onLoad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants