Allow viewing on old record status in onBeforeUpdate

[wilsonge]

I'm not 100% if it's better to even clone the model and inject the whole model - but as $dataBeforeBind already exists - it would appear to make sense to use that.

I've had several cases when updating a record I need to look at the previous value - but I either have to add a check in onBeforeSave for if the record exists OR I have to add have the data available before bind in onBeforeUpdate - so i'm going with the latter approach here.

In all honesty I'd kinda expect the onBeforeUpdate and onBeforeCreate events to be thrown at the same time as onBeforeSave (i.e. before the bind) but as for b/c we can't do that I guess this is the next best alternative?

[nikosdion]

Why did you remove the array_merge($this->recordData) line? This is required to do a copy-by-value of the array. Otherwise some versions of PHP do a copy by reference which is not what we want.

Furthermore, $dataBeforeBind is currently populated only when you have relation-important fields. In any other case that's a blank array.

Storing a copy of the old data before binding anything is memory intensive. I don't think it should be enabled by default.

Finally, you are changing the signature of onBeforeUpdate causing PHP notices to crop up.

Instead of doing any of that I'd simply override save() to store a copy of the current data in a protected object property (e.g. dataBeforeSave) and create an onBeforeUpdate that takes this data into account. You can turn that code into a trait that you can reuse in all your models that need this kind of functionality without taxing the memory usage of the typical FOF 3 use case which doesn't require such a feature (I only use this kind of feature in one model of Akeeba Subs).

[wilsonge]

Removing array_merge is just my bad - I thought it was bloat code - I stand corrected :)

Yah it didn't occur to me about it being relations only. That was me trying to use what we had rather than doing what we probably should have been doing (cloning the model).

I didn't think adding an extra param throws PHP Notices?

I mean I also considered adding an $isNew param to the onBeforeSave but inside the model isn't determined until after the binding of data has been done - so you have to do SQL Queries knowing the details of the model.

I can do the override of save like you suggest I guess - but it just seemed like overkill (every FOF update means I'll need to check the trait to stay "in sync". What is the intended use case of onBeforeUpdate? because having the binded data with the array of what changed - without knowing what the data was previously makes it seems super limited in terms of what you can do with it?

[nikosdion]

I didn't think adding an extra param throws PHP Notices?

If my onBeforeUpdate implementation expects one parameter and you supply two PHP throws a Notice.

I can do the override of save like you suggest I guess - but it just seemed like overkill (every FOF update means I'll need to check the trait to stay "in sync".

No you wouldn't. The save() method would populate the protected variable and then call parent::save. Think simple!