j4mie / paris

A lightweight Active Record implementation for PHP5, built on top of Idiorm.
http://j4mie.github.com/idiormandparis/
996 stars 131 forks source link

retrieve unupdated attribute #64

Closed lalop closed 11 years ago

lalop commented 11 years ago

I have a model with an image, my model as a attribute image with the name of the image. While I save it I want to make some manipulation on this image if the attribute is dirty and I would like to remove the old image file.

Today to retrieve the old file name I have to make a new request, I think the orm can keep the new and the old value of the field while it's dirty.

Especially the object have an property _dirty_fields which is an array off each field qith the new value duplicated. So why don't put the original value here instead of the new one ?

treffynnon commented 11 years ago

I may be missing something, but can't you just clone the instance?

$my_model = Model::factory('MyModel')->find_one();
$my_model->my_property = 'something';

$my_model2 = clone $my_model;

$my_model->another_property = 'value';

if(some_condition) {
    $my_model->another_property = $my_model2->another_property;
}
$my_model->save();

Idiorm doesn't output the original value as this would be confusing. If you call $object->var = 1; then you naturally expect to get var_dump($object->var); // 1.

lalop commented 11 years ago

What id $object->var ?

Yes it's effectively what I'm expecting but if I do var_dump($object) I can see that the object contains an array with updated property and their updated value : $object->orm->_dirty_fields What is the interest of this property ? This could has the original value and _data the updated value, a simple key matching will give the dirty fields.

treffynnon commented 11 years ago

To be honest I do not have a clue why it has been done that way. Perhaps it was a pull request that was merged or perhaps that is how @j4mie wrote it. It has been a little while now since it was written so I am not even sure that Jamie would know the reasons why.

Anyway I am going to close this ticket as I think we have reached an answer on the technical problem and unfortunately I do not have any more information on the reasons why.

Feel free to re-open if you have more you want to ask.

lalop commented 11 years ago

To start it was more a functional improvement than an technical answer on how add this improvement.

What do you think of the functionality ?

treffynnon commented 11 years ago

Without giving it too much thought it would appear to be backwards incompatible to me. Have you got thoughts on that?

lalop commented 11 years ago

That wiil affect idiorm more than paris since the dirty property is in it but the idea is to build request with the value registred in the _data property rather than them in _dirty. Keeping dirty property to mark an attribute has updated and not saved but registring in it the originla value not the new one ( this one is actually already registred in _data ). One time that is made, all would run normally and you can add a method like orinal_value($key) that will return the value found in database if the model isn't saved. You just have to report that to paris in a second time.

I can make a pull request to idiorm if you think that can be interesting.

treffynnon commented 11 years ago

I really don't see the need or what this gives over using PHP's clone construct. I would suggest that this a rare use case and that there are probably better ways of working this without resorting to changing a long standing part of the library.

lalop commented 11 years ago

You can see a use case in my first comment for an image attachment. If I use the clone method in save method it's to late, to prevent that I have to clone every object at the creation... I think that is not the good way. You can see here the ror implementation of the dirty feature http://api.rubyonrails.org/classes/ActiveModel/Dirty.html

Do you see the point ?

treffynnon commented 11 years ago

Just assign that value to a variable before you overwrite it or clone the object. I am not going to change this part of the library for backwards compatibility reasons.

This issue is closed.