Skip to content

[RFR] Added "endFlush" event #481

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

[RFR] Added "endFlush" event #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4c0aa1
Added "endFlush" event
dantleech Apr 27, 2014
3ee3bf3
Trying to avoid infinite loop
dantleech Jun 11, 2014
05af81d
Modify a copy of actualData (changeSet)
dantleech Jun 12, 2014
2f529e8
Always call session#save and fixes to tests
dantleech Jun 13, 2014
04b0e5c
Fix
dantleech Jun 13, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/Doctrine/ODM/PHPCR/Event.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ final class Event

const preFlush = 'preFlush';
const postFlush = 'postFlush';
const endFlush = 'endFlush';
const onFlush = 'onFlush';
const onClear = 'onClear';
const loadClassMetadata = 'loadClassMetadata';
Expand Down
121 changes: 73 additions & 48 deletions lib/Doctrine/ODM/PHPCR/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,18 +1157,20 @@ public function computeChangeSet(ClassMetadata $class, $document)
$this->changesetComputed[] = $oid;

$actualData = $this->getDocumentActualData($class, $document);
$changeSet = $actualData;

$id = $this->getDocumentId($document, false);

$isNew = !isset($this->originalData[$oid]);
if ($isNew) {
// Document is New and should be inserted
$this->originalData[$oid] = $actualData;
$this->documentChangesets[$oid] = array('fields' => $actualData, 'reorderings' => array());
$this->originalData[$oid] = $changeSet;
$this->documentChangesets[$oid] = array('fields' => $changeSet, 'reorderings' => array());
$this->scheduledInserts[$oid] = $document;
}

