Ensure onFlush and postFlush events are always called

[frosas]

No description provided.

[travisbot]

This pull request passes (merged 3b1bc25 into 5d2a3bc).

[Ocramius]

What about raising postFlush here instead? (and moving the triggering logic to its own protected method?)

[frosas]

Whatever you prefer. Notice something similar should be done with onFlush.

[stof]

I don't think calling onFlush is a good idea when nothing is flushed.

[Ocramius]

@stof I must agree it would be kind of expected by the user though...

[stof]

@Ocramius I don't think so: PreUpdate and PostUpdate are only triggered if there is an actual update.

[Ocramius]

@stof then we should eventually change the docs at http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/events.html#lifecycle-events

[Ocramius]

I still like the idea of the patch though :)

[frosas]

@stof preUpdate and postUpdate are lifecycle callbacks (so being attached to the affected entities) while *Flush events are not. To my understanding *Flush events are tied to the fact a flush operation is being executed, not whether the flush ended persisting any changes.

Also, if onFlush/postFlush events end up not being raised if there are no changes, preFlush should also not be raised (to keep the consistency). The same goes for other events like onClear, which now is raised even when nothing is cleared.

[stof]

@frosas there is also PreUpdate and PostUpdate events, not only lifecycle callbacks

[frosas]

@stof oh I didn't noticed this, anyway I still think it is coherent: *Flush events are raised when a flush is executed and *Update events when updates are, event names talk by themselves :)

If there is an interest to not change the current implementation I would at least change the event names to something like onFlushWithChanges and postFlushWithChanges.

[beberlei]

The chnage would be consinstent, you might have an event listener that makes changes and want a flush operation to trigger these changes. However the coding style of the PR is not aligned with Doctrine standards at all. Please rewrite the PR to using guard clauses and early exists instead of the huge realignment through the additional if clause.

[frosas]

Yes, this big if clause is quite ugly and your (and @Ocramius) proposed changes looks fine to me. On the other hand commit() is 100 lines now, another option is to extract the code inside the if clause into its own method:

<?php

public function commit($entity = null)
{
    // Raise preFlush
    if ($this->evm->hasListeners(Events::preFlush)) {
        $this->evm->dispatchEvent(Events::preFlush, new Event\PreFlushEventArgs($this->em));
    }

    // Compute changes done since last commit.
    if ($entity === null) {
        $this->computeChangeSets();
    } else {
        $this->computeSingleEntityChangeSet($entity);
    }

    $anythingToDo =
        $this->entityInsertions ||
        $this->entityDeletions ||
        $this->entityUpdates ||
        $this->collectionUpdates ||
        $this->collectionDeletions ||
        $this->orphanRemovals;

    if ($anythingToDo && $this->orphanRemovals) {
        foreach ($this->orphanRemovals as $orphan) {
            $this->remove($orphan);
        }
    }

    // Raise onFlush
    if ($this->evm->hasListeners(Events::onFlush)) {
        $this->evm->dispatchEvent(Events::onFlush, new Event\OnFlushEventArgs($this->em));
    }

    if ($anythingToDo) {
        $this->commitComputedChangesets();
    }

    // Raise postFlush
    if ($this->evm->hasListeners(Events::postFlush)) {
        $this->evm->dispatchEvent(Events::postFlush, new Event\PostFlushEventArgs($this->em));
    }

    // Clear up
    $this->entityInsertions =
    $this->entityUpdates =
    $this->entityDeletions =
    $this->extraUpdates =
    $this->entityChangeSets =
    $this->collectionUpdates =
    $this->collectionDeletions =
    $this->visitedCollections =
    $this->scheduledForDirtyCheck =
    $this->orphanRemovals = array();
}

I would even extract some more code:

<?php

public function commit($entity = null)
{
    // Raise preFlush
    if ($this->evm->hasListeners(Events::preFlush)) {
        $this->evm->dispatchEvent(Events::preFlush, new Event\PreFlushEventArgs($this->em));
    }

    // Compute changes done since last commit.
    if ($entity === null) {
        $this->computeChangeSets();
    } else {
        $this->computeSingleEntityChangeSet($entity);
    }

    $this->removeOrphans();

    // Raise onFlush
    if ($this->evm->hasListeners(Events::onFlush)) {
        $this->evm->dispatchEvent(Events::onFlush, new Event\OnFlushEventArgs($this->em));
    }

    $this->commitComputedChangesets();

    // Raise postFlush
    if ($this->evm->hasListeners(Events::postFlush)) {
        $this->evm->dispatchEvent(Events::postFlush, new Event\PostFlushEventArgs($this->em));
    }

    $this->cleanUp();
}

private function removeOrphans()
{
    if ($this->orphanRemovals) {
        foreach ($this->orphanRemovals as $orphan) {
            $this->remove($orphan);
        }
    }
}

private commitComputedChangesets()
{
    if (! $this->hasComputedChangesets()) {
        return;
    }

    // ...
}

private function hasComputedChangesets()
{
    return 
        $this->entityInsertions ||
        $this->entityDeletions ||
        $this->entityUpdates ||
        $this->collectionUpdates ||
        $this->collectionDeletions ||
        $this->orphanRemovals;
}

[Ocramius]

Since the usage of the listeners is done only twice, I'd wrap that piece in a private method instead...

[travisbot]

This pull request passes (merged 7a9f96c into 5d2a3bc).

[Ocramius]

Looks good to me :)

[frosas]

There is one thing that bugs me about this PR. I wrote it against version 2.2 but, if it is accepted, it should be also applied to 2.3 and master. Can it be done in a single PR?

[Ocramius]

@frosas it can be merged to different branches (can be done in git :) )

[guilhermeblanco]

-2 from me.

1- Since you are not flushing anything (no entities), it should not trigger an operation that is not being executed.
2- Commit is long for performance reasons. By decoupling a single method makes around 15% slower the overall execution. It's a performance sensitive method, we cannot decouple that one, unfortunately,

[guilhermeblanco]

If you want a single attempt of this being accepted, please enable the performance tests and execute the code before and after your patch. If there's no huge performance impact, we may consider the second point as +1 from me.

Please post both results here when you try it.

[frosas]

@guilhermeblanco

1. I'll quote myself:

   To my understanding *Flush events are tied to the fact a flush operation is being executed, not whether the flush ended persisting any changes.

   Also, if onFlush/postFlush events end up not being raised if there are no changes, preFlush should also not be raised (to keep the consistency). The same goes for other events like onClear, which now is raised even when nothing is cleared.

2. How do I run those performance tests? There is no reference about it in http://www.doctrine-project.org/contribute.html and don't know why but a phpunit --group performance gives me 0 assertions.

Also this PR is because I wanted to detect flush overlappings but I managed to do it without these changes so I have no inconvenience in closing it.

[Ocramius]

@guilhermeblanco also, the commit method is usually called only once :)

[beberlei]

@guilhermeblanco the performance is neglectable here - plus performance tests don't test this. If you have an on flush event you normally check for "give me all inserted etntities", "give me all changed entities" etc, but these are empty anyways, so nothing happens in listeners.

@frosas can you close this PR and open a new one against master? we dont merge against versions.

[beberlei]

Closing now, this should be opened on master.

[frosas]

Any idea on how to easily apply these changes to master? Otherwise I'll create a new branch against master and PR it.