Ocramius / ChangeSet

:vhs: A simple changeset list that supports operations like insert/update/delete, event triggering, commits and rollbacks
MIT License
68 stars 9 forks source link

Feature: alias/inheritance types #25

Open Ocramius opened 9 years ago

Ocramius commented 9 years ago

This PR is about supporting inheritances/aliases:

class Foo { public $id; }
class Bar extends Foo {}

Since Bar is a subtype of Foo, requesting Foo to the IdentityMap should also look up in the Bar instances.

$bar = new Bar;

$bar->id = 123;

$identityMap->add($bar);

$this->assertSame($bar, $identityMap->getObject('Foo', 123));

The opposite is not true:

$foo = new Foo;

$foo->id = 123;

$identityMap->add($foo);

$this->assertNull($identityMap->getObject('Bar', 123));

I really could need some help on this, as I'm a bit confused about how to actually implement this logic in a performance-sensitive way.

I may also need to split the type resolver so that we actually loop through subclass/superclass types, or we duplicate the references in the identity map.

Thoughts anybody?

zerkms commented 9 years ago

(from a person who never used inheritance in DB/ORMs entities): it may be a problem when there is no constraint that guarantees that both Bar and Foo don't collide their id's.

Ocramius commented 9 years ago

As an additional feedback, I'd add that this behavior is actually configurable case-by-case. Following cases are also possible:

mnapoli commented 9 years ago

This is probably very naive but indexing the object in $this->objectsByIdentityMap for each parent class?

i.e. $this->objectsByIdentityMap['Foo-123'] and $this->objectsByIdentityMap['Bar-123'] would point to the same object… Cheap lookup, but maybe not cheap to add()?

Ocramius commented 9 years ago

@mnapoli yeah, that is one possible solution indeed, though it's not very efficient, isn't it?

mnapoli commented 9 years ago

Not so efficient for memory usage but I have no idea if the difference is actually problematic.

But in any case you will have to get the parent classes. But is that kind of metadata cached? If it is, then I guess it's just a matter of get/set X times in the array instead of once (where X is the number of parent classes). Shouldn't be too bad?

Ocramius commented 9 years ago

But is that kind of metadata cached?

Yep, I will cache everything possible

Shouldn't be too bad?

Probably better indeed (rather than slowing down the process for every type). I'll think about it, thanks :-)

nikita2206 commented 9 years ago

If there's a constraint that they all share the same id space then I would always go to the root and use its name. So for Foo extends Bar, with Foo#123 and Bar#827 there would be an IdentityMap{ Bar{ 123: Foo#123, 827: Bar#827 }}