-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added PersisterFactory to ORM. #1260
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3507 We use Jira to track the state of pull requests and the versions they got |
* | ||
* @return \Doctrine\ORM\Persisters\PersisterFactory | ||
*/ | ||
public function getPersisterFactory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't seem to fit the EntityManager at all: it would honestly be better to not expose it to end-users like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it seems the wrong way to expose it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hibernate exposes it at EntityManager level because it can be swappable with another implementation.
Default implementation is also reachable from outside, allowing you to provide a ServiceInjector (read as DI Container).
I'm ok to move this to UnitOfWork, but I would not consider this as an extension point (the part I wanted to discuss internally). This means there'll be no interface bound to PersisterFactory.
Right now I made it part of EntityManager and wanted to discuss IF we'd make it an extension point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: the UnitOfWork as-is is already poop: let's stash poop in there instead of shoveling it on other classes.
That makes our EntityManager some kind of locator (meh).
I'd prefer to make a tradeoff here, and do this crappy thing in the UnitOfWork instead (yes, with all the deficiencies that come from it).
The UnitOfWork as we know it will go away at some point in time, while the EntityManager will stay and be consumed by userland.
This change seems wrong overall: the |
* | ||
* @package Doctrine\ORM\Persisters | ||
*/ | ||
class PersisterFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be an interface with a default implementation to make it an extension point ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree: introducing a concrete implementation with no interface seems a bit messy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if we consider this as an extension point, which in the current state of this PR it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with guilherme, it is not an extension point at the moment.
General question, whats the benefit of this? Do we need htis refactoring? What for? |
@beberlei AS currently it is here, 0 benefits. |
I'll close this PR soon. Will implement a new one using PersisterFactory on UnitOfWork. |
Closing as |
No description provided.