-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial refactoring to work with doctrine ODM #34
Conversation
Thanks @wh1pp3rz I'll review the code asap, and try to merge it by this week. |
It's locks like, you are supporting only MongoDB. But there is other kind of documents also, like PHPCR, CouchDB. 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 That will reduce many code duplication. What do you think? |
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 ***@***.***> 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
<#34 (comment)>,
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*
|
I was thinking to rename the |
Makes perfect sense, I'll refactor the PR accordingly.
…On Mon, Dec 3, 2018, 9:07 PM Roni Saha ***@***.*** 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
<#34 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALGbRXNLkPaz_QQqUQuYZeG6ZBKfCQBZks5u1djdgaJpZM4Y8Nu0>
.
|
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. |
No description provided.