webdevilopers / php-ddd

PHP Symfony Doctrine Domain-driven Design
200 stars 11 forks source link

Doctrine Repositories best practice #13

Open webdevilopers opened 8 years ago

webdevilopers commented 8 years ago

For naming conventions please see:

Injecting EntityManager only and the Law of Demeter:

Extend the Doctrine EntityRepository as suggested by @MacDada?

Blending ORM and ODM:

webdevilopers commented 8 years ago

On extending the EntityRepository: I've seen several examples inside the @dddinphp PHP in DDD book doing it:

class DoctrineOrderRepository
    extends EntityRepository
    implements OrderRepository

I guess this is a matter of taste similar to using ArrayCollection in your Domain Models:

MacDada commented 8 years ago

I guess this is a matter of taste similar to using ArrayCollection in your Domain Models

BTW, I try to avoid ArrayCollection in accesors' phpdocs.

class Foo
{
    /**
     * @var ArrayCollection|Bar[]
     */
    private $bars;

    /**
     * @return ArrayCollection|Bar[]
     */
    public function getBars()
    {
        return $this->bars;
    }
}

/\ That way any object could modify my $bars collection outside Foo. For example, this is possible:

$foo->getBars()->add('haha, not a Bar');

That's why I limit phpdoc for getBars() to just Bar[]:

class Foo
{
    /**
     * @var ArrayCollection|Bar[]
     */
    private $bars;

    /**
     * @return Bar[]
     */
    public function getBars()
    {
        return $this->bars;
    }

    public function addBar(Bar $bar)
    {
        $this->bars->add($bar);
    }
}

Now the devs are "forced" to use addBar() method which will guard for only Bar elements in the collection.

It is still technically possible to modify $bars from the outside, as the returned stuff is actually an ArrayCollection for PHP. But it's not for developers. Developers won't know that (shouldn't know, shouldn't care) and are forced to treat it as array.

Well, not exactly array, as they cannot do array_map() for example. But they can do foreach or [2].

If I want to give more power "for the outside", I would do Bar[]|Traversable|Selectable, etc.

But I usually want to encapsulate any stuff related to $bars collection inside Foo.

webdevilopers commented 8 years ago

A nice compromise on Naming Conventions by @theUniC:

I think that naming them DoctrineORMFooRepository or DoctrineODMBarRepository it's a pretty simple and good solution.

But I guess I will go for:

MacDada commented 8 years ago

Acme\Infrastructure\Persistence\Doctrine\MysqlFooRepository

@webdevilopers That's a bad name. U can switch Mysql to Progress in one minute when using Doctrine ORM/DBAL. It's a matter of configuration. Doctrine gives u abstraction over SQL databases – unless u use "native queries", your repo won't change when u change the DB vendor.

Acme\Infrastructure\Persistence\Doctrine\SqlFooRepository Acme\Infrastructure\Persistence\Doctrine\DbalFooRepository Acme\Infrastructure\Persistence\Doctrine\OrmFooRepository

Would be better.

sstok commented 8 years ago

Concerning the Collection problem, you could return a wrapped/decorated Collection that only allows getting but not modifying. Internally you still have the actual mutable Collection 👍

Whenever a getter method is called you forward to the internal collection, but when someone tries to call remove() they get an exception.

webdevilopers commented 8 years ago

Some more notes on extending repositories:

Some of the @dddinphp book examples show the minimum implementation for EntityRepository using a custom constructor:

class DoctrinePostRepository implements PostRepository
{
    protected $em;

