Avoid calling flush() in logger

This is to avoid unintended side effect of trying to save modified
or detached entities.

[MAILPOET-5745]

mailpoet/lib/Logging/LogHandler.php

@@ -1,10 +1,8 @@
-<?php // phpcs:ignore SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing
+<?php declare(strict_types = 1);
 
 namespace MailPoet\Logging;
 
-use MailPoet\Doctrine\EntityManagerFactory;
 use MailPoet\Entities\LogEntity;
-use MailPoetVendor\Doctrine\ORM\EntityManager;
 use MailPoetVendor\Monolog\Handler\AbstractProcessingHandler;
 
 class LogHandler extends AbstractProcessingHandler {
@@ -30,16 +28,8 @@ class LogHandler extends AbstractProcessingHandler {
   /** @var LogRepository */
   private $logRepository;
 
-  /** @var EntityManager */
-  private $entityManager;
-
-  /** @var EntityManagerFactory */
-  private $entityManagerFactory;
-
   public function __construct(
     LogRepository $logRepository,
-    EntityManager $entityManager,
-    EntityManagerFactory $entityManagerFactory,
     $level = \MailPoetVendor\Monolog\Logger::DEBUG,
     $bubble = \true,
     $randFunction = null
@@ -47,8 +37,6 @@ class LogHandler extends AbstractProcessingHandler {
     parent::__construct($level, $bubble);
     $this->randFunction = $randFunction;
     $this->logRepository = $logRepository;
-    $this->entityManager = $entityManager;
-    $this->entityManagerFactory = $entityManagerFactory;
   }
 
   protected function write(array $record): void {
@@ -60,13 +48,7 @@ class LogHandler extends AbstractProcessingHandler {
     $entity->setCreatedAt($record['datetime']);
     $entity->setRawMessage($record['message']);
     $entity->setContext($record['context']);
-
+    $this->logRepository->saveLog($entity);
-    if (!$this->entityManager->isOpen()) {
-      $this->entityManager = $this->entityManagerFactory->createEntityManager();
-      $this->logRepository = new LogRepository($this->entityManager);
-    }
-    $this->logRepository->persist($entity);
-    $this->logRepository->flush();
 
     if ($this->getRandom() <= self::LOG_PURGE_PROBABILITY) {
       $this->purgeOldLogs();

mailpoet/lib/Logging/LogRepository.php

@@ -5,16 +5,53 @@ namespace MailPoet\Logging;
 use MailPoet\Doctrine\Repository;
 use MailPoet\Entities\LogEntity;
 use MailPoet\Entities\NewsletterEntity;
+use MailPoet\InvalidStateException;
 use MailPoet\Util\Helpers;
+use MailPoet\WP\Functions;
 use MailPoetVendor\Carbon\Carbon;
 use MailPoetVendor\Doctrine\DBAL\Driver\PDO\Connection;
+use MailPoetVendor\Doctrine\ORM\EntityManager;
 
 /**
  * @extends Repository<LogEntity>
  */
 class LogRepository extends Repository {
-  protected function getEntityClassName() {
+  /** @var Functions */
-    return LogEntity::class;
+  private $wp;
+
+  public function __construct(
+    EntityManager $entityManager,
+    Functions $wp
+  ) {
+    parent::__construct($entityManager);
+    $this->wp = $wp;
+  }
+
+  public function saveLog(LogEntity $log): void {
+    // Save log entity using DBAL to avoid calling "flush()" on the entity manager.
+    // Calling "flush()" can have unintended side effects, such as saving unwanted
+    // changes or trying to save entities that were detached from the entity manager.
+    $this->entityManager->getConnection()->insert(
+      $this->entityManager->getClassMetadata(LogEntity::class)->getTableName(),
+      [
+        'name' => $log->getName(),
+        'level' => $log->getLevel(),
+        'message' => $log->getMessage(),
+        'raw_message' => $log->getRawMessage(),
+        'context' => json_encode($log->getContext()),
+        'created_at' => (
+          $log->getCreatedAt() ?? Carbon::createFromTimestamp($this->wp->currentTime('timestamp'))
+        )->format('Y-m-d H:i:s'),
+      ],
+    );
+
+    // sync the changes with the entity manager
+    if ($this->entityManager->isOpen()) {
+      $lastInsertId = (int)$this->entityManager->getConnection()->lastInsertId();
+      $log->setId($lastInsertId);
+      $this->entityManager->getUnitOfWork()->registerManaged($log, ['id' => $log->getId()], []);
+      $this->entityManager->refresh($log);
+    }
   }
 
   /**
@@ -94,4 +131,16 @@ class LogRepository extends Repository {
       ->getQuery()
       ->getSingleColumnResult();
   }
+
+  public function persist($entity): void {
+    throw new InvalidStateException('Use saveLog() instead to avoid unintended side effects');
+  }
+
+  public function flush(): void {
+    throw new InvalidStateException('Use saveLog() instead to avoid unintended side effects');
+  }
+
+  protected function getEntityClassName() {
+    return LogEntity::class;
+  }
 }

mailpoet/lib/Logging/LoggerFactory.php

@@ -3,9 +3,7 @@
 namespace MailPoet\Logging;
 
 use MailPoet\DI\ContainerWrapper;
-use MailPoet\Doctrine\EntityManagerFactory;
 use MailPoet\Settings\SettingsController;
-use MailPoetVendor\Doctrine\ORM\EntityManager;
 use MailPoetVendor\Monolog\Processor\IntrospectionProcessor;
 use MailPoetVendor\Monolog\Processor\MemoryUsageProcessor;
 use MailPoetVendor\Monolog\Processor\WebProcessor;
@@ -50,22 +48,12 @@ class LoggerFactory {
   /** @var LogRepository */
   private $logRepository;
 
-  /** @var EntityManager */
-  private $entityManager;
-
-  /** @var EntityManagerFactory */
-  private $entityManagerFactory;
-
   public function __construct(
     LogRepository $logRepository,
-    EntityManager $entityManager,
-    EntityManagerFactory $entityManagerFactory,
     SettingsController $settings
   ) {
     $this->settings = $settings;
     $this->logRepository = $logRepository;
-    $this->entityManager = $entityManager;
-    $this->entityManagerFactory = $entityManagerFactory;
   }
 
   /**
@@ -92,8 +80,6 @@ class LoggerFactory {
 
       $this->loggerInstances[$name]->pushHandler(new LogHandler(
         $this->logRepository,
-        $this->entityManager,
-        $this->entityManagerFactory,
         $this->getDefaultLogLevel()
       ));
     }
@@ -104,8 +90,6 @@ class LoggerFactory {
     if (!self::$instance instanceof LoggerFactory) {
       self::$instance = new LoggerFactory(
         ContainerWrapper::getInstance()->get(LogRepository::class),
-        ContainerWrapper::getInstance()->get(EntityManager::class),
-        ContainerWrapper::getInstance()->get(EntityManagerFactory::class),
         SettingsController::getInstance()
       );
     }

mailpoet/tests/DataFactories/Log.php

@@ -33,13 +33,11 @@ class Log {
 
   public function create(): LogEntity {
     $entity = new LogEntity();
-    $entity->setName( $this->data['name']);
+    $entity->setName($this->data['name']);
     $entity->setLevel($this->data['level']);
     $entity->setMessage($this->data['message']);
     $entity->setCreatedAt($this->data['created_at']);
-
+    $this->repository->saveLog($entity);
-    $this->repository->persist($entity);
-    $this->repository->flush();
     return $entity;
   }
 

mailpoet/tests/integration/Logging/LogHandlerTest.php

@@ -5,6 +5,7 @@ namespace MailPoet\Logging;
 use MailPoet\Doctrine\EntityManagerFactory;
 use MailPoet\Entities\LogEntity;
 use MailPoet\Entities\SubscriberEntity;
+use MailPoet\WP\Functions as WPFunctions;
 use MailPoetVendor\Carbon\Carbon;
 use MailPoetVendor\Doctrine\ORM\EntityManager;
 
@@ -21,11 +22,7 @@ class LogHandlerTest extends \MailPoetTest {
   }
 
   public function testItCreatesLog() {
-    $logHandler = new LogHandler(
+    $logHandler = new LogHandler($this->repository);
-      $this->repository,
-      $this->entityManager,
-      $this->entityManagerFactory
-    );
     $time = new \DateTime();
     $logHandler->handle([
       'level' => \MailPoetVendor\Monolog\Logger::EMERGENCY,
@@ -49,9 +46,7 @@ class LogHandlerTest extends \MailPoetTest {
     $entity->setLevel(5);
     $entity->setMessage('xyz');
     $entity->setCreatedAt(Carbon::now()->subDays(100));
-
+    $this->repository->saveLog($entity);
-    $this->repository->persist($entity);
-    $this->repository->flush();
 
     $random = function() {
       return 0;
@@ -59,8 +54,6 @@ class LogHandlerTest extends \MailPoetTest {
 
     $logHandler = new LogHandler(
       $this->repository,
-      $this->entityManager,
-      $this->entityManagerFactory,
       \MailPoetVendor\Monolog\Logger::DEBUG,
       true,
       $random
@@ -84,9 +77,7 @@ class LogHandlerTest extends \MailPoetTest {
     $entity->setLevel(5);
     $entity->setMessage('xyz');
     $entity->setCreatedAt(Carbon::now()->subDays(100));
-
+    $this->repository->saveLog($entity);
-    $this->repository->persist($entity);
-    $this->repository->flush();
 
     $random = function() {
       return 100;
@@ -94,8 +85,6 @@ class LogHandlerTest extends \MailPoetTest {
 
     $logHandler = new LogHandler(
       $this->repository,
-      $this->entityManager,
-      $this->entityManagerFactory,
       \MailPoetVendor\Monolog\Logger::DEBUG,
       true,
       $random
@@ -115,12 +104,8 @@ class LogHandlerTest extends \MailPoetTest {
 
   public function testItResilientToSqlError(): void {
     $entityManager = $this->entityManagerFactory->createEntityManager();
-    $logRepository = new LogRepository($entityManager);
+    $logRepository = new LogRepository($entityManager, new WPFunctions());
-    $logHandler = new LogHandler(
+    $logHandler = new LogHandler($logRepository);
-      $logRepository,
-      $entityManager,
-      $this->entityManagerFactory
-    );
     $time = new \DateTime();
 
     try {

mailpoet/tests/unit/Logging/LoggerFactoryTest.php

@@ -2,9 +2,7 @@
 
 namespace MailPoet\Logging;
 
-use MailPoet\Doctrine\EntityManagerFactory;
 use MailPoet\Settings\SettingsController;
-use MailPoetVendor\Doctrine\ORM\EntityManager;
 use MailPoetVendor\Monolog\Handler\AbstractHandler;
 use PHPUnit\Framework\MockObject\MockObject;
 
@@ -20,9 +18,7 @@ class LoggerFactoryTest extends \MailPoetUnitTest {
     parent::_before();
     $this->settings = $this->createMock(SettingsController::class);
     $repository = $this->createMock(LogRepository::class);
-    $entityManager = $this->createMock(EntityManager::class);
+    $this->loggerFactory = new LoggerFactory($repository, $this->settings);
-    $entityManagerFactory = $this->createMock(EntityManagerFactory::class);
-    $this->loggerFactory = new LoggerFactory($repository, $entityManager, $entityManagerFactory, $this->settings);
   }
 
   public function testItCreatesLogger() {