ice / framework

Source code of Ice framework
https://www.iceframework.org
BSD 3-Clause "New" or "Revised" License
341 stars 45 forks source link

Model, refresh new data after updated #244

Closed Yahasana closed 5 years ago

Yahasana commented 5 years ago

fixed model loss its data (e.g. primary key) after updated to db

mruz commented 5 years ago

It shouldn't lose it's data, we don't remove primary key here: https://github.com/ice/framework/pull/244/files#diff-adc4d2a2f6f7dca511a6e8073769eb86R378 Also as I said before https://github.com/ice/framework/pull/202#discussion_r221505817, maybe we could move to fields() this https://github.com/ice/framework/pull/244/files#diff-adc4d2a2f6f7dca511a6e8073769eb86R413.

Yahasana commented 5 years ago

$m->update(['a' => 1]);

$m has no primary key now

mruz commented 5 years ago

I added some tests, but they passed https://github.com/ice/framework/pull/246/files Is the primary key an array? or auto-increment is false?

Yahasana commented 5 years ago

Oops, I change my repos to support $m->update(['a' => 1]);. the correct way to work with yours repos should be

$m->a = 1;
$m->update(['a']);

goddamn ugly implement

mruz commented 5 years ago

Both ways should work... I just wonder why they first isn't working for you.

mruz commented 5 years ago

Fields with data are merged here https://github.com/ice/framework/blob/dev/ice/mvc/model.zep#L240, so we don't need to merge after. We just need rollback if failed https://github.com/ice/framework/blob/dev/ice/mvc/model.zep#L421.

Yahasana commented 5 years ago

L240 only works for the case $m->update() without fields parameter or $m->update(['logins' => 1]) with empty $this->fields but doesn't work for $m->update(['logins']).

primary key are removed L226, and other unrelated data fields are removed at L243 or L257 or L260. L378 make the changes happen.

EDIT: check pr #248 plz

mruz commented 5 years ago

Right, your fix should work then. Could you please merge the tests here #248?