Skip to content
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

Purpose behind 'json_object' => json_encode( $audit ) ? #30

Closed
angelxmoreno opened this issue Jul 22, 2014 · 9 comments
Closed

Purpose behind 'json_object' => json_encode( $audit ) ? #30

angelxmoreno opened this issue Jul 22, 2014 · 9 comments

Comments

@angelxmoreno
Copy link

My rows are too big, even for longText on MySQL. I am curious what the goal was here especially since the deltas are already tracking what changed.

@robwilkerson
Copy link
Owner

The goal was simply to provide a snapshot of the integrated object at the time of the change. Doing so means I never have to reassemble the object as it existed at a given point.

@angelxmoreno
Copy link
Author

Problem with this approach is that we are saving massive amounts of redundant data. Perhaps a better solution would be the ability to reconstruct the object based on aggregating the deltas.

Currently my records have too much data for me to afford storing them multiple times. I guess another approach would be to extend the behavior and simply not that object.

What are your thoughts? I can get started on a PR.

@robwilkerson
Copy link
Owner

Life is a trade-off. This approach was easy and didn't have any negative side effects for my needs. :-)

That said, I have no problem progressing beyond that. What I'd like to be sure we have, though, is an easy to call API to retrieve that object snapshot. The developer shouldn't have to reconstruct the object from deltas. I didn't want to do it when I started down this road and that's why I just slapped in the JSON field. I'd like to keep that capability alive.

@angelxmoreno
Copy link
Author

sure thing. My company will be making changes and will focus on keeping the existing functionality while at the same time abstract a few parts to add flexibility. We will be sending PRs for your approval as we add these changes.

@robwilkerson
Copy link
Owner

Let's keep this open and commit against this issue as you all work on the pull request. That should keep things nice and tidy.

@robwilkerson robwilkerson reopened this Jul 22, 2014
@deizel
Copy link

deizel commented Jul 24, 2014

Seems another fork has come across this issue also: jippi@de5c3d3

@robwilkerson
Copy link
Owner

I like that this version retains an option of storing JSON snapshot.

@ravage84
Copy link
Contributor

@xhs345 let's close this and concentrate on #78. This issue is mentioned there, so it's referenced.

@xhs345
Copy link
Collaborator

xhs345 commented Sep 20, 2016

Agree

@xhs345 xhs345 closed this as completed Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants