Skip to content

Implemented an ObjectPersisterInterface for entity/object storage#317

Closed
linaori wants to merge 2 commits intodoctrine:masterfrom
linaori:master
Closed

Implemented an ObjectPersisterInterface for entity/object storage#317
linaori wants to merge 2 commits intodoctrine:masterfrom
linaori:master

Conversation

@linaori
Copy link

@linaori linaori commented Feb 21, 2014

Why is this useful?

Instead of using the repositoryClass, we use 'Repository as a Service'. This means that we inject our repository service instead of doing $em->getRepository('MyEntity'), this provides typehinting and autocomplete in IDEs. This introduces a minor inconvenience, we need to inject an EntityManagerInterface to provide persist/flush. We don't want the entire EntityManagerInterface capabilities just to store an object.

Using the ObjectPersisterInterface we can "hide" the EntityManagerInterface and only let the code know we have the persist and flush methods available. By default this is implemented on the EntityManager, but is also possible to be mocked or replaced by a custom implementation (rotating entity managers?).

Example

class MyService
{
    private $op;
    private $es;

    public function __construct(ObjectPersisterInterface $op, MyEntityService $es)
    {
        $this->op = $op;
        $this->es = $es;
    }

    public function doSomething($value)
    {
        $entity = $this->es->findByValue($value)->setValue('something');
        $this->op->flush();
    }

    public function doSomethingElse()
    {
        $entity = new MyEntity();
        $this->op->persist($entity);
        $this->op->flush();
    }
}

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-235

We use Jira to track the state of pull requests and the versions they got
included in.

@wouterj
Copy link

wouterj commented Feb 24, 2014

👍

1 similar comment
@fprochazka
Copy link

👍

@beberlei
Copy link
Member

You can use composition to hide the the EntityManager. I don't think this is really necessary to have in Doctrine core, because it doesnt add much benefit. Its much better you provide your own interface, wrap the EntityManager and then have something with a much better name.

@beberlei beberlei closed this Feb 24, 2014
@Ocramius
Copy link
Member

@beberlei I still think that the interface being splitted in common allows for more granular reuse from all those packages that directly depend on doctrine/common.

Re-defining a persister interface in sub-packages can obviously be done, but why not having a persister that does not handle finder logic?

For example, a queue where to push objects to be "saved" (don't know what it does, internally) will never be able to implement the ObjectManager without the assumption that we can retrieve the data from it afterwards.

I'm fine with having such an interface in my own package, but having it here would do no harm either IMO.

@linaori
Copy link
Author

linaori commented Feb 24, 2014

@beberlei, Its part of Doctrine, Doctrine is lacking a proper interface to actually store things. You are always forced to directly program on the EntityManager like this. If so, then why even bother hiding it in the repository? I can't interface this myself, it would be used on too many of our bundles/projects. Making a composer package for 1 interface is a no-go too.

If not like this, then how are people supposed to properly flush without having the EntityManager flying around everywhere? I know for sure that if I pass the EntityManagerInterface around, people will start using calls at places they shouldn't. I don't want to depent on the world if I need 2 methods.

There is animo for this PR, so why instantly close it?

@wouterj
Copy link

wouterj commented Dec 8, 2015

Given that a core team member thinks this kinda makes sense, can this please be reconsidered? Having to create an interface doing this in all packages seems strange...

@Ocramius
Copy link
Member

Ocramius commented Dec 9, 2015

@wouterj looked at it again, and I don't think we should re-evaluate this before having a plan for new APIs (3.x). For now, composition is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants