nextras / orm

Orm with clean object design, smart relationship loading and powerful collections.
https://nextras.org/orm
MIT License
309 stars 59 forks source link

Implement IdentityMap::refresh #23

Closed hrach closed 7 years ago

hrach commented 10 years ago

In long-run processes it's quite commont we need to refetch actual state of entity in database. This api should be a little bit hidden in IdentityMap. refresh would refetch data and updated entity state. If entity isModified() === TRUE, exception should be thrown.

hrach commented 9 years ago

higher priority, I need it now. Yesterday was late :innocent: :angel:
the implementation will be different.

hrach commented 9 years ago

Todo:

hrach commented 9 years ago

Advanced implementation postponed after v1.0.0.

JanTvrdik commented 9 years ago

:+1: I need this now as well. For thread-safe critical code I'd like to do sth like

lock();
$repo->refresh($entity); // make sure I have latest data
doCiritalStuff();
unlock();
JanTvrdik commented 9 years ago

And I need this again.

hrach commented 9 years ago

Feel free to open PR. Master is ready to merge it :)

hrach commented 9 years ago

Refresh (probably) shouldn't touch relationship properties, it should update only non-relationship keys. Is it ok for your usecase, @JanTvrdik? Updating relationships would have to trigger recursive update, that could end badly.

hrach commented 9 years ago

Well, Doctrine do only recursive refresh: https://github.com/doctrine/doctrine2/blob/f88896cc9df756ce2d0e3f3f63bcaf24ef536dd5/lib/Doctrine/ORM/UnitOfWork.php#L2030-L2049

hrach commented 9 years ago

non-recurive impl: https://github.com/hrach/nextras-orm/commit/8cd04279d6a8d9f46ccac57e89838c678ccaed70

JanTvrdik commented 9 years ago

Yes, non-recursive refresh is currently (hopefully) enough for me.

f3l1x commented 9 years ago

@hrach I need it too.

I have before save trigger in DB and after persist and need to reload entity again with new data.

Mikulas commented 9 years ago

That would be our use case as well.

Generic refresh aside, this exact refresh could be solved with INSERT RETURNING (which returns when after-triggers are complete). This could be an opt-in feature of some advanced mapper; something like this in userland:

class Entity {
  // property $id
  // property $foo
  // property $rating
}

class Mapper {
  public function fieldsToRefreshAutomatically() {
    return ['rating'];
  }
}

and both update and insert queries woud be generated with the returning clause and then updating the fields.

f3l1x commented 9 years ago

@Mikulas Maybe.

Or just some method to clear entity in identity map.

Mikulas commented 9 years ago

It's not as easy as clearing the identity map, already existing entities (the one entity) must be updated.

As far as I can tell it only has upsides: one less query, returning will be faster then loading the data again in new query, it's automatic (devs won't be able to forget a refresh() call after new insert/update), and it's way more—as I find myself saying a lot lately—semantic: method in mapper vs calls to refresh. Also it's a mapper feature and it should be in mapper, not after every insert/update.

hrach commented 9 years ago

Guys, #68

hrach commented 8 years ago

68 was implemented. Postpoing this to 2.1, since I had to figure out the proper bahavior what to do, when something changes in relationships.

JanTvrdik commented 8 years ago

And I need this again.

hrach commented 7 years ago

Changing priority since refreshAll will be implemented in 3.0. #219

hrach commented 7 years ago

Well, closing for now. #219 & #227.