Moved dispatching of postLoad event to the point when entities are fully loaded and all associations initialized #470

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
Contributor

chives commented Oct 11, 2012

This is proposed solution for very old http://www.doctrine-project.org/jira/browse/DDC-54. I moved dispatching of postLoad from UnitOfWork to both object hydrators (SimpleObjectHydrator and ObjectHydrator). Now it's dispatched when entities are fully hydrated into objects with all associations.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-250

@stof stof commented on an outdated diff Oct 11, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,21 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postHydrate);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postHydrate]);
+ if ($hasLifecycleCallback || $hasListeners) {
+ foreach ($entities as $entity) {
+ if ($hasLifecycleCallback)
@stof

stof Oct 11, 2012

Member

curly braces are missing

@stof stof commented on an outdated diff Oct 11, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -269,7 +292,12 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ $entity = $this->_uow->createEntity($className, $data, $this->_hints);
+ if (!isset($this->hydratedObjects[$className]))
+ $this->hydratedObjects[$className] = array();
@stof

stof Oct 11, 2012

Member

they are also missing here

@stof stof commented on an outdated diff Oct 11, 2012

...octrine/Tests/ORM/Functional/PostHydrateEventTest.php
@@ -0,0 +1,81 @@
+<?php
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\CMS\CmsUser;
+use Doctrine\ORM\Event\LifecycleEventArgs;
+use Doctrine\ORM\Events;
+require_once __DIR__ . '/../../TestInit.php';
@stof

stof Oct 11, 2012

Member

This require_once should be removed as it is useless

Owner

beberlei commented Oct 12, 2012

Very good addition, but can you remove "postHydrate" and move the "postLoad" event here instead? I don't want to add yet another event and i don't think this is a BC break to move.

Contributor

chives commented Oct 26, 2012

@beberlei according to your suggestion I removed postHydrate and moved dispatching of postLoad from UnitOfWork to both object hydrators (SimpleObjectHydrator and ObjectHydrator). It was a little bit complicated due to detection of proxies and refresh hints but looks like it works perfectly now. Please take a look.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Please align = signs.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Please align = signs.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postLoad]);
+ if ($hasLifecycleCallback || $hasListeners) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Can't you just make the normal flow the non-branched step?
This means you'd have to negate the condition and continue. It would remove a level of indentation easily.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postLoad]);
+ if ($hasLifecycleCallback || $hasListeners) {
+ foreach ($entities as $entity) {
+ if (($entity instanceof Proxy) && !($entity->__isInitialized__)) {
+ continue;
+ }
+ if ($hasLifecycleCallback) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -181,6 +189,26 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postLoad]);
+ if ($hasLifecycleCallback || $hasListeners) {
+ foreach ($entities as $entity) {
+ if (($entity instanceof Proxy) && !($entity->__isInitialized__)) {
+ continue;
+ }
+ if ($hasLifecycleCallback) {
+ $meta->invokeLifecycleCallbacks(Events::postLoad, $entity);
+ }
+ if ($hasListeners) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -269,7 +297,20 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ if (($entity = $this->getEntityFromIdentityMap($className, $data)) && (!($entity instanceof Proxy) || $entity->__isInitialized__))
+ {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

The open curly braces should be at the same line.
Generally speaking, this condition should be:

if (($entity = $this->getEntityFromIdentityMap($className, $data)) && ( ! ($entity instanceof Proxy) || $entity->__isInitialized__)) {

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -269,7 +297,20 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ if (($entity = $this->getEntityFromIdentityMap($className, $data)) && (!($entity instanceof Proxy) || $entity->__isInitialized__))
+ {
+ if (!isset($this->_hints[Query::HINT_REFRESH]) &&
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing spaces around !.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -269,7 +297,20 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ if (($entity = $this->getEntityFromIdentityMap($className, $data)) && (!($entity instanceof Proxy) || $entity->__isInitialized__))
+ {
+ if (!isset($this->_hints[Query::HINT_REFRESH]) &&
+ ((!isset($this->_hints[Query::HINT_REFRESH_ENTITY]) || ($this->_hints[Query::HINT_REFRESH_ENTITY] !== $entity))))
+ return $entity;
+ }
+
+ $entity = $this->_uow->createEntity($className, $data, $this->_hints);
+ if (!isset($this->hydratedObjects[$className])) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -269,7 +297,20 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ if (($entity = $this->getEntityFromIdentityMap($className, $data)) && (!($entity instanceof Proxy) || $entity->__isInitialized__))
+ {
+ if (!isset($this->_hints[Query::HINT_REFRESH]) &&
+ ((!isset($this->_hints[Query::HINT_REFRESH_ENTITY]) || ($this->_hints[Query::HINT_REFRESH_ENTITY] !== $entity))))
+ return $entity;
+ }
+
+ $entity = $this->_uow->createEntity($className, $data, $this->_hints);
+ if (!isset($this->hydratedObjects[$className])) {
+ $this->hydratedObjects[$className] = array();
+ }
+ $this->hydratedObjects[$className][] = $entity;
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

...trine/ORM/Internal/Hydration/SimpleObjectHydrator.php
@@ -52,6 +57,26 @@ protected function hydrateAllData()
$this->_em->getUnitOfWork()->triggerEagerLoads();
+ $evm = $this->_em->getEventManager();
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Align = signs

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

...trine/ORM/Internal/Hydration/SimpleObjectHydrator.php
@@ -52,6 +57,26 @@ protected function hydrateAllData()
$this->_em->getUnitOfWork()->triggerEagerLoads();
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 3, 2012

...trine/ORM/Internal/Hydration/SimpleObjectHydrator.php
@@ -52,6 +57,26 @@ protected function hydrateAllData()
$this->_em->getUnitOfWork()->triggerEagerLoads();
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->_em->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postLoad]);
+ if ($hasLifecycleCallback || $hasListeners) {
@guilhermeblanco

guilhermeblanco Nov 3, 2012

Owner

Missing line break.
What I previously have referred of normal flow being your current level's branch is also valid here.

Contributor

chives commented Nov 6, 2012

It seems that I ran into a bug in PHP with this addition after rebase on the current master branch. Fix for this bug is going to be merged in next PHP release - https://bugs.php.net/bug.php?id=63055 . If someone is interested in details - https://travis-ci.org/#!/chives/doctrine2/jobs/3081989

Owner

beberlei commented Nov 12, 2012

This PR has some problems:

  • It does not work in a scenario where "->iterate()" is used.
  • It caches the objects again, possibly leading to a memory leak when iterating.

@beberlei beberlei and 1 other commented on an outdated diff Nov 12, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -280,7 +328,7 @@ private function getEntity(array $data, $dqlAlias)
private function getEntityFromIdentityMap($className, array $data)
{
// TODO: Abstract this code and UnitOfWork::createEntity() equivalent?
- $class = $this->ce[$className];
@beberlei

beberlei Nov 12, 2012

Owner

why do you remove this?

@chives

chives Nov 14, 2012

Contributor

original version caused errors in the test suite, but i fixed this in another way

Contributor

chives commented Nov 14, 2012

@beberlei only references to all hydrated objects are additionally stored in ObjectHydrator. I'm aware of the costs but i think that potential gain is much bigger in this case.

Considering the problem with $query->iterate() what do you think about another hint that would be automatically set on the Query object in iterate() method ? Setting that hint would cause dispatching of postLoad in hydrateRowData() instead of hydrateAllData() .

I can think of yet another (opposed) solution. We could dispatch postLoad in hydrateRowData() by default and move it to hydrateAllData() only if some hint was set on Query. This would additionally optimize memory usage in usual cases when the hint is not set.

Which one is better in your opinion?

Owner

beberlei commented Nov 14, 2012

@chives there is already a hint about ITERATE being currently done. I think we can use that.

Contributor

chives commented Nov 14, 2012

@beberlei is my new solution sufficient?

@stof stof commented on an outdated diff Nov 15, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -180,6 +188,35 @@ protected function hydrateAllData()
foreach ($this->initializedCollections as $coll) {
$coll->takeSnapshot();
}
+
+ if (isset($this->_hints[Query::HINT_INTERNAL_ITERATION]))
@stof

stof Nov 15, 2012

Member

curly braces are missing

@stof stof commented on an outdated diff Nov 15, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
+ if (isset($this->_hints[Query::HINT_INTERNAL_ITERATION]))
+ return $result;
+
+ $evm = $this->_em->getEventManager();
+ $hasListeners = $evm->hasListeners(Events::postLoad);
+
+ foreach ($this->hydratedObjects as $class => $entities) {
+ $meta = $this->getClassMetadata($class);
+ $hasLifecycleCallback = isset($meta->lifecycleCallbacks[Events::postLoad]);
+
+ if ( ! $hasLifecycleCallback && ! $hasListeners) {
+ continue;
+ }
+
+ foreach ($entities as $entity) {
+ if (($entity instanceof Proxy) && ! ($entity->__isInitialized__)) {
@stof

stof Nov 15, 2012

Member

the braces are not needed here

@stof stof commented on an outdated diff Nov 15, 2012

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -260,6 +297,9 @@ private function getEntity(array $data, $dqlAlias)
$className = $this->ce[$className]->discriminatorMap[$data[$discrColumn]];
+ if (!isset($this->ce[$className]))
@stof

stof Nov 15, 2012

Member

missing curly braces

Owner

Ocramius commented Nov 18, 2012

@chives I know I'm picky, but is there a way to test this (or at least verify that it works with the various different associations)? That would prevent a regression if somebody changes your code later on...

Contributor

chives commented Nov 19, 2012

@Ocramius you're absolutely right. I'll try to add some tests that will prove this code works in different cases.

Contributor

chives commented Nov 26, 2012

@beberlei is there anything missing in this PR ?

Contributor

chives commented Dec 12, 2012

@beberlei, @stof, @Ocramius are you suggesting any more changes in this PR or is it ready to merge?

Contributor

chives commented Feb 27, 2013

@beberlei, @guilhermeblanco is there any chance to merge this before 2.4 release?

@beberlei beberlei commented on an outdated diff Feb 27, 2013

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -276,7 +305,30 @@ private function getEntity(array $data, $dqlAlias)
$this->_hints['fetchAlias'] = $dqlAlias;
- return $this->_uow->createEntity($className, $data, $this->_hints);
+ if (($entity = $this->getEntityFromIdentityMap($className, $data)) && ( ! ($entity instanceof Proxy) || $entity->__isInitialized__)) {
@beberlei

beberlei Feb 27, 2013

Owner

How is this related to postLoad event triggering? this condition looks scary.

@beberlei beberlei commented on an outdated diff Feb 27, 2013

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -183,6 +193,21 @@ protected function hydrateAllData()
$coll->takeSnapshot();
}
+ if (isset($this->_hints[Query::HINT_INTERNAL_ITERATION])) {
+ return $result;
+ }
+
+ foreach ($this->hydratedObjects as $class => $entities) {
@beberlei

beberlei Feb 27, 2013

Owner

I don't like this, it includes all objects even if we would know much earlier if these objects actually have a post load event.

Owner

beberlei commented Feb 27, 2013

I think the full logic with regard to post load handling should be seperated into its own object that is reused inside both hydrators. This can also hold the event dispatcher and handle the filtering for ITERATE and if the classes even have post load events.

Please refactor the code accordingly. The class could be named "PostLoadEventDispatcher" and have methods "dispatchPostLoad()" and "dispatchEnqueuedPostLoadEvents()".

Contributor

chives commented Feb 27, 2013

@beberlei Thanks for your answers. I fully agree with the idea of reusable PostLoadEventDispatcher and will try to refactor this PR as soon as possible.

Contributor

chives commented Feb 28, 2013

@beberlei what do you think about the new version of this PR ?

@norzechowicz norzechowicz commented on an outdated diff Feb 28, 2013

lib/Doctrine/ORM/Event/PostLoadEventDispatcher.php
+ * @param object $entity
+ */
+ public function dispatchPostLoad($entity)
+ {
+ $className = get_class($entity);
+ $meta = $this->metadataFactory->getMetadataFor($className);
+ $invoke = $this->invoker->getSubscribedSystems($meta, Events::postLoad);
+
+ if ($invoke === ListenersInvoker::INVOKE_NONE) {
+ return;
+ }
+
+ if (isset($this->hints[Query::HINT_INTERNAL_ITERATION])) {
+ $this->invoker->invoke($meta, Events::postLoad, $entity, new LifecycleEventArgs($entity, $this->entityManager), $invoke);
+ } else {
+ if (!isset($this->entities[$className])) {
@norzechowicz

norzechowicz Feb 28, 2013

Contributor

Missing spaces around negation

Owner

beberlei commented Mar 12, 2013

@chives much better, but i would rather have a solution where the dispatcher could be local to the hydrators and not necessary in the UnitOfWork. I am thinking of a way how to achieve this.

Contributor

chives commented Mar 14, 2013

@beberlei what about this new solution? I personally don't like it too much but it's close to your last suggestions.

Contributor

chives commented Mar 25, 2013

@beberlei ping

Miliooo commented Oct 13, 2013

Is there any progress on this? I wanted to use a permalink setter listener on a postload event, but now i need to pull this information from an objects association. Since this association hasn't been initialized i'm kind of stuck.

Owner

Ocramius commented Oct 16, 2013

@Miliooo the first thing you can do is pulling this locally, adding a test for your particular use case and PR against @chives' branch

Contributor

chives commented Dec 3, 2013

@Ocramius @beberlei i've rebased this PR over the current master branch. @Miliooo could you provide some real life test case which fails without these changes? it may help to prove that this is a useful addition.

👍

Is there any progress? Would be nice to see this PR merged.

fmosca commented Mar 5, 2014

+1

bassrock commented Mar 5, 2014

Would love to see this in the next version.

Ocramius self-assigned this Jan 12, 2015

Owner

Ocramius commented Jan 12, 2015

This has been handled in #1001, but I will merge the tests from this PR into master

Ocramius closed this Jan 13, 2015

Owner

Ocramius commented Jan 13, 2015

I decided to exclude the iterate() case: when iterating, postLoad listeners will simply be executed when the end of the iteration is reached.

I will document this as a limitation for now.

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - reverting hydrator changes, as patch DDC-3005 …
…already deals with the issue
b1144e7

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - minor test cleanups, changing test according t…
…o current limitation to document the actual expected behavior
0a19fbb

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - reverting assertion
`postLoad` should be triggered eagerly when using `iterate()`, as worse problems may be experienced with
missed initialization via listeners.
5cd73f0

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - simple-object hydration should also trigger `p…
…ostLoad` events when iterating over single results
0ffc752

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - query iteration must cause eager `hydrationCom…
…plete` logic to be fired
f571a9e

@Ocramius Ocramius added a commit that referenced this pull request Jan 13, 2015

@Ocramius Ocramius #470 DDC-54 DDC-3005 - documenting `postLoad` and `Doctrine\ORM\Abstr…
…actQuery#iterate()` partial incompatibility
b81209c
Owner

Ocramius commented Jan 13, 2015

Integrated via 14c3adb

I've documented the limitations of Doctrine\ORM\AbstractQuery#iterate() (when combined with postLoad) at https://github.com/doctrine/doctrine2/blob/14c3adbec02d567339e7aad0168442af353ec120/docs/en/reference/events.rst#lifecycle-events

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment