PersistenceManager->persistAll() is called without reason, if no unpersisted changes are present

[sorenmalling]

Description

Setting up a new project, where there is no persistence credetnails in Settings.yaml i noticed exceptions eing registred, because the registered signal in Neos.Flow/Package.php for persistAll was called upon a site change (redirect to a login page).

There is no changes to persist at this time, since no objects are changed. So this is a proposal for checking there is any changes to persist after all

Steps to Reproduce

1. setup a requestpattern that triggers a redirect
2. See a exception like this in your logs 20-01-08 12:45:43 96381 ERROR Neos.Flow Exception in line 166 of /Users/soren/Projects/.../Packages/Libraries/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php: An exception occurred in driver: SQLSTATE[HY000] [1045] Access denied for user ''@'localhost' (using password: NO) - See also

Expected behavior

Nothing is persisted, so no reason to try and persist anything

Actual behavior

The error above

Affected Versions

Flow: 6

[sorenmalling]

This a proposed change, in this case only tested for the safeMethod part

        $dispatcher->connect(Mvc\Dispatcher::class, 'afterControllerInvocation', function ($request) use ($bootstrap) {
            // No auto-persistence if there is no PersistenceManager registered or during compile time
            if (
                $bootstrap->getObjectManager()->has(Persistence\PersistenceManagerInterface::class)
                && !($bootstrap->getObjectManager() instanceof CompileTimeObjectManager)
            ) {
                if (!$request instanceof Mvc\ActionRequest || SecurityHelper::hasSafeMethod($request->getHttpRequest()) !== true) {
                    $bootstrap->getObjectManager()->get(Persistence\PersistenceManagerInterface::class)->persistAll();
                } elseif (SecurityHelper::hasSafeMethod($request->getHttpRequest())) {
### LOOK HERE - THIS IS MY CHANGE
                    /** @var PersistenceManagerInterface $pm */
                    $pm = $bootstrap->getObjectManager()->get(Persistence\PersistenceManagerInterface::class);
                    if ($pm->hasUnpersistedChanges()) {
                        $bootstrap->getObjectManager()->get(Persistence\PersistenceManagerInterface::class)->persistAll(true);
                    }
### LOOK HERE - THIS IS MY CHANGE
                }
            }
        });

Notice the hasUnpersistedChanges() condition check. It does a check and no exceptions in the log afterwards

[albe]

That's a good idea to avoid opening a database connection needlessly. I think this needs a small code optimization though, because otherwise this would end up calling doctrines calculateChangeSets() twice or even thrice, which is very costly. One easy optimization would be to check $unitOfWork->size() and if it is zero, just directly bail out (of the hasUnpersistedChanges() check). Another optimization would be to keep information that computeChangeSets() has already been invoked in hasUnpersistedChanges() and not invoke it again in persistAll(), though that will only avoid one of those calls.

However, probably the best solution of all would be to completely change the Doctrine\PersistenceManager such that it only does the whitelist check and the database-reconnect inside doctrine's onFlush() event. This will be invoked immediately after doctrine has calculated changesets, so we can do the hasUnpersistedChanges() check there without having to recalculate everything. And it will also bail out of persisting anything if no changes outstanding.

See https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/events.html#onflush

[sorenmalling]

probably the best solution of all would be to completely change the Doctrine\PersistenceManager

Do that in Flows Neos\Flow\Persistence\Doctrine\PersistenceManager::hasUnpersistedChanges

    public function hasUnpersistedChanges(): bool
    {
        $unitOfWork = $this->entityManager->getUnitOfWork();
        $unitOfWork->computeChangeSets();

        return $unitOfWork->getScheduledEntityInsertions() !== []
            || $unitOfWork->getScheduledEntityUpdates() !== []
            || $unitOfWork->getScheduledEntityDeletions() !== []
            || $unitOfWork->getScheduledCollectionDeletions() !== []
            || $unitOfWork->getScheduledCollectionUpdates() !== [];
    }

Can you point me further to your idea, about allowedObjects checking?

[albe]

The idea was roughly to create a new onFlush EventListener (https://flowframework.readthedocs.io/en/stable/TheDefinitiveGuide/PartIII/Persistence.html#on-the-doctrine-event-system) and then do checks there, because you don't need to call computeChangeSets() any more:

class PersistenceManager extends ... {

    public function persistAll($onlyAllowedObjects = false)
    {
        if (!$this->entityManager->isOpen()) {
            $this->logger->error('persistAll() skipped flushing data, the Doctrine EntityManager is closed. Check the logs for error message.', LogEnvironment::fromMethodName(__METHOD__));
            return;
        }

        $this->entityManager->flush();
        $this->emitAllObjectsPersisted(); // Could (should?) land in a postFlush event
    }

    public function onFlush(OnFlushEventArgs $args) {
        $unitOfWork = $args->getEntityManager()->getUnitOfWork();
        if ($unitOfWork->getScheduledEntityInsertions() === []
            && $unitOfWork->getScheduledEntityUpdates() === []
            && $unitOfWork->getScheduledEntityDeletions() === []
            && $unitOfWork->getScheduledCollectionDeletions() === []
            && $unitOfWork->getScheduledCollectionUpdates() === []
        ) {
            return;
        }

        if (/*NOTE: Need a sane way to transfer this flag over here for the single invocation*/ $onlyAllowedObjects) {
            $objectsToBePersisted = $unitOfWork->getScheduledEntityUpdates() + $unitOfWork->getScheduledEntityDeletions() + $unitOfWork->getScheduledEntityInsertions();
            foreach ($objectsToBePersisted as $object) {
                $this->throwExceptionIfObjectIsNotAllowed($object);
            }
        }

        $connection = $args->entityManager->getConnection();
        try {
            if ($connection->ping() === false) {
                $this->logger->info('Reconnecting the Doctrine EntityManager to the persistence backend.', LogEnvironment::fromMethodName(__METHOD__));
                $connection->close();
                $connection->connect();
            }
        } catch (ConnectionException $exception) {
            $message = $this->throwableStorage->logThrowable($exception);
            $this->logger->error($message, LogEnvironment::fromMethodName(__METHOD__));
        }
     }

Also note that EntityManager::flush already does it's own hasUnpersistedChanges() check and bails out if nothing to do, so we just return.

PS: We should actually deprecate hasUnpersistedChanges() method and remove it at some point. There is no use any longer since the removal of the generic Persistence and the optimization above.

[sorenmalling]

@albe I believe we can close this issue as of now. It wasn't automatically closed by the merge of #2423