    public function __construct(EntityManager $em)
    {
        $this->em = $em;
    }

Personally I prefer extending the @doctrine EntityRepository - an approach which is shown in some @dddinphp examples later too:

class DoctrineOrderRepository
    extends EntityRepository
    implements OrderRepository

Any reasons why the second implementation is mentioned only later in the @dddinphp book @carlosbuenosvinos ?

In order to make the second solution work in @symfony using the getRepository method you have to define your custom repository e.g. via annotations:

/**
 * @ORM\Entity(repositoryClass="Acme\Infrastructure\Persistence\Doctrine\OrderRepository")
 */

In order to use these repositories in your services you should create a service in order to inject them.

The ODM version looks similiar:

/**
 * @MongoDB\Document(repositoryClass="Acme\Infrastructure\Persistence\Doctrine\OrderRepository")
 */

The service:

order.repository:
    class: Acme\Infrastructure\Persistence\Doctrine\OrderRepository
    factory_service: doctrine_mongodb.odm.document_manager
    factory_method:  getRepository
    arguments: ["Acme\AppBundle\Document\Order"]

As answered by @ahmedmhmd here:

Related issues:

webdevilopers commented 7 years ago

After a lot of research - thanks for everybody involved here - I started my first blog post about my experiences with this topic:

webdevilopers commented 7 years ago

Some related posts:

@kbond

deeky666 commented 7 years ago

Being a member of the Doctrine team I would like to add my 2 cents on this topic.

Personally I don't like very much the bloated API that Doctrine repositories expose. IIRC we had some discussions about that and a general consensus about that was that we would not implement it like this again. It is too much of a facade to the entity manager.

My personal opinion is that the repository should basically really only behave like a collection as Martin Fowler describes and limit its responsibilities to being a thin layer to the data mapper / storage. If we had generics in PHP, I would even define a generic repository interface like for a general collection or map interface. Methods like save() / persist() / insert() / update(), flush(), createQuery() should not be part of a repository as those are too specific for databases. Instead one should use term like add, filter, reduce, retain etc. like in a regular collection.

This is also the reason why I don't extend the Doctrine repository in my projects but define my own thin interfaces instead and simply inject the entity manager.

Just my opinion, though...

webdevilopers commented 7 years ago

Thanks for your thoughts @deeky666 . I get your point!

I guess the same is true for the @hibernate API since @doctrine was inspired by it?

The examples in my blog indeed take full "advantage" of what Doctrine "offers". Guess my future implementations will look similar to your suggestions too.

Would like to hear what @Ocramius thinks about this too!

ScreamingDev commented 7 years ago

Just found this. Cool! I have some neat thoughts to share too. Queries have been an issue to me. On the one side building those in controller would be very flexible but on the other hand the majority of developer would vasectomize me for that. So I used DDD itself to build simple "queries" like this:

abstract class AbstractRepository implements RepositoryInterface
{
protected function createQueryBuilder($amount = null, $start = null, $order = [])
    {
        $qb = $this->repository->createQueryBuilder('e');

        if ($amount) {
            $qb->setMaxResults($amount);
        }

        if ($start) {
            $qb->setFirstResult($start);
        }

        foreach ($order as $field => $direction) {
            $qb->orderBy($field, $direction);
        }

        return $qb;
    }

    public function findAllSimilar($entity, $amount = null, $start = null, $order = [])
    {
        $qb = $this->createQueryBuilder($amount, $start, $order);

        $this->parseQuery($entity, $qb);
    }

    abstract protected function parseQuery($entity, $queryBuilder);
}

First of all: This is done using the query builder because the Doctrine Paginator likes it. With this you can fill an entity with some information and use it as a filter:

$human = new Human();
$human->setGender('yes');
$human->setColor('#ABC');
$human->setOccupation('dev');

$others = $repo->findSimilar($human);

This should explain it already. In the concrete repo the ::parseQuery method is implemented:

class HumanRepo extends AbstractRepository {
    protectec function parseQuery($human, $queryBuilder) {
      if ($human->getGender()) {
        $queryBuilder->andWhere($queryBuilder->expr()->eq('e.gender', $human->getGender()));
      }

      // and so on
    }
}

This made prototyping very easy and gave the controller some flexibility.

webdevilopers commented 7 years ago

@sourcerer-mike Have you tried @Happyr Doctrine Specifications as showcased in https://github.com/webdevilopers/php-ddd/issues/17 ?

yvoyer commented 7 years ago

It is still technically possible to modify $bars from the outside, as the returned stuff is actually an ArrayCollection for PHP. But it's not for developers. Developers won't know that (shouldn't know, shouldn't care) and are forced to treat it as array. - MacDada

@MacDada Usually I try to never expose the inner parts of my objects to clients. When using Collection on an object I only give basic PHP type as return value or value object from my domain.

To fix the problem of client modifying the inner collection of the object on getBars(), I use one of the following strategy:

ie.

final class Post
{
    /**
     * @var ArrayCollection|Comment[]
     */
    private $comments;

    public function writeComment(CommentId $id, $text)
    {
        $comment = new Comment($id, $this, $text, new \DateTime());
        $this->comments[] = $comment;
    }

    /**
     * @return CommentId[]
     */
    public function comments()
    {
        return $this->comments->map( // Return Identity when the inner object is an aggregate also
            function (Comment $comment) {
                return $comment;
            })->getValues();
    }

