VersionStrategy::SNAPSHOT saves all attributes instead of all versionable attributes

[mansoorkhan96]

The VersionStrategy::SNAPSHOT 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?

// Versionable.php, line: 215

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?

[overtrue]

I am working on this.

[mansoorkhan96]

Right now, changing the logic for VersionStrategy::SNAPSHOT might introduce a breaking change.

[mansoorkhan96]

I am working on this.

Great.

Thanks!

[overtrue]

What do you think the following definitions:

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.

[overtrue]

so maybe we should add a strategy named FULL_SNAPSHOT ? or any other name?

[mansoorkhan96]

What do you think the following definitions:

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.

Thats the ideal definition.

But.. Would not it introduce a breaking change? Users might be using SNAPSHOT to save all the Model attributes and now when they upgrade they would only see the $versionable attributes.

[mansoorkhan96]

so maybe we should add a strategy named FULL_SNAPSHOT ? or any other name?

FULL_SNAPSHOT is nice

[overtrue]

What do you think the following definitions:

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.

Thats the ideal definition.

But.. Would not it introduce a breaking change? Users might be using SNAPSHOT to save all the Model attributes and now when they upgrade they would only see the $versionable attributes.

Yes, we can create a major version.

[mansoorkhan96]

What do you think the following definitions:

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.

Thats the ideal definition. But.. Would not it introduce a breaking change? Users might be using SNAPSHOT to save all the Model attributes and now when they upgrade they would only see the $versionable attributes.

Yes, we can create a major version.

Great, we are good then.

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.
• FULL_SNAPSHOT or FULL or COMPLETE or ALL: All of the model attributes.

Now that you are releasing a major version. Maybe refactor VersionStrategy class to an enum?

[overtrue]

What do you think the following definitions:

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.

Thats the ideal definition. But.. Would not it introduce a breaking change? Users might be using SNAPSHOT to save all the Model attributes and now when they upgrade they would only see the $versionable attributes.

Yes, we can create a major version.

Great, we are good then.

• DIFF: Changed attributes in the $versionable
• SNAPSHOT: All the attributes in the $versionable, not all of the model attributes.
• FULL_SNAPSHOT or FULL or COMPLETE or ALL: All of the model attributes.

Now that you are releasing a major version. Maybe refactor VersionStrategy class to an enum?

Great idea!

[overtrue]

Here's a query, I don't know what you think, I think that the DIFF model probably shouldn't exist, for example:

// model Post protected $versionable = ['title', 'content', 'extends'];

$post = Post::create(['title' => 'version1', 'content' => 'version1 content']); $post->update(['title' => 'version2', 'extends' => ['foo' => 'bar' ]]); $post->versions.

$post->versions: 
//[id => 1, 'title' => 'version1', 'content' => 'version1 content']
//[id => 2, 'title' => 'version2', extends => ['foo' => 'bar'] ] ```

If we roll back to version 1, we will get unexpected results:

```php
$post->firstVersion->revert(); dd($post->extends); // ['foo' => 'bar'] 

Because the implementation logic is taking the version 1 contents (title, content) and overriding the current field (title,content), the extends are not overridden, leading to the wrong result, and since extends is in the versionable, it looks like a bug.

[mansoorkhan96]

Here's a query, I don't know what you think, I think that the DIFF model probably shouldn't exist, for example:

// model Post protected $versionable = ['title', 'content', 'extends'];

$post = Post::create(['title' => 'version1', 'content' => 'version1 content']); $post->update(['title' => 'version2', 'extends' => ['foo' => 'bar' ]]); $post->versions.

$post->versions: 
//[id => 1, 'title' => 'version1', 'content' => 'version1 content']
//[id => 2, 'title' => 'version2', extends => ['foo' => 'bar'] ] ```

If we roll back to version 1, we will get unexpected results:

