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

Optionally create initial version of model #53

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

mansoorkhan96
Copy link
Contributor

This pull request adds support for optionally creating a version for the initial/existing state of the model.

It fixes the issue: #51

@mansoorkhan96
Copy link
Contributor Author

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.

The reason for adding this option was this issue:
If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.

Here is a screenshot from a plugin i am currently working on.
image

@overtrue
Copy link
Owner

overtrue commented Mar 4, 2024

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.

The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.

Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

@mansoorkhan96
Copy link
Contributor Author

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.
The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.
Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

Ah sounds like that would do it.
Let me try.

@mansoorkhan96
Copy link
Contributor Author

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.
The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.
Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

Yes, changing Version Strategy already does that.
We can discard the last commit.

Thanks for time :)

@overtrue
Copy link
Owner

overtrue commented Mar 4, 2024

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.
The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.
Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

Yes, changing Version Strategy already does that. We can discard the last commit.

Thanks for time :)

Good!

@mansoorkhan96
Copy link
Contributor Author

Should i push a commit by removing last commit changes?

@overtrue
Copy link
Owner

overtrue commented Mar 4, 2024

Should i push a commit by removing last commit changes?

Yes, please. thanks.

@mansoorkhan96
Copy link
Contributor Author

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.
The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.
Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

Just realised the Snapshot version strategy saves all the model attributes instead of saving on the ones specified using: protected $versionable = ['title', 'content'];

@mansoorkhan96
Copy link
Contributor Author

@overtrue I have added another commit. It adds a new config record_only_changed_attributes, true by default, which allows to optionally disable it and record all the attributes.
The reason for adding this option was this issue: If you dont change certain attributes, the diff starts becoming inconsistent by adding null to the diff.
Here is a screenshot from a plugin i am currently working on. image

https://github.com/overtrue/laravel-versionable?tab=readme-ov-file#custom-version-store-strategy Does this fulfill your needs?

Just realised the Snapshot version strategy saves all the model attributes instead of saving on the ones specified using: protected $versionable = ['title', 'content'];

I think this is because of $this->getAttributes() in following code?

if ($this->getVersionStrategy() === VersionStrategy::SNAPSHOT && (! empty($changes) || ! empty($attributes))) {
    $changedKeys = array_keys($this->getAttributes());
}

It should be $changedKeys = array_keys($this->getVersionable());

Or we can even add a completely new strategy to do that?

@overtrue overtrue merged commit 3218fcd into overtrue:4.x Mar 4, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants