overtrue / laravel-versionable

⏱️Make Laravel model versionable
MIT License
530 stars 47 forks source link

Commit introduced breaking changes #77

Open tasinttttttt opened 4 months ago

tasinttttttt commented 4 months ago

Versions >4.4 have a merged commit which introduced a breaking change (with tests not passing with the breaking change).

An example is found in the Feature test post_can_revert_to_target_version

In the tests, there's a json field called extends which is cast as an array. With the change introduced here, the cast is not taken into account:

Failed asserting that '{"foo":"bar"}' is identical to Array &0 [
    'foo' => 'bar',
].

Makes me think of the merge process, I don't think tests are run before merging right?

Originally posted by @tasinttttttt in https://github.com/overtrue/laravel-versionable/issues/50#issuecomment-2182652741

overtrue commented 4 months ago

@tasinttttttt Yes, but I think it's wrong before:

Before

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

dd($post->versions[1]->contents['extends']);

# output
array:1 [
  "foo" => "bar"
]

when we revert the version, it will cause the error:

dump($post->versions[1]->revert(), $post);
// TypeError: json_decode(): Argument #1 ($json) must be of type string, array given

After

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

dd($post->versions[1]->contents['extends']);

# output
"{"foo":"bar"}"
dump($post->versions[1]->revert(), $post->extends);

# output 
array:1 [
  "foo" => "bar"
]

So, I think it would be ok if you have any questions please let me know, and I will try my best to fix it.

tasinttttttt commented 4 months ago

@tasinttttttt Yes, but I think it's wrong before:

Before

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

dd($post->versions[1]->contents['extends']);

# output
array:1 [
  "foo" => "bar"
]

when we revert the version, it will cause the error:

dump($post->versions[1]->revert(), $post);
// TypeError: json_decode(): Argument #1 ($json) must be of type string, array given

By "Before", do you mean v4.4.0 or versions before 4.4.0 ? (Either way I wasn't able to reproduce the json_decode issue.)

Also, don't you think the mentioned failing test in 4.x should at least be updated to show the expected behavior?