search

xiidea / EasyAuditBundle

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

Initial refactoring to work with doctrine ODM #34

Closed wh1pp3rz closed 5 years ago

ronisaha commented 5 years ago

Thanks @wh1pp3rz I'll review the code asap, and try to merge it by this week.

ronisaha commented 5 years ago

It's locks like, you are supporting only MongoDB. But there is other kind of documents also, like PHPCR, CouchDB. As my current code is tightly coupled with ORM your implementation is tightly coupled with MongoDb.

I think I'll go for new version major release with ODM support. It will be BC breaking in my view.

For instance I'll move to Doctrine\Common\Persistence\Event\LifecycleEventArgs instead of Doctrine\ORM\Event\LifecycleEventArgs then it would bbe compatible for both ORM and ODM.

That will reduce many code duplication.

What do you think?

wh1pp3rz commented 5 years ago

Yes I totally agree. It was thinking about making a separation ODM version, hence my initial email requesting your permission to release a separate bundle based off yours. I could also try to modify the PR to make it be compatible with both ORM and ODM. However that would require a major refactoring to decouple it from ORM and also modify the configurations a bit, maybe something of the sort:

xiidea_easy_audit: persistence: driver: orm | odm entity_class: MyApp\Entitiy\AuditLog (if driver is orm) document_class: MyApp\Document\AuditLog (if driver is odm) [Add some logic in Config to make orm and odm mutually exclusive] other exisitng configs ....

However such a change will also break BC and I'm not sure if there's a great demand for an ODM version. Let me know what you think, I can take either approach.

Regards,

On Sun, Dec 2, 2018 at 8:39 PM Roni Saha notifications@github.com wrote:

It's locks like, you are supporting only MongoDB. But there is other kind of documents also, like PHPCR, CouchDB. As my current code is tightly coupled with ORM your implementation is tightly coupled with MongoDb.

I think I'll go for new version major release with ODM support. It will be BC breaking in my view.

For instance I'll move to Doctrine\Common\Persistence\Event\LifecycleEventArgs instead of Doctrine\ORM\Event\LifecycleEventArgs then it would bbe compatible for both ORM and ODM.

That will reduce many code duplication.

What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/xiidea/EasyAuditBundle/pull/34#issuecomment-443563958, or mute the thread https://github.com/notifications/unsubscribe-auth/ALGbRYAWZS3bydcKTfExsZgth8KsWb1xks5u1IDngaJpZM4Y8Nu0 .

-- Not everything that counts can be counted, and not everything that can be counted counts - Albert Einstein

ronisaha commented 5 years ago

I was thinking to rename the entity_class option to audit_log_class there is no need for two different option for it if these values are mutually exclusive.

wh1pp3rz commented 5 years ago

Makes perfect sense, I'll refactor the PR accordingly.

On Mon, Dec 3, 2018, 9:07 PM Roni Saha <notifications@github.com wrote:

I was thinking to rename the entity_class option to audit_log_class there is no need for two different option for it if these values are mutually exclusive.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/xiidea/EasyAuditBundle/pull/34#issuecomment-443942692, or mute the thread https://github.com/notifications/unsubscribe-auth/ALGbRXNLkPaz_QQqUQuYZeG6ZBKfCQBZks5u1djdgaJpZM4Y8Nu0 .

ronisaha commented 5 years ago

Could you close this PR and start new one based on feature/odm branch. and we can use #35 to track the progress.

Thanks for your contributions.