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

[11.x] Add "previous" state to models #53901

Open
wants to merge 1 commit into
base: 11.x
Choose a base branch
from

Conversation

dshafik
Copy link
Contributor

@dshafik dshafik commented Dec 15, 2024

This change make it possible to retrieve the previously saved state (i.e. what original was before update/save). This gives you the ability to do similar checks as with current vs original state after save.

This adds ->getPrevious($key = null, $default = null) and ->getRawPrevious($key = null, $default = null), as well as syncPrevious() but it might be better to make the latter protected.

I also noticed a delete test in the EloquentUpdateTest so I moved it to the EloquentDeleteTest and added a correct test case in EloquentUpdateTest in its place.

@chu121su12
Copy link
Contributor

Attempted at v9 (#42504). Still a nice to have in core while being an optional trait.

@devfrey
Copy link
Contributor

devfrey commented Dec 15, 2024

This has the potential to greatly increase memory utilization for large models and should therefore be opt-in, as suggested in the previous attempt.

@shaedrich
Copy link
Contributor

Would it make sense to have a method to revert the model to the previous state?

@crynobone crynobone marked this pull request as draft December 16, 2024 05:21
@crynobone
Copy link
Member

Mark PR as a draft since tests are failing.

@donnysim
Copy link
Contributor

donnysim commented Dec 16, 2024

Previous state is really important when working spread out model actions as it becomes extremely hard to track changes. An alternative way, which I prefer and do on my base model, is to disable model automatic "original" syncing and call it manually (which in 99% cases you'll never do or need, though this may have a side effect if you call save many times on the same model) which allows to always access the original data from the moment the model was fetched. Maybe it could also be done as an opt in, which should be easier than tracking previous.

@dshafik dshafik force-pushed the add-previous-state-to-models branch 2 times, most recently from 3f2cfd7 to a078be5 Compare January 1, 2025 12:44
@dshafik dshafik force-pushed the add-previous-state-to-models branch from a078be5 to 00f4323 Compare January 1, 2025 12:45
@dshafik dshafik marked this pull request as ready for review January 1, 2025 12:48
@ollieread
Copy link
Contributor

Would it not make more sense to store the original value for attributes that changed, not all of them @dshafik? You're currently storing all the model attributes using Model::$original, when surely it's best to only store those that appear in Model::$dirty?

I also feel like this should be an optional trait as it's a niche use case, as most people that need the previous state can do what they need to do with model events, or they need more than just an array.

@dshafik
Copy link
Contributor Author

dshafik commented Jan 2, 2025

Would it not make more sense to store the original value for attributes that changed, not all of them @dshafik? You're currently storing all the model attributes using Model::$original, when surely it's best to only store those that appear in Model::$dirty?

I considered this but I decided that having to reconstruct state through current -> original -> previous to get the full previous state is less than great. I also decided that just copying the array (which only happens if you make an update, it's copy on write) was cheaper than constructing a new array.

I also feel like this should be an optional trait as it's a niche use case, as most people that need the previous state can do what they need to do with model events, or they need more than just an array.

I'm not opposed to this, if there's an appetite for this at all I'm happy to make the change.

@stancl
Copy link
Contributor

stancl commented Jan 6, 2025

The type of copy that's cheaper probably depends on the expected use case. If it's expected that the model would just be returned to the previous state, it's probably better to just make use of the CoW here yeah. Otherwise it'd be more expensive since you'd be storing the original (saved) state, the full previous state, and the new dirty state.

Though I'd say storing the full state also only makes sense if this feature is somehow opt-in. My guess about expected use is:

  • No use of previous: >95%
  • Restoring previous in full: majority of the remainder
  • Restoring previous and making changes: minority of the remainder

In which case optimizing for the cheap copy on restore pessimizes the common case I think.

So moving the current implementation to a trait as suggested would work best I'd say.

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.

8 participants