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

[Bug]: Not properly working with VersionStrategy::DIFF #10

Closed
mutschler opened this issue Mar 18, 2024 · 7 comments
Closed

[Bug]: Not properly working with VersionStrategy::DIFF #10

mutschler opened this issue Mar 18, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@mutschler
Copy link

mutschler commented Mar 18, 2024

What happened?

If you use VersionStrategy::DIFF (default) only changed values are stored, so the revision view breaks when you update a filed that wasn't saved in the version before

How to reproduce the bug

  1. Use VersionStrategy::DIFF as strategy
  2. Create a new Model with title, content fields
  3. Update the title => only new title will be saved
  4. Update the content => only new content will be saved
  5. Click "Revisions" and page breaks since content is not present on the previous version

Package Version

0.0.6

PHP Version

8.2

Laravel Version

11

Which operating systems does with happen with?

Linux

Notes

This isn't directly related to this plugin i guess but to the Diff class you wrote for the upstream package...

From my understanding: In DIFF Mode you can't directly compare one version to the previous one. You'll have to either load the first version, apply every diff and compare the result to the current one OR go back till you have all keys which where changed in the current version and show them from there

@mutschler mutschler added the bug Something isn't working label Mar 18, 2024
@mutschler mutschler changed the title [Bug]: Not properly with VersionStrategy::DIFF [Bug]: Not properly working with VersionStrategy::DIFF Mar 18, 2024
@mansoorkhan96
Copy link
Owner

mansoorkhan96 commented Mar 19, 2024

This is an issue in Laravel versionable. It will be fixed soon.

overtrue/laravel-versionable#56

@mansoorkhan96
Copy link
Owner

@mutschler Have you tried to use VersionStrategy::SNAPSHOT, thats what we are going to keep in future.

@mutschler
Copy link
Author

mutschler commented Mar 22, 2024

yeah i've tried that and it works. However i didn't plan to keep full snapshots on the data but only save the changes. Full Snapshots provide too much overhead. Since i've already red upstream that there are plans for dropping DIFF strategy i've came up with something else...

However i've spent some time looking into it before and it's actually relatively easy to fix. Actually revertWithoutSaving already does most of the things needed by looping through all changes up to the current one, so you'll get the keys you need to have when comparing. I'm gonna check if is still got my solution lying around somewhere if you're interested ;)

From what i've still got in my head: load the initial version, apply all changes up to the current one -1 resulting in a full entry with all fields then get rid of all keys which are not in the current record.

@mansoorkhan96
Copy link
Owner

I will check.

@overtrue maybe you should read this.

@mansoorkhan96
Copy link
Owner

@mutschler I am open for a pr or any code block that helps.

@mutschler
Copy link
Author

mutschler commented Mar 23, 2024

i've pushed the code which seemed to work for me here overtrue/laravel-versionable#62 not really sure if the if/else part for loading the $newContents is really needed but i did it that way for a reason i can't really rememer right now.

As mentioned before it's more or less the same thing revertWithoutSaving does: Load initial data and apply each pach up to the latest previous version to that data, then remove all unneeded keys from the array

@mansoorkhan96
Copy link
Owner

@mutschler I think its fixed and working for you? I am closing for now, feel free to open if you still have issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants