SkyPHP / skyphp

PHP5 Framework
http://switchbreak.com/skyphp
17 stars 38 forks source link

[3.0] 1-to-1 objects should update without specifying id #256

Closed will123195 closed 10 years ago

will123195 commented 10 years ago
$album = new album([
    'album_id' => 1,
    'artist' => [
        'name' => 'xxx'
    ]
]);
$album->save();

Since artist is 1-to-1, this should UPDATE the artist record, but currently it inserts and changes $album->artist_id to the newly inserted value.

stanistan commented 10 years ago

From the given example, I don't think this is a bug. That looks like exactly desired behavior.

For it to be an update, the artist should be provided with the artist_id.


For the scenario provided this seems like a non-obvious code pattern, wouldn't you instead want to do:

album::get(1)->artist->update(['name' => 'xxx']); 

Or is a desired behavior to fetch the model on construct if the primary id is given in the data, and then merge?

This seems weird. The constructor should be lightweight.

If this is a desired behavior, it should be a separate method.

will123195 commented 10 years ago

It is a bug because the existing artist sub-object is orphaned unexpectedly and replaced with a newly inserted artist. It makes it really easy to "lose" data. I agree with your point in regard to 1-to-many sub-objects though. My example makes more sense in the context of saving form data, i.e.

POST
album_id=1
artist[name]=xxx  <-- this should update (id is implied)
tracks[0][id]=123
tracks[0][name]=yyy  <-- this should update (id must be given)
// save the posted form data
$album = album::get($_POST)->save();
will123195 commented 10 years ago

The drawback with an implied id is you can't do this with a single form submit:

$artist = artist::insert(['name' => 'xxx']);
$album->update([
    'artist_id' => $artist->id
])

But I think this is desirable...thoughts?

stanistan commented 10 years ago

I think adding artist[artist_id]=x would fix that.

Given how save behavior is (if I remember correctly), the artist would be saved first because of the foreign key order.


I like the idea of having a mode where everything that is an update running through the ORM would have already loaded that object-- and given that sub-objects are lazily loaded, everything is cached, and only changed fields are being saved, that shouldn't be an issue at all.

But having consistent behavior is good, regarding 1-1 and 1-many.

will123195 commented 10 years ago

Yea, I agree, I changed my mind. You'll have to explain what you mean about the other mode, I'm not following.

stanistan commented 10 years ago

So current behavior is to load $_POST data to the model and save, regardless of if data is actually being changed or not, it is being written, which isn't necessarily desired. If we know we are doing an update, should we load the model and see if there's actually a write happening (that data is being changed)?

That's kind of a corollary of the implied loading of the data in the creation of this thread.