xiidea / EasyAuditBundle

A Symfony Bundle To Log Selective Events
http://xiidea.github.io/EasyAuditBundle/
MIT License
89 stars 22 forks source link

Saving collection of entity form types problem: PreUpdateEventArgs::__construct() must be of the type array, null given #27

Closed pribeirojtm closed 6 years ago

pribeirojtm commented 6 years ago

Hi,

Uncaught PHP Exception Symfony\Component\Debug\Exception\FatalThrowableError: "Type error: Argument 3 passed to Doctrine\ORM\Event\PreUpdateEventArgs::construct() must be of the type array, null given, called in /var/www/app/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php on line 1060" at /var/www/app/vendor/doctrine/orm/lib/Doctrine/ORM/Event/PreUpdateEventArgs.php line 46 {"exception":"[object] (Symfony\Component\Debug\Exception\FatalThrowableError(code: 0): Type error: Argument 3 passed to Doctrine\ORM\Event\PreUpdateEventArgs::construct() must be of the type array, null given, called in /var/www/app/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php on line 1060 at /var/wwwapp/vendor/doctrine/orm/lib/Doctrine/ORM/Event/PreUpdateEventArgs.php:46)"}

}

- I'm posting here the simplest code and clear as possible to you get a quick understand:
```php
<?php 
// MyController.php Class:
class MyController extends Controller
{
    //....

    public function editMawbFlightsFactModalAction(Mawb $mawb, Request $request) {

        $entityManager = $this->getDoctrine()->getManager();

        $form = $this->createForm(MawbFlightsFactType::class, $mawb);

        if ($request->isMethod('POST')) {

            $form->submit($request->request->get($form->getName()), false);

            // - Handle submitted data
            if ($form->isSubmitted() && $form->isValid()) {

                // @var Mawb $mawb
                $mawb = $form->getData();

                $entityManager = $this->getDoctrine()->getManager();
                $entityManager->persist($mawb);
                $entityManager->flush();

                return new JsonResponse(['msg' => 'ok']);
            }
        }

        return $this->render('app/editMawbFlightsFact.html.twig',
            array(
                'form' => $form->createView(),
                'mawb' => $mawb
            )
        );
    }
}

//-----------------------------------------------------------------------------------------
//Mawb.php Entity Class:
namespace AppBundle\Entity;
use Doctrine\ORM\Mapping as ORM;

/**
 * Mawb
 *
 * @ORM\Table(name="mawb")
 * @ORM\Entity
 */
class Mawb
{
    /**
     * @var integer
     *
     * @ORM\Column(name="id_mawb", type="integer", nullable=false, unique=false)
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private $idMawb;

    //... other entity properties......

    /**
     * @var \Doctrine\Common\Collections\Collection
     *
     * @ORM\OneToMany(targetEntity="FlightsFactMawb", mappedBy="mawb", orphanRemoval=true, cascade={"persist"})
     */
    private $flightsFactMawbCollection;

    public function __construct() {
        $this->flightsFactMawbCollection = new ArrayCollection();
    }

    //...getters and setters..
}

//-----------------------------------------------------------------------------------------
//MawbFlightsFactType.php form Type
class MawbFlightsFactType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options) {
        $builder
            ->add('flightsFactMawbCollection', CollectionType::class, array(
                'entry_type' => FlightFactMawbType::class,
                'by_reference' => false,
                'prototype' => true
            ));
    }

    public function configureOptions(OptionsResolver $resolver) {
        $resolver->setDefaults(['data_class' => Mawb::class]);
    }
}

//-----------------------------------------------------------------------------------------
//FlightFactMawbType.php form Type
class FlightFactMawbType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options) {
        $builder
            ->add('flightFact', FlightFactType::class,['data_class' => FlightsFact::class])
            ->add('assigned');   
    }

    public function configureOptions(OptionsResolver $resolver) {
        $resolver->setDefaults(['data_class' => FlightsFactMawb::class]);
    }
}

//-----------------------------------------------------------------------------------------
//FlightsFactMawb.php Entity Class:
namespace AppBundle\Entity;
use Doctrine\ORM\Mapping as ORM;
use AppBundle\Entity\Traits\CreatedUpdatedAtTrait;

/**
 * FlightsFactMawb
 *
 * @ORM\Table(name="flights_fact_mawb")
 * @ORM\Entity
 * @ORM\HasLifecycleCallbacks
 */
class FlightsFactMawb
{
    use CreatedUpdatedAtTrait;

    /**
     * @var integer
     *
     * @ORM\Column(name="id", type="integer", nullable=false)
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private $id;

    /**
     * @var boolean
     *
     * @ORM\Column(name="assigned", type="boolean", nullable=false, unique=false, options={"default" : "1"})
     */
    private $assigned;

    // .... other entity properties.....

    /**
     * @var FlightsFact
     *
     * @ORM\ManyToOne(targetEntity="FlightsFact", cascade={"persist"})
     * @ORM\JoinColumns({
     *   @ORM\JoinColumn(name="id_flights_fact", referencedColumnName="id_flights_fact", nullable=false, onDelete="CASCADE")
     * })
     */
    private $flightFact;

    //...getters and setters..
}

//-----------------------------------------------------------------------------------------
//FlightsFact.php Entity Class:
namespace AppBundle\Entity;
use Doctrine\ORM\Mapping as ORM;
use AppBundle\Entity\Traits\CreatedUpdatedAtTrait;

/**
 * @ORM\Table(name="flights_fact")
 * @ORM\Entity
 * @ORM\HasLifecycleCallbacks
 */
class FlightsFact
{
    use CreatedUpdatedAtTrait;

    /**
     * @var integer
     *
     * @ORM\Column(name="id_flights_fact", type="integer", nullable=false, unique=false)
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private $idFlightFact;

    /**
     * @var string
     *
     * @ORM\Column(name="number", type="string", length=5, nullable=true, unique=false)
     */
    private $number;

    //...

    /**
     * @ORM\PreUpdate
     */
    public function preUpdateScheduledEpochs() {
        if(!empty($this->getNumber()) {
            $this->setNumber($this->getNumber().'*');
        }
    }
}

//-----------------------------------------------------------------------------------------
//FlightFactMawbType.php form Type
class FlightFactType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options) {

        $builder
            ->add('number', null, ['required' => true]);
    }

    public function configureOptions(OptionsResolver $resolver) {
        $resolver->setDefaults(array('data_class' => FlightsFact::class));
    }
}

//------------------------------------------------------------------------------------------
// CreatedUpdatedAtTrait.php
namespace AppBundle\Entity\Traits;

use Doctrine\ORM\Event\PreUpdateEventArgs;
use Doctrine\ORM\Mapping as ORM;

trait CreatedUpdatedAtTrait
{
    /**
     * @var \DateTime
     * @ORM\Column(name="created_at", type="datetime", nullable=false)
     */
    protected $createdAt;

    /**
     * @var \DateTime
     * @ORM\Column(name="updated_at", type="datetime", nullable=false)
     */
    protected $updatedAt;

    /**
     * @ORM\PrePersist
     */
    public function onPrePersist() {
        $this->updatedAt = new \DateTime('now');
        $this->createdAt = $this->createdAt == null ? $this->updatedAt : $this->createdAt;
    }

    /**
     * @ORM\PreUpdate
     */
    public function onPreUpdate() {
        $this->updatedAt = new \DateTime('now');
        if (!$this->createdAt) {
            $this->createdAt = new \DateTime('now');
        }
    }

    /**
     * @return \DateTime
     */
    public function getCreatedAt() {
        return $this->createdAt;
    }

    /**
     * @param \DateTime $createdAt
     * @return $this
     */
    public function setCreatedAt($createdAt) {
        $this->createdAt = $createdAt;
        return $this;
    }

    /**
     * @return \DateTime
     */
    public function getUpdatedAt() {
        return $this->updatedAt;
    }

    /**
     * @param \DateTime $updatedAt
     * @return $this
     */
    public function setUpdatedAt($updatedAt) {
        $this->updatedAt = $updatedAt;
        return $this;
    }
}

Can you give a hand on this? Can you reproduce the error? Thanks.

ronisaha commented 6 years ago

Hi @pribeirojtm ,

I'll look into it when I could make some time. It would be great help for me if you could create a symfony app with enough code to reproduce it. Then I can just test with that and ensure faster response from me. Else It requires some extra time to setup an app with specific use cases. You see, easy audit is not created with only doctrine logger in mind, there can be some other implementation of logger like, filesystem, email, logger with queueing support etc. The doctrine-logger just an implementation example which can handle most case.

To handle specific use case you can create your own logger by extending existing one or just implementing the LoggerInterface.

pribeirojtm commented 6 years ago

Hi @ronisaha,

I've tried to setup a new application with the code to reproduce the problem, but until now, I still don't know exactly what is really causing the problem, so I don't have the exact case to you to reproduce, but I have some clues or suspicions of what is causing the problem. I also have some questions for you, If you can help me to get clarified, It could help me/us the point a direction on the solution.

[2018-06-27 15:39:33] doctrine.DEBUG: "START TRANSACTION" [] [] [2018-06-27 15:39:33] doctrine.DEBUG: UPDATE currency SET currency_value = ? WHERE currency_id = ? [1274.6,14] [] [2018-06-27 15:39:33] doctrine.DEBUG: INSERT INTO audit_log (type_id, type, description, event_time, user, ip, change_sets, id_fos_user, id_processo) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) {"1":"easy_audit.doctrine.entity [...]","2":"Currency updated","3":"Currency has been updated [...]","4":"2018-06-27 15:39:33","5":"pribeiro","6":"127.0.0.1","7":"a:1:{s:13:\"currencyValue\"; [...]","8":76,"9":null} [] [2018-06-27 15:39:33] doctrine.DEBUG: "COMMIT" [] []

[2018-06-27 15:37:59] doctrine.DEBUG: "START TRANSACTION" [] [] [2018-06-27 15:37:59] doctrine.DEBUG: DELETE FROM currency WHERE currency_id = ? [23] [] [2018-06-27 15:37:59] doctrine.DEBUG: "COMMIT" [] [] [2018-06-27 15:37:59] doctrine.DEBUG: "START TRANSACTION" [] [] [2018-06-27 15:37:59] doctrine.DEBUG: INSERT INTO audit_log (type_id, type, description, event_time, user, ip, change_sets, id_fos_user, id_processo) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) {"1":"easy_audit.doctrine.entity [...]","2":"Currency deleted","3":"Currency has been deleted [...]","4":"2018-06-27 15:37:59","5":"pribeiro","6":"127.0.0.1","7":"a:0:{}","8":76,"9":null} [] [2018-06-27 15:37:59] doctrine.DEBUG: "COMMIT" [] []

I don't know which one is the better approach, but the "deleted" one I think is also valid and makes sense.

<?php 
//Logger.php
protected function saveLog(BaseAuditLog $event)
{
    $this->getEntityManager()->persist($event);

    $connection = $this->getEntityManager()->getConnection();
    //The nesting level. A value of 0 means there's no active transaction.
    if ($connection->getTransactionNestingLevel() > 0) {
        // do nothing... we're already within a transaction started by a flush...
    } else {
        $this->getEntityManager()->flush($event);
    }
}

Other notes:

Hope we can detect and solve this issue. I'll continue to check what can I do.. Hope this insight help you to understand my problem and my questions.

Thanks.

ronisaha commented 6 years ago

Questions:

  1. Are you saving any reference object in AuditLog entity?
  2. As you said, Have you debug the conflicting with your LifecycleCallback using logger

I dont know if it is possible to have any kind of doctrine PreUpdate /PrePersist events conflits in this case, since I have * @Orm\HasLifecycleCallbacks in some entities with PreUpdate /PrePersist.

  1. Have you tried to disable the log for a entity to find out which AuditLog triggering the error.
  2. Have you tried to disable the library, to check if the problem with your code, not within the library.

Observation/Sugestations/Answers

  1. The default logger implementation is doing immediate log entry to database. And the flush operation is doing only for the log entity. So if the log entity does not have any reference field then there should not be any problem in it.

  2. As my memory serves, I think LifecycleCallback calls before doctrine subscribed event. But we need to be sure. If in your case LifecycleCallback conflicting with it, we may need to tweak with the priority of doctrine subscribed events

  3. By disabling particular entity log, you may can narrow it down with the root cause of your problem.

  4. Some time I've faced some wired problem, and by digging into that most of the time I found problem with my nested entity collection. The parent entity were not persisted or flushed property to be able to create the child entity. So you can disable the library to debug that first.

  5. You can always extend the current logger implementation and implement your hypothesis.

  6. There may be different approach to solve your problem.

    • For advance performant approach you can write a logger with queuing support to postpond the log entry. This will not block or interfere with your transaction block.
    • You can exclude the problematic event from logging an dispatch a custom event and register to log that
    • I'm sure you can find many more workaround if you give a thought... :)

But if the problem is reproducible, I would love to investigate more to find the root cause of this error. So if you can share a reproducible source code; I'll look into that.

Thanks

pribeirojtm commented 6 years ago

Hi,

Regarding your questions:

  1. Are you saving any reference object in AuditLog entity?

I was, like having an FK fosUser in AuditLog entity, associated with User entity, I've removed now, but I think that was not the problem.

  1. As you said, Have you debug the conflicting with your LifecycleCallback using logger

I've tried, but since it seems to me very complex to debug and I dont understand very much the flow, I am lost in the code.. :(

  1. Have you tried to disable the log for a entity to find out which AuditLog triggering the error.

From what I've tested, I reached the conclusion that was the entities in the collection that have lifecycle callbacks

  1. Have you tried to disable the library, to check if the problem with your code, not within the library.

Yes. Disabling library, collection is perfectly persisted without any errors.

Observation/Sugestations/Answers

  1. The default logger implementation is doing immediate log entry to database. And the flush operation is doing only for the log entity. So if the log entity does not have any reference field then there should not be any problem in it.

As I've tested early and said in previous post, it seems that is occurring for ENTITY_UPDATE and ENTITY_CREATED events, but not now for DELETE_ENTITY as the code in the master branch.

My question: Can I overwrite the DoctrineSubscriber? How? I'm asking this because the first approach you suggested:

For advance performant approach you can write a logger with queuing support to postpond the log entry. This will not block or interfere with your transaction block.

I think it avoids any possible problem regarding lifecyclecallbacks conflicts, or order of execution. I've tried postponing changing the code to postponing and now It logs with no problem. The only thing is that it is creating a single transaction for each new audit_log record..

I'm saying this, because I've tested the code and now It works, it saves and logs with no problem.

I've changed your DoctrineSubscriber code to this (but I want to know if you can help me postponing correctly the audit_log savings (added preUpdate and changed postUpdate. I've also renamed toBeDeleted to toBeLogged):

<?php

/*
 * This file is part of the XiideaEasyAuditBundle package.
 *
 * (c) Xiidea <http://www.xiidea.net>
 *
 * This source file is subject to the MIT license that is bundled
 * with this source code in the file LICENSE.
 */

namespace Xiidea\EasyAuditBundle\Subscriber;

use Doctrine\Common\EventSubscriber;
use Doctrine\Common\Util\ClassUtils;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerAwareTrait;
use Xiidea\EasyAuditBundle\Events\DoctrineEntityEvent;
use Xiidea\EasyAuditBundle\Events\DoctrineEvents;

class DoctrineSubscriber implements ContainerAwareInterface, EventSubscriber
{
    use ContainerAwareTrait;

    /** @var \Doctrine\Common\Annotations\Reader */
    private $annotationReader;

    private $toBeLogged = [];

    /**
     * @var array
     */
    private $entities;

    public function __construct($entities = array())
    {
        $this->entities = $entities;
    }

    public function getSubscribedEvents()
    {
        return array(
            'postPersist',
            'preUpdate',
            'postUpdate',
            'preRemove',
            'postRemove'
        );
    }

    public function postPersist(LifecycleEventArgs $args)
    {
        $this->handleEvent(DoctrineEvents::ENTITY_CREATED, $args);
    }

    public function postUpdate(LifecycleEventArgs $args)
    {
        //$this->handleEvent(DoctrineEvents::ENTITY_UPDATED, $args);

        $identity = $this->getToBeLoggedId($args->getEntity());

        if ($identity) {
            $this->container->get('event_dispatcher')->dispatch(DoctrineEvents::ENTITY_UPDATED,
                new DoctrineEntityEvent($args, $identity)
            );
        }
    }

    public function preRemove(LifecycleEventArgs $args)
    {
        if (false === $this->isConfiguredToTrack($args->getEntity(), DoctrineEvents::ENTITY_DELETED)) {
            return;
        }

        $className = ClassUtils::getClass($args->getEntity());

        if (!isset($this->toBeLogged[$className])) {
            $this->toBeLogged[$className] = [];
        }

        $this->toBeLogged[$className][spl_object_hash($args->getEntity())] = $this->getIdentity($args, $className);
    }

    public function preUpdate(LifecycleEventArgs $args)
    {
        if (false === $this->isConfiguredToTrack($args->getEntity(), DoctrineEvents::ENTITY_UPDATED)) {
            return;
        }

        $className = ClassUtils::getClass($args->getEntity());

        if (!isset($this->toBeLogged[$className])) {
            $this->toBeLogged[$className] = [];
        }

        $this->toBeLogged[$className][spl_object_hash($args->getEntity())] = $this->getIdentity($args, $className);
    }

    public function postRemove(LifecycleEventArgs $args)
    {
        $identity = $this->getToBeLoggedId($args->getEntity());

        if ($identity) {
            $this->container->get('event_dispatcher')->dispatch(DoctrineEvents::ENTITY_DELETED,
                new DoctrineEntityEvent($args, $identity)
            );
        }
    }

    private function getToBeLoggedId($entity)
    {
        try{
            return $this->toBeLogged[ClassUtils::getClass($entity)][spl_object_hash($entity)];
        }catch (\Exception $exception) {
            return false;
        }
    }

    /**
     * @param string $eventName
     * @param LifecycleEventArgs $args
     */
    private function handleEvent($eventName, LifecycleEventArgs $args)
    {
        if (true === $this->isConfiguredToTrack($args->getEntity(), $eventName)) {
            $this->container->get('event_dispatcher')->dispatch($eventName,
                new DoctrineEntityEvent($args, $this->getIdentity($args, ClassUtils::getClass($args->getEntity())))
            );
        }
    }

    /**
     * @param $entity
     * @param string $eventName
     * @return bool
     */
    private function isConfiguredToTrack($entity, $eventName = '')
    {
        $class = ClassUtils::getClass($entity);
        $eventType = DoctrineEvents::getShortEventType($eventName);

        if (null !== $track = $this->isAnnotatedEvent($entity, $eventType)) {
            return $track;
        }

        if (!$this->isConfigured($class)) {
            return FALSE;
        }

        if ($this->shouldTrackAllEventType($class)) {
            return TRUE;
        }

        return $this->shouldTrackEventType($eventType, $class);
    }

    /**
     * @param $entity
     * @param string $eventType
     * @return bool|null
     */
    protected function isAnnotatedEvent($entity, $eventType)
    {
        $metaData = $this->hasAnnotation($entity);

        if (!$metaData) {
            return null;
        }

        return empty($metaData->events) || in_array($eventType, $metaData->events);
    }

    /**
     * @param $entity
     * @return null|object
     */
    protected function hasAnnotation($entity)
    {
        $reflection = $this->getReflectionClassFromObject($entity);

        return $this
            ->getAnnotationReader()
            ->getClassAnnotation($reflection, 'Xiidea\EasyAuditBundle\Annotation\ORMSubscribedEvents');

    }

    /**
     * @return \Doctrine\Common\Annotations\Reader
     */
    protected function getAnnotationReader()
    {
        return $this->annotationReader;
    }

    /**
     * @param $object
     * @return \ReflectionClass
     */
    protected function getReflectionClassFromObject($object)
    {
        $class = ClassUtils::getClass($object);

        return new \ReflectionClass($class);
    }

    /**
     * @param string $eventType
     * @param string $class
     * @return bool
     */
    private function shouldTrackEventType($eventType, $class)
    {
        return (is_array($this->entities[$class]) && in_array($eventType, $this->entities[$class]));
    }

    /**
     * @param string $class
     * @return bool
     */
    private function shouldTrackAllEventType($class)
    {
        return empty($this->entities[$class]);
    }

    /**
     * @param string $class
     * @return bool
     */
    protected function isConfigured($class)
    {
        return isset($this->entities[$class]);
    }

    /**
     * @param \Doctrine\Common\Annotations\Reader $annotationReader
     */
    public function setAnnotationReader($annotationReader = null)
    {
        $this->annotationReader = $annotationReader;
    }

    /**
     * @param LifecycleEventArgs $args
     * @param $className
     * @return array
     */
    protected function getIdentity(LifecycleEventArgs $args, $className)
    {
        return $args->getEntityManager()->getClassMetadata($className)->getIdentifierValues($args->getEntity());
    }
}

I've created a CustomLogger and deactivated your default Logger, to make this change [CODE UPDATED]:

<?php

namespace AppBundle\Logger;

use Doctrine\ORM\EntityManager;
use Xiidea\EasyAuditBundle\Entity\BaseAuditLog;
//use AppBundle\Entity\AuditLog;
use Doctrine\Bundle\DoctrineBundle\Registry;
use Xiidea\EasyAuditBundle\Logger\LoggerInterface;
use Xiidea\EasyAuditBundle\Events\DoctrineEvents;

class CustomLogger implements LoggerInterface
{

    private $entityLogs = [];

    /**
     * @var \Doctrine\Bundle\DoctrineBundle\Registry
     */
    private $doctrine;

    public function __construct(Registry $doctrine)
    {
        $this->doctrine = $doctrine;
    }

    public function log(BaseAuditLog $event = null)
    {
        if(empty($event)) {
            return;
        }

        //FOR ALL 3 TYPES OF EVENTS. Changed the name of the property
        $this->entityLogs[] = $event;

    }

    /**
     * @return EntityManager
     */
    protected function getEntityManager()
    {
        return $this->getDoctrine()->getManager();
    }

    /**
     * @return \Doctrine\Bundle\DoctrineBundle\Registry
     */
    public function getDoctrine()
    {
        return $this->doctrine;
    }

    public function savePendingLogs()
    {
        foreach ($this->entityLogs as $log) {
            $this->getEntityManager()->persist($log);
        }

        $this->getEntityManager()->flush();

        $this->entityLogs = [];
    }

}

Also, DoctrineDeleteEventLogger Subscriber class name and alias, could be renamed to DoctrineEventLoggerSubscriber.

ronisaha commented 6 years ago

I don't want to change the DoctrineSubscriber and DoctrineDeleteEventLogger Classes. I think these classes doing exactly what should they do. We can create different logger or update the logger implementation.

pribeirojtm commented 6 years ago

Do you agree on passing all the savings to onKernelReponse to avoid conflits with LifeCycleCallbacks? I am suggesting to apply the same rules that were applied for DELETE events, to UPDATE AND CREATE events. Doing this, I'm not having problems on persisting collection of forms with lifecyclecallbacks. There's a specific transaction now to save those pending audit_log records... If you dont agree, what can I do to overcome the issue? I only see If I somehow overritwe the implementation of DoctrineSubscriber. Can you help me on this?

pribeirojtm commented 6 years ago

Hi @ronisaha ,

I've tested now with your DoctrineSubscriber, without my modifications, and with my custom logger and..... it works!

So, sorry If I was suggesting unwanted/unneeded/unnecessary changes regarding preUpdate events... I thought it was because of that it wasn't working. PreUpdatedEventArgs exception name suggested me that.

So, now I have a custom Logger, deactivated yours, In this logger, in log method I am just "storing" in an array the baseAuditLog events... that will be executed in CustomDoctrineEventLoggerSubscriber and onKernelResponse I proccess the savePendingLogs... This way I can solve the problem.

Thanks for your help @ronisaha

ronisaha commented 6 years ago

Hi @pribeirojtm ,

If you think your logger implementation is stable enough to handle such scenarios, you can publish that as a library. That will be example for others to build many more custom logger for this bundle without implement in the core. Once I thought, I'll split my bundle and move the default logger to separate repository too. But decided to keep a generic logger implementation and added the configuration to enable/disable that. By default if you are not using doctrine the default logger will not registered.

So for specialized logger implementation, I think it would be batter in other repo(s)

pribeirojtm commented 6 years ago

I don't think the code is perfect, but It not seems wrong for me the idea to postpone the logging for kernel-response. Since logging is not critical to the application, but is important, the save of this logs could be in a separate transaction in this case. I don't clearly understand why those problems of lifecycle events mixed, so I think that was really the problem, and this way we overcome the possible problem (not everyone will have this kind of issue) .

Also I've added a little more code to the Subscriber, in case of we are dealing with entities in commands, when there onKernelResponse does not exits, so I've added a new Subscribed Event: ConsoleEvents::TERMINATE => 'onConsoleTerminate' which does the same as onKernelResponse:

public function onConsoleTerminate() {
      $this->logger->savePendingLogs();
}

I'm sharing the code and thoughts here to still improve the idea and debate on this. It could have some flaws that I don't yet stepped.

Thanks for your library. Hope also you can create a new tag with the code in the master sooner since it has the bugfix of delete entities that fails and the log is still registered. Thanks.

ronisaha commented 6 years ago

Nice catch for console event. I'll update the DoctrineDeleteEventLogger And create a new tag.

Thanks for reporting the bugs.

Happy Coding!