    /**
     * @return Comment[]
     */
    public function comments()
    {
        return $this->comments->getValues(); // Try to avoid returning the objects owned by the agggregate, when they are created internally 
    }
}

I use getValues() instead of toArray() to avoid having missing keys when elements were removed before.

$collection = new ArrayCollection([3, 4, 5]);
var_dump($collection->toArray()); // [0 => 3, 1 => 4, 2 => 5]
$collection->remove(4);
var_dump($collection->toArray()); // [0 => 3, 2 => 5] missing the 1 key
var_dump($collection->getValues()); // [0 => 3, 1 => 5] all keys are ok
J7mbo commented 7 years ago

Some colleagues started using DoctrineSqlRepository implements Repository, whereby the Repository interface declares a save() method.

My question here came from this tweet:

It's calling both persist and flush (doctrine-specific implementations) here that's the problem - multiple flu[s]hes

In our concrete doctrine repository, calling save() would have to perform the following two operations to be considered adhering to the well defined interface's save() method:

$em->persist($entity) and $em->flush($entity)

Many times, many multiple repositories may be used within a single request, and this is perfectly fine. The problem comes with our concrete save method calling flush($entity) many times.

Solutions I don't like include having an out-of-infrastructure flush on application shutdown. Any other solutions when using Doctrine repositories?

Federkun commented 7 years ago

As has already been said, a save/update method leak into the domain the fact that there's some kind of persistence storage under the hood. After all, Repositories are just Collections. Tracking changes should be the responsability of the UoW.

I like @carlosbuenosvinos's approach:

$applicationService = new MyApplicationService($someRepository, $someOtherRepository);
$applicationServiceProxy = new TransactionalApplicationService(
    $applicationService,
    new DoctrineSession($entityManager)
);

// a single unit of work is used, within a single transaction 
$applicationServiceProxy->execute($request);

https://github.com/dddinphp/ddd/blob/master/src/Infrastructure/Application/Service/DoctrineSession.php https://github.com/dddinphp/ddd/blob/master/src/Application/Service/TransactionalApplicationService.php

Ocramius commented 7 years ago

Please never ever flush($entity) if this is about doctrine. Don't have details here, but that is not a limitation on persistence operations, just a performance optimization.

On 5 May 2017 15:59, "James Mallison" notifications@github.com wrote:

Some colleagues started using DoctrineSqlRepository implements Repository, whereby the Repository interface declares a save() method.

My question here came from this tweet https://twitter.com/J7mbo/status/859776463972823040:

It's calling both persist and flush (doctrine-specific implementations) here that's the problem - multiple flu[s]hes

In our concrete doctrine repository, calling save() would have to perform the following two operations to be considered adhering to the well defined interface's save() method:

persist($entity) and $flush($entity)

Many times, many multiple repositories may be used within a single request, and this is perfectly fine. The problem comes with our concrete save method calling flush($entity) many times.

Solutions I don't like include having an out-of-infrastructure flush on application shutdown. Any other solutions when using Doctrine repositories?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/webdevilopers/php-ddd/issues/13#issuecomment-299471990, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakCQ8u28QyMIyrhZZs1AKSBYqVUqfks5r2ytHgaJpZM4IseHA .

akomm commented 4 years ago

I have the feeling, all the blog posts, presentations and/or sliders explicitly and purposely dodge being really practical in terms of DDD, when it comes to explain practical usage of it in combination with persistence (as a separate layer over it).

They either do it obviously wrong or dodge major issues of their solution by avoiding those being part of the example.

You often see a Doctrine ORM implementation of collections. One totally unmentioned thing is: this implementation can not be part of domain, even though the interface is part of it, because the common proposed solution to adding and removing entities via collection is calling ->persist and ->remove on entity manager inside the add and remove methods of those collections. You do not want persistence handling to be part of your domain...

Secondly, those solution, if they don't call it done there, continue with having a service that depends on EntityManager and a Repository, which adds the entity via Repository::add and then calls EntityManager::flush. This has some issues in my eyes. First of all, Repository::add in those examples also depends on EntityManager. But there is no contract/guarantee that those instances of EntityManager, which was injected into the service and the repository are the same. Secondly, if you work with Interfaces of said types (both EntityManager and Repository), a magical relation between the add, remove and the EntityManager's flush implementation is assumed. One thing to note is, that the service we speak here about must be application level service and not domain, because it calls ->flush which seem to be never mentioned - so its just my conclusion (correct?).

Another often dodged, or incorrectly proposed solution is: updates. From domain point of view, when I get an entity from a collection and $entity->rename('valid name');, the change is done. The collection contains a reference to the $entity and, assuming successful, the name has been changed. There is no explicit save it needed. It would not make any sense. However, to have the change persisted on application level, I basically have to use an application level service which 1. fetches from a collection, 2. performs or delegates the operation and 3. calls flush. Implementing the issue I explained just above.

@Federkun's example seem to go exactly this direction.

I have yet to find a book/blog post/whatever source, which does not make some non-sense or breaks own rules, to supposedly achieve true DDD. All are cheating by doing some compromises without telling it. One who reads it then tries to figure out how to make the "true" DDD, while there is no trace of hint that its possible at all, when you consider actually persisting the data in SQL-Like databases/using ORM. On its own, DDD is obviously not the issue. I hope its clear.

I am actually currently glancing at using functional approach to it (immutable data, composition) and document based database like for example arangodb. Dozens of issues separating persistence and domain just vanish. Documents also matches quite well the Aggregate Idea in terms of persisting such an aggregate on application level.