[4.x] Feature: add `isDirty` / `isClean`

[ryanmitchell]

This PR adds a trait for isDirty() / isClean() support on Entries through the use of a trait. I set it up this way to allow it to be extended to other stores if the approach is felt to be correct.

Tests are failing as the test environment doesn't seem to like using $entry->fresh() - not sure how to get around that!

You can use the functions as follows:

$entry->isDirty();
$entry->isDirty('field');
$entry->isDirty(['field1', 'field2']);

$entry->isClean();
$entry->isClean('field');
$entry->isClean(['field1', 'field2']);

Closes https://github.com/statamic/ideas/issues/7

[ryanmitchell]

No problem - I've updated that method to be called getDirtyArray() (please come up with something better), and put some of the data logic in there.

Also separated out a separate getOriginal() seeing as the logic was there for it anyway.

[ryanmitchell]

Yep I can see the syncOriginal function - see how I've updated to look for getDirtyArray if it exists (seemed like the best approach). However, I've no idea where in the entry 'lifecycle' to insert syncOriginal() to get it to populate with any yaml-stored values?

[jasonvarga]

That would need to go in the Stache where it initially makes an instance from the file. Right about here: https://github.com/statamic/cms/blob/71fac6d87fadb2c954f5fa9e1574400e6804cc07/src/Stache/Stores/CollectionEntriesStore.php#L100

[ryanmitchell]

Thank you! Thats updated now - let me know what you think.

[ryanmitchell]

Not sure why tests are failing on this (and they seem unrelated to the changes made). They are all passing for me locally.

[ryanmitchell]

@jasonvarga Struggling a bit with the tests on this one. On the two asset failures, do I just update the tests to expect the .meta files? Or do I handle it some other way?

[ryanmitchell]

I've got all tests passing locally now, but I cant get them working here in GitHub actions. If you can see why that would be really helpful.

[duncanmcclean]

I've got all tests passing locally now, but I cant get them working here in GitHub actions. If you can see why that would be really helpful.

Looks like an error on GitHub's side, it doesn't get past setting up the jobs.. probably needs to be re-run.

[ryanmitchell]

@jasonvarga Just looping back to this one. I've fixed up the merge conflicts and tests are passing. Would you have time to do another review of it?

[jonassiewertsen]

Nice. Although the toCacheableArray method is never used, I think that's a leftover relic from v2. I'm going to sneak the removal of it into 3.3, and you can make your own dedicated method in this PR.

Could toCacheableArray get marked as deprecated before removal for a fair amount of time?

That would avoid breaking changes on our end, as we are heavily reimplementing all kinds of contracts and may rely on public methods 😁

[jasonvarga]

It was already removed in 3.3. If you really need it back, I'm pretty sure you have a custom Entry class - you can put it in there.

[jonassiewertsen]

I simply tried to ask nicely, if it's possible to avoid such changes in the future and mark methods which will be removed, as deprecated before removing them.

Those changes, which can be breaking, are hard to spot as this removal would not have been in any change log.

Thanks for considering. ⭐️

[jasonvarga]

Thanks for this PR. Another banger. And sorry for the delay, it's pretty substantial.

I made a handful of changes:

• Moved HasDirtyState into Statamic\Data where there are similar data-related traits.
• Use HasDirtyState everywhere, and deprecate the existing SyncsOriginalState.
• Ensures syncOriginal() is called at the end of save() so that events etc get the appropriate dirty state, and added tests for this.
• Renamed getDirtyArray to a wordier but more explicit getCurrentDirtyStateAttributes so it doesn't get confused with getDirty which Eloquent models have and I imagine we might add down the road.
• Add tests for isClean
• Moved all the stache syncOriginal stuff to a consistent spot in one class - when it's created from the file or loaded from the cache.

Some additional notes:

• The user events test should be testing creating/created but it would mean changing unrelated tests, so I just left a comment instead.
• Since assets store "data" in a sub-array, you needed to add extra logic in isDirty to handle that. I'd prefer it to be a flat array like everywhere else but that would have made this PR grow in scope, and might've been a breaking change. We'll consider changing that for v5.

[ryanmitchell]

Thanks for all the changes. As always you’ve made it much better.

[ryanmitchell]

❤️

[edalzell]

KA BOOM

[afonic]

Thanks for this PR, it will be massively helpful.

Not sure where to ask so I'll try here! This breaks some tests for an addon of mine and I am struggling to understand why. For example this test: https://github.com/reachweb/locale-lander/blob/main/tests/Feature/LocaleLanderTest.php#L61 works fine if I remove the HasDirtyState trait from the Entry model (or downgrade to v4.50.0).

It seems that when using the trait, when checking for the translation here: https://github.com/reachweb/locale-lander/blob/main/src/Support/LocaleHelper.php#L63 it will return null. To make matters even worse, if I run the test 10 times, it will find the Entry 2 times out of 10 (or something like that).

Any ideas on what causes this and how to fix the tests?

Note that this only affects the test environment, the addon works fine in the actually Statamic install.

[duncanmcclean]

@afonic I haven't dug into it but could your issue be related to #9651? If not, maybe start a new Discussion? (to avoid everyone in this PR being notified of any comments 😄 )