Permalink
Browse files

[DDC-884] Allow subclassing EntityManager.

  • Loading branch information...
1 parent b13c299 commit 04ab0cd8fcb90cf60b9ec764ff5eea7c4cbfbaeb @guilhermeblanco guilhermeblanco committed Feb 21, 2011
Showing with 1 addition and 1 deletion.
  1. +1 −1 lib/Doctrine/ORM/EntityManager.php
View
2 lib/Doctrine/ORM/EntityManager.php
@@ -725,6 +725,6 @@ public static function create($conn, Configuration $config, EventManager $eventM
throw new \InvalidArgumentException("Invalid argument: " . $conn);
}
- return new EntityManager($conn, $config, $conn->getEventManager());
+ return new self($conn, $config, $conn->getEventManager());
}
}

19 comments on commit 04ab0cd

@romanb

-1 ;)

@stof
Doctrine member

If you really want to make it subclassable it should be new static. self always references the class where the method using it is defined.

@beberlei
Doctrine member

I will revert this, subclassing the EntityManager is not a use-case we want to support.

@henrikbjorn

If reversed wont this break extensions like SoftDelete that jonwage did, it uses a EntityManager subclass if i remember correctly

@stof
Doctrine member

Jonwage's SoftDelete extension is for the ODM AFAIK.

Thus this commit does not allow subclassing. See my previous comment.

@henrikbjorn

Yes i am aware of the self vs static. My comment was to question why it should not be supported

@beberlei
Doctrine member

The API of the EntityManager spans pages in the manual. The contracts of all the methods are huge and mostly delegated to the UnitOfwork, which is the most complex object. Subclassing the EntityManager might always lead to certain nasty behaviors.

@avalanche123
Doctrine member

Let's declare it as final then?

@beberlei
Doctrine member

Yah but then you cant mock it anymore. Maybe make an interface for it though.

@avalanche123
Doctrine member

You should mock by interface. Mocking a concrete class is asking for trouble anyway. Now that Jon added manager interfaces, its possible

@guilhermeblanco
Doctrine member

I don't think it this should be reverted.
We should (and I agree with most parts here) prevent user from doing mistakes. But there are use cases for this change, such as a implementing a custom behavior to EntityManager. This is a limitation I experienced when I was playing with HierarchicalManager, which lead me to do workarounds to fulfill this limitation.

Although we should not encourage people to extend EntityManager, there're situations where it is required, and we're preventing them. I consider this as a limitation which could be fixed.

Cheers,

@romanb

Inheritance is never "required". There are always other good (even better) ways to do things.

@romanb

The decision is in the hands of Benjamin. I can just say, once you officially allow and encourage that, people will do it like hell and use it as their standard mechanism for "reusing" or "extending" the EntityManager functionality and it is highly unlikely that they do that with so much care that the contracts of the public API are not being broken. Not to speak of the heavy burden of backwards compatibility of the EM and UoW if you now have to take care not to break subclasses :-)

@henrikbjorn

We dont have to encourage it, but we also dont have to punish extendability on the other hand. What about a Versions behavior thing, i think i have seen one and that subclassed the manager aswell.

@romanb

Inheritance != Extensibility

@avalanche123
Doctrine member

The solution in those cases would be to wrap real entity manager in versionable entity manager for example - use composition.

@jwage
Doctrine member

My extension for the ODM does not use inheritance, it uses composition. You have a new SoftDeleteManager instance that wraps the DocumentManager. I am -1 for this. Much better ways to implement this without a delicate inheritance solution.

@beberlei
Doctrine member

Inheritance to add behaviors is a bad idea, because you can obviously only add one behavior or add them all into one class. Neither is good OOP practice. Composition is much more useful for the EntityManager.

@beberlei
Doctrine member

I reverted the change.

Please sign in to comment.