xiidea / EasyAuditBundle

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

Log should not be registered when action fails #21

Closed pribeirojtm closed 6 years ago

pribeirojtm commented 6 years ago

Hi

I'm having incoherences in AudiLog, in cases when an action Update/Create/Delete to an entity fails for any reason: log is still registered while action actual failed.

Imagine you have 2 Entities: Product and Category . And assuming a Product can't "live" without a Category.

Product.php:

namespace AppBundle\Entity;
use Doctrine\ORM\Mapping as ORM;

/**
* @ORM\Table(name="product")
* @ORM\Entity
*/
class Product {

/**
* @ORM\Id
* @ORM\GeneratedValue(strategy="IDENTITY")
*/
private $id;

/**
*  @ORM\Column(type="string")
*/
private $name;

/**
Or OneToOne
* @var Category
* @ORM\ManyToOne(targetEntity="Category") 
* @ORM\JoinColumns({
*     @ORM\JoinColumn(name="id_category", referencedColumnName="id")
* })
*/
private $category;
}

Castegory.php:

namespace AppBundle\Entity;
use Doctrine\ORM\Mapping as ORM;

/**
* @ORM\Table(name="category")
* @ORM\Entity
*/
class Category {

/**
* @ORM\Id
* @ORM\GeneratedValue(strategy="IDENTITY")
*/
private $id;

/**
*  @ORM\Column(type="string")
*/
private $name;
}

Records in each table:

Product:

| id | name | id_category |
| 1  | a    | 1           |

Category:

| id | name | 
| 1  | A    |

Example of deleting Category with id 1:

Because category has its PK present as FK in Product entity and it has the default "onDelete Restrict", it fails to delete the parent entity record, in this case the Category. If I wanted really to delete the category, I had to first delete the reference in the Product, but I don't want that in this case. In these cases of failure, the log is still registered.

Category has been deleted with id = "1".

Do you recommend a way to tackle this issue?

pribeirojtm commented 6 years ago

Hi @ronisaha,

Have you tested this case?

ronisaha commented 6 years ago

Sorry, could not make time yet. Could you make a sample code repository with the code to reproduce the use case, so I can just pull and run.

Thanks

ronisaha commented 6 years ago

Ok, I've looked into the issue. The reason for this issue exists for the code here. I have logging the delete action on preRemove instead of postRemove.

The reason behind this was for the implementation in doctrine. As doctrine return null for id of an deleted entity

In my use case I had to access the id field to refer to the entity. So i had no choice but use the preRemove event instead of postRemove.

Now about solutions, for the time being until I got any proper way to address this issue you can do the follow:

You can disable the tracking of delete event for that entity by following the documentation then on successful delete dispatch any other generic event to track that event.

Thanks

pribeirojtm commented 6 years ago

Thanks @ronisaha.

Do you think this also applies to failures in Updating and Creation of Entities? Or the problem is just in this case of failing entity removals? I've reported initially that the problem would be the same for creation and updating entities (without firstly reproduce the problem. assuming it was the same as the deleted one), but reading your answer it seems that I does not apply to those cases.

Thanks for the workaround solution. I'll apply it till it is found a solution for this. At least I've detected and reported a bug. If I have time and sufficient knowledge about doctrine and your library, I'll try to look deeply and try to solve the problem.

ronisaha commented 6 years ago

The problem is only in removal.

Thanks for reporting this Bug.

ronisaha commented 6 years ago

Hi, @pribeirojtm Could you please check the master-branch if the problem is being resolved or not.

Thanks for your co-operation.

pribeirojtm commented 6 years ago

Hi @ronisaha ,

It is now working as expected. Will you create a new tag?

ronisaha commented 6 years ago

Yes I'll create a new tag after I'm done with updating the 1.3.x branch with the patch

pribeirojtm commented 6 years ago

Hi @ronisaha ,

Is this already in a new tag? or still in master?

ronisaha commented 6 years ago

It's still in master, waiting some time to get tested response from you guys, then will create a tag.