```php
$post->firstVersion->revert(); dd($post->extends); // ['foo' => 'bar'] 

Because the implementation logic is taking the version 1 contents (title, content) and overriding the current field (title,content), the extends are not overridden, leading to the wrong result, and since extends is in the versionable, it looks like a bug.

Let me try that.

[mansoorkhan96]

@overtrue I think you are right. Reverting with DIFF is complex, because you dont have the attributes and you dont know what was the state for the unchanged attributes in that version.

[overtrue]

yes, so i think we will remove this strategy. 🤣

[overtrue]

I am working on branch 5.x

[overtrue]

Let's do some work first and keep changing it later, feel free to discuss any comments, thanks!

[mansoorkhan96]

Let's do some work first and keep changing it later, feel free to discuss any comments, thanks!

Great. Let me know if you want to assign anything to me.

[overtrue]

For now I'm hoping that when I'm done with the changes you can help verify that all of your needs are met and that your package is working properly, thanks!

[mansoorkhan96]

For now I'm hoping that when I'm done with the changes you can help verify that all of your needs are met and that your package is working properly, thanks!

No problems, I will do that.

[overtrue]

Hi, I've found a problem, if you are in diff mode, the revert version should be stacked with all the versions from the initial version, while snapshot will just use that version.

[overtrue]

revert to version 3 DIFF: array_merge(version 1, version 2, version 3)

so we must keep the first version with full mode:

if ($post->isFirstVersion()) {
   $version->contents = $post->refresh()->getAttributes();
} else {
   $version->contents = by strategy.
}

[overtrue]

These three schemas have the following characteristics:

1. DIFF 1.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 1.2. the version history keeps only every change; 1.3. rollbacks are stacked chronologically from the initialization version to the target version; 1.4. deleting a version will affect rollback behavior.

2. SNAPSHOT 2.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 2.2. the version history keeps only the fields that were changed in each $versionables; 2.3. the initial version + the target version is sufficient for rollback; 2.4. deleting the version will not affect the rollback behavior.

3. FULL/ALL 3.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 3.2. the version history keeps all fields; 3.3. the target version can be used directly for rollback; 3.4. deleting the version will not affect the rollback behavior.

[overtrue]

In this way, SNAPSHOT mode can actually be a new version of FULL mode, keeping the original DIFF and SNAPSHOT, and just changing the implementation to the FULL effect. The catch here is that the first version must be the full version.

[mansoorkhan96]

These three schemas have the following characteristics:

1. DIFF 1.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 1.2. the version history keeps only every change; 1.3. rollbacks are stacked chronologically from the initialization version to the target version; 1.4. deleting a version will affect rollback behavior.
2. SNAPSHOT 2.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 2.2. the version history keeps only the fields that were changed in each $versionables; 2.3. the initial version + the target version is sufficient for rollback; 2.4. deleting the version will not affect the rollback behavior.
3. FULL/ALL 3.1. the initialized version is for all fields and is required to get the default values of the fields from the database; 3.2. the version history keeps all fields; 3.3. the target version can be used directly for rollback; 3.4. deleting the version will not affect the rollback behavior.

I dont understand the problem you mentioned above.

But 2.2 could be following? 2.2. the version history keeps all the $versionables fields; if any of the $versionables fields has changed.

yes.

[overtrue]

I mean, my original implementation of "revert" was wrong, my original implementation was:

$post->revertTo($targetVersion);

1. Fetch $targetVersion contents
2. Merge $post->all() and $targetVersion->contents

the step 2 is wrong, it is latest attributes + $targetVersion->contents.

it should be first full attributes + ($versions)... + $targetVersion->contents.

do you understand?

[mansoorkhan96]

I mean, my original implementation of "revert" was wrong, my original implementation was:

$post->revertTo($targetVersion);

1. Fetch $targetVersion contents
2. Merge $post->all() and $targetVersion->contents

the step 2 is wrong, it is latest attributes + $targetVersion->contents.

it should be first full attributes + ($versions)... + $targetVersion->contents.

do you understand?

it should be first full attributes + ($versions)... + $targetVersion->contents.

Well this might be necessary for the DIFF mode, because we only have certain fields in some versions.

But for snapshot, we will have all the fields we care about and can revert easily to any target version we need.

For my usecase, i might eventually write my own revert, as i would allow the user to revert only selected fields.

[overtrue]

So, I think the new version will have two strategy like this:

1. DIFF
   • init with all attributes(refreshed from database)
   • save changed attributes(subset of $versionable)
   • rollback: init version + all of versions to $targetVersion + attributes not in the $versionable of latest version.

example:

$post = Post::create(['title' => 'Init title', 'type' => 'Blog', 'status' => 'published'])
$post->update(['title' => 'version 2 title']);
$post->update(['status' => 'draft']); // no version
$post->update(['type' => 'Vlog']);

// versions:
// ['id' => 1, 'title' => 'Init title', 'type' => 'Blog', 'status' => 'published']
// ['id' => 2, 'title' => 'version 2 title']
// ['id' => 3, 'type' => 'vlog']

$post->revert(2);
// [
//      'title' => 'version 2 title', 
//      'type' => 'Blog', 
//      'status' => 'draft' // latest value, because this field is not in $versionable
// ]

3. SNAPSHOT
   • init with all attributes(refreshed from database)
   • save all attributes
   • rollback: $targetVersion

[mansoorkhan96]

So, I think the new version will have two strategy like this:

1. DIFF

• init with all attributes(refreshed from database)
• save changed attributes(subset of $versionable)
• rollback: init version + all of versions to $targetVersion + attributes not in the $versionable of latest version.

example:

$post = Post::create(['title' => 'Init title', 'type' => 'Blog', 'status' => 'published'])
$post->update(['title' => 'version 2 title']);
$post->update(['status' => 'draft']); // no version
$post->update(['type' => 'Vlog']);

// versions:
// ['id' => 1, 'title' => 'Init title', 'type' => 'Blog', 'status' => 'published']
// ['id' => 2, 'title' => 'version 2 title']
// ['id' => 3, 'type' => 'vlog']

$post->revert(2);
// [
//      'title' => 'version 2 title', 
//      'type' => 'Blog', 
//      'status' => 'draft' // latest value, because this field is not in $versionable
// ]

3. SNAPSHOT

• init with all attributes(refreshed from database)
• save all attributes
• rollback: $targetVersion

I trust your instincts. Do it if its the best way to get around it ❤️

init with all attributes(refreshed from database)

But maybe lets make the saving all attributes functionality optional. Because then the diff could get messy.

[overtrue]

@mansoorkhan96 Yes, the default strategy is DIFF, so do you think it's better to snapshot $versionable rather than snapshot all attributes?

[mansoorkhan96]

@mansoorkhan96 Yes, the default strategy is DIFF, so do you think it's better to snapshot $versionable rather than snapshot all attributes?

I think its better to keep $versionable attributes only, when user explicitly opts for that.

[overtrue]

like this?

// SNAPSHOT: $versionable = ['title', 'type']
$post = Post::create(['title' => 'Init title', 'type' => 'Blog', 'status' => 'published'])
$post->update(['title' => 'version 2 title']);
$post->update(['status' => 'draft']); // no version
$post->update(['type' => 'Vlog']);

// versions:
// ['id' => 1, 'title' => 'Init title', 'type' => 'Blog', 'status' => 'published']
// ['id' => 2, 'title' => 'version 2 title', 'type' => 'Blog',]
// ['id' => 3, 'title' => 'version 2 title', 'type' => 'vlog']

$post->revert(2);
// [
//      'title' => 'version 2 title', 
//      'type' => 'Blog', 
//      'status' => 'draft' // latest value, because this field is not in $versionable
// ]

[overtrue]

OK, if the $versionable is empty, snapshot all?

[mansoorkhan96]

like this?

// SNAPSHOT: $versionable = ['title', 'type']
$post = Post::create(['title' => 'Init title', 'type' => 'Blog', 'status' => 'published'])
$post->update(['title' => 'version 2 title']);
$post->update(['status' => 'draft']); // no version
$post->update(['type' => 'Vlog']);

// versions:
// ['id' => 1, 'title' => 'Init title', 'type' => 'Blog', 'status' => 'published']
// ['id' => 2, 'title' => 'version 2 title', 'type' => 'Blog',]
// ['id' => 3, 'title' => 'version 2 title', 'type' => 'vlog']

$post->revert(2);
// [
//      'title' => 'version 2 title', 
//      'type' => 'Blog', 
//      'status' => 'draft' // latest value, because this field is not in $versionable
// ]

Yep, this looks good.

[mansoorkhan96]

OK, if the $versionable is empty, snapshot all?

yes, that should be the default.

[overtrue]

OK, if the $versionable is empty, snapshot all?

yes, that should be the default.

Nice! thank you!

[mansoorkhan96]

@overtrue FYI, I was inspired by the Wordpress Revisions system. I borrowed some styling from there too.

Maybe you should also try it to see how they handle it. With 0 plugins, when you have updated a post/page 2-3 times. You should see the Revisions button on right side bar and then it opens up the diff page. And it only shows you the diff for the title and content. Its more like what we are doing with snapshot.

https://wordpress.com/support/page-post-revisions/

[overtrue]

@overtrue FYI, I was inspired by the Wordpress Revisions system. I borrowed some styling from there too.

Maybe you should also try it to see how they handle it. With 0 plugins, when you have updated a post/page 2-3 times. You should see the Revisions button on right side bar and then it opens up the diff page. And it only shows you the diff for the title and content. Its more like what we are doing with snapshot.

https://wordpress.com/support/page-post-revisions/

OK, thanks.

[overtrue]

https://github.com/WordPress/WordPress/blob/master/wp-includes/revision.php#L130

[mansoorkhan96]

Also, can we pass an a third option to diff functions like toSideBySideHtml to also include attributes that were not changed.

Right now the diff removes the title if it was not changed.

Thanks.

[overtrue]

Also, can we pass an a third option to diff functions like toSideBySideHtml to also include attributes that were not changed.

Right now the diff removes the title if it was not changed.

Thanks.

Good idea, I will try it after these works.

[mansoorkhan96]

Also, can we pass an a third option to diff functions like toSideBySideHtml to also include attributes that were not changed. Right now the diff removes the title if it was not changed. Thanks.

Good idea, I will try it after these works.

You are the man. Thank you so much.

[overtrue]

Do you think the initial version should include all attributes?

[overtrue]

Do you think the initial version should include all attributes?

I prefer to Yes.

[mansoorkhan96]

Do you think the initial version should include all attributes?

I prefer to Yes.

Well if needed. But then i have to strip them out and only show the $versionable.

[overtrue]

Do you think the initial version should include all attributes?

I prefer to Yes.

Well if needed. But then i have to strip them out and only show the $versionable.

OK.

[mansoorkhan96]

I have hacked your Diff => render method by manually creating a simple diff for identical strings 😈

public function render(string $renderer = null, array $differOptions = [], array $renderOptions = []): array
{
    if (empty($differOptions)) {
        $differOptions = $this->differOptions;
    }

    if (empty($renderOptions)) {
        $renderOptions = $this->renderOptions;
    }

    $fromContents = $this->fromVersion->contents;
    $toContents = $this->toVersion->contents;

    $diff = [];
    $createDiff = function ($key, $old, $new) use (&$diff, $renderer, $differOptions, $renderOptions) {
        if ($renderer) {
            $old = is_string($old) ? $old : json_encode($old);
            $new = is_string($new) ? $new : json_encode($new);

            if ($old === $new) {
                // If there are no changes, write a hardcoded custom diff.
                $diff[$key] = '<table class="diff-wrapper diff diff-html diff-side-by-side"><tbody class="change change-eq"><tr><td class="old">'.$old.'</td><td class="new">'.$new.'</td></tr></tbody></table>';
            } else {
                $diff[$key] = str_replace('\n No newline at end of file', '', DiffHelper::calculate($old, $new, $renderer, $differOptions, $renderOptions));
            }
        } else {
            $diff[$key] = compact('old', 'new');
        }
    };

    // we are not using ArrayDiffMultidimensional, as we dont need to strip out the identical string. so a simple foreach on our attributes
    foreach ($fromContents as $key => $value) {
        $createDiff($key, Arr::get($toContents, $key), Arr::get($fromContents, $key));
    }

    foreach (array_diff_key($fromContents, $toContents) as $key => $value) {
        $createDiff($key, null, $value);
    }

    return $diff;
}

[mansoorkhan96]

I have opened an issue on PHP Diff package: https://github.com/jfcherng/php-diff/issues/78

[overtrue]

I have hacked your Diff => render method by manually creating a simple diff for identical strings 😈

public function render(string $renderer = null, array $differOptions = [], array $renderOptions = []): array
{
    if (empty($differOptions)) {
        $differOptions = $this->differOptions;
    }

    if (empty($renderOptions)) {
        $renderOptions = $this->renderOptions;
    }

    $fromContents = $this->fromVersion->contents;
    $toContents = $this->toVersion->contents;

    $diff = [];
    $createDiff = function ($key, $old, $new) use (&$diff, $renderer, $differOptions, $renderOptions) {
        if ($renderer) {
            $old = is_string($old) ? $old : json_encode($old);
            $new = is_string($new) ? $new : json_encode($new);

            if ($old === $new) {
                // If there are no changes, write a hardcoded custom diff.
                $diff[$key] = '<table class="diff-wrapper diff diff-html diff-side-by-side"><tbody class="change change-eq"><tr><td class="old">'.$old.'</td><td class="new">'.$new.'</td></tr></tbody></table>';
            } else {
                $diff[$key] = str_replace('\n No newline at end of file', '', DiffHelper::calculate($old, $new, $renderer, $differOptions, $renderOptions));
            }
        } else {
            $diff[$key] = compact('old', 'new');
        }
    };

    // we are not using ArrayDiffMultidimensional, as we dont need to strip out the identical string. so a simple foreach on our attributes
    foreach ($fromContents as $key => $value) {
        $createDiff($key, Arr::get($toContents, $key), Arr::get($fromContents, $key));
    }

    foreach (array_diff_key($fromContents, $toContents) as $key => $value) {
        $createDiff($key, null, $value);
    }

    return $diff;
}

5.x branch seems ready, do you need add this to 5.x ?

[mansoorkhan96]

I have opened an issue on PHP Diff package: jfcherng/php-diff#78

This request was completed by the Diff author 🚀

@overtrue This how your render would look now:

public function render(string $renderer = null, array $differOptions = [], array $renderOptions = []): array
{
    if (empty($differOptions)) {
        $differOptions = $this->differOptions;
    }

    if (empty($renderOptions)) {
        $renderOptions = $this->renderOptions;
    }

    $fromContents = $this->fromVersion->contents;
    $toContents = $this->toVersion->contents;

    $diff = [];
    $createDiff = function ($key, $old, $new) use (&$diff, $renderer, $differOptions, $renderOptions) {
        if ($renderer) {
            $old = is_string($old) ? $old : json_encode($old);
            $new = is_string($new) ? $new : json_encode($new);
            $diff[$key] = str_replace('\n No newline at end of file', '', DiffHelper::calculate($old, $new, $renderer, $differOptions, $renderOptions));
        } else {
            $diff[$key] = compact('old', 'new');
        }
    };

    // we are not using ArrayDiffMultidimensional, as we dont need to strip out the identical string. so a simple foreach on our attributes
    foreach ($fromContents as $key => $value) {
        $createDiff($key, Arr::get($toContents, $key), Arr::get($fromContents, $key));
    }

    foreach (array_diff_key($fromContents, $toContents) as $key => $value) {
        $createDiff($key, null, $value);
    }

    return $diff;
}

[overtrue]

Great!