improved lazy loading for ChildrenCollection's #268

Merged
merged 1 commit into from Mar 19, 2013

Conversation

Projects
None yet
3 participants
Member

lsmith77 commented Mar 16, 2013

also added support for string offset's in ChildrenCollection::slice(), dropped UoW::getChildren()/getReferrers(), added UoW::isScheduledForInsert().

btw .. it would be great to at some point provide an even "lazier" solution that creates the instances while iterating.

- *
- * @return ArrayCollection a collection of child documents
- */
- public function getChildren($document, $filter = null, $fetchDepth = null, $locale = null)
@lsmith77

lsmith77 Mar 16, 2013

Member

while this method was public, there was no reason to really use it from the outside

+ *
+ * @return boolean
+ */
+ public function isScheduledForInsert($document)
@lsmith77

lsmith77 Mar 16, 2013

Member

maybe we should add also add equivalent method for the other types of scheduled. also the ORM has a isDocumentScheduled() method too.

@dbu

dbu Mar 18, 2013

Member

if the ORM does that, we should also have it. but lets create a ticket for that and do it when actually needed.

@dbu

dbu Mar 19, 2013

Member

i did not find a ticket so i created this http://www.doctrine-project.org/jira/browse/PHPCR-104 - please close if there already is one that i missed.

- *
- * @return ArrayCollection a collection of referrer documents
- */
- public function getReferrers($document, $type = null, $name = null, $locale = null)
@lsmith77

lsmith77 Mar 16, 2013

Member

while this method was public, there was no reason to really use it from the outside

@dantleech

dantleech Mar 17, 2013

Contributor

Both this method and getChildren are proxied from DocumentManager. I use this method to get referrers for a document that hasn't got a referrers mapping or has one but I don't know what it.is.

@dantleech

dantleech Mar 17, 2013

Contributor

Doh, ignore me, I didn't read the complete diff.

+ $nodeNames = array_slice($nodeNames, $offset, $length);
+ $parentPath = $this->getNode()->getPath();
+ array_walk($nodeNames, function (&$data) use ($parentPath) {
+ $data = "$parentPath/$data";
@dantleech

dantleech Mar 17, 2013

Contributor

Would it make sense to rename $data to $nodeName to make it easier to see whats going on?

@lsmith77

lsmith77 Mar 17, 2013

Member

sure. will do

+ $uow = $this->dm->getUnitOfWork();
+ $oldFetchDepth = $uow->setFetchDepth($this->fetchDepth);
+ $this->node = $this->dm->getPhpcrSession()->getNode($uow->getDocumentId($this->document));
+ $uow->setFetchDepth($oldFetchDepth);
@dbu

dbu Mar 18, 2013

Member

why not use the new depthHint parameter to the session directly? https://github.com/phpcr/phpcr/blob/master/src/PHPCR/SessionInterface.php#L302

@@ -41,7 +43,7 @@ class ChildrenCollection extends PersistentCollection
* @param DocumentManager $dm The DocumentManager the collection will be associated with.
* @param object $document Document instance
* @param string $filter filter string
- * @param integer $fetchDepth optional fetch depth if supported by the PHPCR session
+ * @param null|int $fetchDepth optional fetch depth if supported by the PHPCR session
@dbu

dbu Mar 18, 2013

Member

this has become a feature of PHPCR itself, so we could drop the "if supported by..."

@@ -52,19 +52,38 @@ public function initialize()
if (!$this->initialized) {
$this->initialized = true;
@dbu

dbu Mar 18, 2013

Member

why do we not set initialized=true at the end of the method? what if an exception occurs during initialization? i guess if the user then accidentally tries to use the collection again, he should get the exception again, rather than hiding.

improved lazy loading for ChildrenCollection's
also added support for string offset's in ChildrenCollection::slice(),
dropped UoW::getChildren()/getReferrers(), added UoW::isScheduledForInsert()
+ $nodeName = "$parentPath/$nodeName";
+ });
+
+ $childNodes = $this->dm->getPhpcrSession()->getNodes($nodeNames);
@lsmith77

lsmith77 Mar 19, 2013

Member

it should be noted that we do not use the fetchDepth when doing slice. not sure if we should lobby to also support fetchDepth for getNodes()

@dbu

dbu Mar 19, 2013

Member

i think fetchDepth and getNodes becomes a lot to load in one go... but in this use case here it would make sense.

@lsmith77

lsmith77 Mar 19, 2013

Member

yeah. I was also thinking its an edge case, but at least there is a use case :-)

lsmith77 added a commit that referenced this pull request Mar 19, 2013

Merge pull request #268 from doctrine/collection_lazy_loading
improved lazy loading for ChildrenCollection's

@lsmith77 lsmith77 merged commit 84ad950 into master Mar 19, 2013

1 check was pending

default The Travis build is in progress
Details

@lsmith77 lsmith77 deleted the collection_lazy_loading branch Mar 19, 2013

@dbu dbu referenced this pull request Feb 15, 2014

Open

UnitOfWork::isScheduled... #426

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