if ($class->parentMapping && isset($actualData[$class->parentMapping])) {
$parent = $actualData[$class->parentMapping];
if ($class->parentMapping && isset($changeSet[$class->parentMapping])) {
$parent = $changeSet[$class->parentMapping];
$parentClass = $this->dm->getClassMetadata(get_class($parent));
$state = $this->getDocumentState($parent);

Expand All @@ -1178,33 +1180,34 @@ public function computeChangeSet(ClassMetadata $class, $document)
}

foreach ($class->childMappings as $fieldName) {
if ($actualData[$fieldName]) {
if (is_array($actualData[$fieldName]) || $actualData[$fieldName] instanceof Collection) {
if ($changeSet[$fieldName]) {
if (is_array($changeSet[$fieldName]) || $changeSet[$fieldName] instanceof Collection) {
throw PHPCRException::childFieldIsArray(
self::objToStr($document, $this->dm),
$fieldName
);
}
if (!is_object($actualData[$fieldName])) {

if (!is_object($changeSet[$fieldName])) {
throw PHPCRException::childFieldNoObject(
self::objToStr($document, $this->dm),
$fieldName,
gettype($actualData[$fieldName])
gettype($changeSet[$fieldName])
);
}

$mapping = $class->mappings[$fieldName];
$actualData[$fieldName] = $this->computeChildChanges($mapping, $actualData[$fieldName], $id, $mapping['nodeName']);
$changeSet[$fieldName] = $this->computeChildChanges($mapping, $changeSet[$fieldName], $id, $mapping['nodeName']);
}
}

$this->computeAssociationChanges($class, $oid, $isNew, $actualData, 'reference');
$this->computeAssociationChanges($class, $oid, $isNew, $actualData, 'referrer');
$this->computeAssociationChanges($class, $oid, $isNew, $changeSet, 'reference');
$this->computeAssociationChanges($class, $oid, $isNew, $changeSet, 'referrer');

foreach ($class->mixedReferrersMappings as $fieldName) {
if ($actualData[$fieldName]
&& $actualData[$fieldName] instanceof PersistentCollection
&& $actualData[$fieldName]->isDirty()
if ($changeSet[$fieldName]
&& $changeSet[$fieldName] instanceof PersistentCollection
&& $changeSet[$fieldName]->isDirty()
) {
throw new PHPCRException("The immutable mixed referrer collection in field $fieldName is dirty");
}
Expand All @@ -1213,16 +1216,16 @@ public function computeChangeSet(ClassMetadata $class, $document)
if ($isNew) {
// much simpler handling for children
foreach ($class->childrenMappings as $fieldName) {
if ($actualData[$fieldName]) {
if (!is_array($actualData[$fieldName]) && !$actualData[$fieldName] instanceof Collection) {
if ($changeSet[$fieldName]) {
if (!is_array($changeSet[$fieldName]) && !$changeSet[$fieldName] instanceof Collection) {
throw PHPCRException::childrenFieldNoArray(
self::objToStr($document, $this->dm),
$fieldName
);
}

$mapping = $class->mappings[$fieldName];
foreach ($actualData[$fieldName] as $originalNodename => $child) {
foreach ($changeSet[$fieldName] as $originalNodename => $child) {
if (!is_object($child)) {
throw PHPCRException::childrenContainsNonObject(
self::objToStr($document, $this->dm),
Expand All @@ -1232,9 +1235,9 @@ public function computeChangeSet(ClassMetadata $class, $document)
}

$nodename = $this->getChildNodename($id, $originalNodename, $child, $document);
$actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
$changeSet[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
if (0 !== strcmp($originalNodename, $nodename)) {
unset($actualData[$fieldName][$originalNodename]);
unset($changeSet[$fieldName][$originalNodename]);
}
$childNames[] = $nodename;
}
Expand All @@ -1245,30 +1248,30 @@ public function computeChangeSet(ClassMetadata $class, $document)
$destPath = $destName = false;

if (isset($this->originalData[$oid][$class->parentMapping])
&& isset($actualData[$class->parentMapping])
&& $this->originalData[$oid][$class->parentMapping] !== $actualData[$class->parentMapping]
&& isset($changeSet[$class->parentMapping])
&& $this->originalData[$oid][$class->parentMapping] !== $changeSet[$class->parentMapping]
) {
$destPath = $this->getDocumentId($actualData[$class->parentMapping]);
$destPath = $this->getDocumentId($changeSet[$class->parentMapping]);
}

if (isset($this->originalData[$oid][$class->nodename])
&& isset($actualData[$class->nodename])
&& $this->originalData[$oid][$class->nodename] !== $actualData[$class->nodename]
&& isset($changeSet[$class->nodename])
&& $this->originalData[$oid][$class->nodename] !== $changeSet[$class->nodename]
) {
$destName = $actualData[$class->nodename];
$destName = $changeSet[$class->nodename];
}

// there was assignment move
if ($destPath || $destName) {
// add the other field if only one was changed
if (false === $destPath) {
$destPath = isset($actualData[$class->parentMapping])
? $this->getDocumentId($actualData[$class->parentMapping])
$destPath = isset($changeSet[$class->parentMapping])
? $this->getDocumentId($changeSet[$class->parentMapping])
: PathHelper::getParentPath($this->getDocumentId($document));
}
if (false === $destName) {
$destName = $actualData[$class->nodename]
? $actualData[$class->nodename]
$destName = $changeSet[$class->nodename]
? $changeSet[$class->nodename]
: PathHelper::getNodeName($this->getDocumentId($document));
}

Expand All @@ -1284,35 +1287,36 @@ public function computeChangeSet(ClassMetadata $class, $document)
}

if (isset($this->originalData[$oid][$class->identifier])
&& isset($actualData[$class->identifier])
&& $this->originalData[$oid][$class->identifier] !== $actualData[$class->identifier]
&& isset($changeSet[$class->identifier])
&& $this->originalData[$oid][$class->identifier] !== $changeSet[$class->identifier]
) {
throw new PHPCRException('The Id is immutable ('.$this->originalData[$oid][$class->identifier].' !== '.$actualData[$class->identifier].'). Please use DocumentManager::move to move the document: '.self::objToStr($document, $this->dm));
throw new PHPCRException('The Id is immutable ('.$this->originalData[$oid][$class->identifier].' !== '.$changeSet[$class->identifier].'). Please use DocumentManager::move to move the document: '.self::objToStr($document, $this->dm));
}

foreach ($class->childrenMappings as $fieldName) {
if ($actualData[$fieldName] instanceof PersistentCollection) {
if (!$actualData[$fieldName]->isInitialized()) {
if ($changeSet[$fieldName] instanceof PersistentCollection) {
if (!$changeSet[$fieldName]->isInitialized()) {
continue;
}

$coid = spl_object_hash($actualData[$fieldName]);
$this->visitedCollections[$coid] = $actualData[$fieldName];
$coid = spl_object_hash($changeSet[$fieldName]);
$this->visitedCollections[$coid] = $changeSet[$fieldName];
}


$mapping = $class->mappings[$fieldName];

$childNames = array();
$movedChildNames = array();
if ($actualData[$fieldName]) {
if (!is_array($actualData[$fieldName]) && !$actualData[$fieldName] instanceof Collection) {
if ($changeSet[$fieldName]) {
if (!is_array($changeSet[$fieldName]) && !$changeSet[$fieldName] instanceof Collection) {
throw PHPCRException::childrenFieldNoArray(
self::objToStr($document, $this->dm),
$fieldName
);
}

foreach ($actualData[$fieldName] as $originalNodename => $child) {
foreach ($changeSet[$fieldName] as $originalNodename => $child) {
if (!is_object($child)) {
throw PHPCRException::childrenContainsNonObject(
self::objToStr($document, $this->dm),
Expand All @@ -1322,9 +1326,9 @@ public function computeChangeSet(ClassMetadata $class, $document)
}

$nodename = $this->getChildNodename($id, $originalNodename, $child, $document);
$actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
$changeSet[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
if (0 !== strcmp($originalNodename, $nodename)) {
unset($actualData[$fieldName][$originalNodename]);
unset($changeSet[$fieldName][$originalNodename]);
$movedChildNames[] = (string) $originalNodename;
}
$childNames[] = $nodename;
Expand Down Expand Up @@ -1370,8 +1374,8 @@ public function computeChangeSet(ClassMetadata $class, $document)
if (!isset($this->documentLocales[$oid])
|| $this->documentLocales[$oid]['current'] === $this->documentLocales[$oid]['original']
) {
// remove anything from $actualData that did not change
foreach ($actualData as $fieldName => $fieldValue) {
// remove anything from $changeSet that did not change
foreach ($changeSet as $fieldName => $fieldValue) {
if (isset($class->mappings[$fieldName])) {
if ($this->originalData[$oid][$fieldName] !== $fieldValue) {
continue;
Expand All @@ -1385,17 +1389,20 @@ public function computeChangeSet(ClassMetadata $class, $document)
}
}

unset($actualData[$fieldName]);
unset($changeSet[$fieldName]);
}
}

if (count($actualData)) {
if (count($changeSet)) {
if (empty($this->documentChangesets[$oid])) {
$this->documentChangesets[$oid] = array('fields' => $actualData, 'reorderings' => array());
} else {
$this->documentChangesets[$oid]['fields'] = $actualData;
$this->documentChangesets[$oid] = array(
'fields' => array(),
'reorderings' => array()
);
}

$this->documentChangesets[$oid]['fields'] = $changeSet;
$this->originalData[$oid] = $actualData;
$this->scheduledUpdates[$oid] = $document;
} elseif (isset($this->documentChangesets[$oid])) {
// make sure we don't keep an old changeset if an event changed
Expand Down Expand Up @@ -1942,6 +1949,22 @@ public function commit($document = null)
}
}

if (empty($this->scheduledInserts) &&
empty($this->scheduledUpdates) &&
empty($this->scheduledRemovals) &&
empty($this->scheduledReorders) &&
empty($this->documentTranslations) &&
empty($this->scheduledMoves)) {
$this->invokeGlobalEvent(Event::onFlush, new ManagerEventArgs($this->dm));
$this->invokeGlobalEvent(Event::postFlush, new ManagerEventArgs($this->dm));

// @deprecated This is to maintain BC with the old behavior, where users may call
// flush instead of PHPCR\SessionInterface#save
$this->session->save();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for removing this in the next major version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. can you open an issue to remind us of that please?


return; // Nothing to do.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have reimplemented this early exit from the ORM. Unfortunately it doesn't seem to avoid the infinite loop in the EventManagerTest. Will have to come back to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event listeners should also take care to not go into endless loops. but this should work so if you can investigate why it does not, that would probably fix some other issues. btw, is there not some public method to ask the dm / uow if there is anything to flush?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find anything in the PHPCR-ODM code, and the ORM does the same as I have done here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing: if somebody was calling flush to trigger a phpcr session save after doing raw phpcr operations, this would be a BC break for him. its an edge case but i would still call session save, it should not hurt.

}

$this->invokeGlobalEvent(Event::onFlush, new ManagerEventArgs($this->dm));

try {
Expand Down Expand Up @@ -2014,6 +2037,8 @@ public function commit($document = null)
$this->visitedCollections =
$this->documentChangesets =
$this->changesetComputed = array();

$this->invokeGlobalEvent(Event::endFlush, new ManagerEventArgs($this->dm));
}

/**
Expand Down
14 changes: 14 additions & 0 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public function testTriggerEvents()
Event::preFlush,
Event::preMove,
Event::postMove,
Event::endFlush,
),
$this->listener
);
Expand All @@ -70,12 +71,14 @@ public function testTriggerEvents()
$this->assertTrue($this->listener->pagePrePersist);
$this->assertFalse($this->listener->itemPrePersist);
$this->assertFalse($this->listener->postFlush);
$this->assertFalse($this->listener->endFlush);
$this->assertFalse($this->listener->preFlush);

$this->dm->flush();

$this->assertTrue($this->listener->onFlush);
$this->assertTrue($this->listener->postFlush);
$this->assertTrue($this->listener->endFlush);
$this->assertTrue($this->listener->preFlush);
$this->assertFalse($this->listener->preUpdate);
$this->assertFalse($this->listener->postUpdate);
Expand Down Expand Up @@ -205,6 +208,7 @@ class TestPersistenceListener
public $itemPostRemove = false;
public $onFlush = false;
public $postFlush = false;
public $endFlush = false;
public $preFlush = false;
public $itemPreMove = false;
public $itemPostMove = false;
Expand Down Expand Up @@ -306,6 +310,16 @@ public function postFlush(ManagerEventArgs $e)
$this->postFlush = true;
}

public function endFlush(ManagerEventArgs $e)
{
$this->endFlush = true;
$dm = $e->getObjectManager();

// endFlush can call ->flush(). The UOW should exit early if there is nothing
// to do, avoiding an infinite recursion.
$dm->flush();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason this still causes an infinite loop event with the early exit. Probably specific to this test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well if other event listeners do any sort of modification, they would be triggered again. if they all only note that the event took place, this flush should not do anything.

}

public function preFlush(ManagerEventArgs $e)
{
$this->preFlush = true;
Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/AttributeTranslationStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ public function testQueryBuilder()
);

$strategy->saveTranslation($data, $node, $this->metadata, 'fr');
$this->dm->flush();
$this->dm->getPhpcrSession()->save();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In accordance with what I said earlier, I don't think we should be calling flush() if we are modifying the PHCPR session directly. The ODM has no knowledge of the inner state of the PHPCR session.


$qb = $this->dm->createQueryBuilder();
$qb->from()->document('Doctrine\Tests\Models\Translation\Article', 'a');
Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/ChildTranslationStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function testSaveTranslation()
$data['topic'] = 'Un sujet intéressant';

$strategy->saveTranslation($data, $node, $this->metadata, 'fr');
$this->dm->flush();
$this->dm->getPhpcrSession()->save();

// Then test we have what we expect in the content repository
$node_en = $this->session->getNode($this->nodeNameForLocale('en'));
Expand Down