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

Doctrine Repositories best practice #13

Open
webdevilopers opened this Issue Jun 2, 2016 · 16 comments

Comments

Projects
None yet
9 participants
@webdevilopers
Owner

webdevilopers commented Jun 2, 2016

@webdevilopers

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Jun 3, 2016

Owner

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:

Owner

webdevilopers commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MacDada

MacDada Jun 3, 2016

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.

MacDada commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Jun 27, 2016

Owner

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:

  • Acme\Infrastructure\Persistence\Doctrine\MysqlFooRepository
  • Acme\Infrastructure\Persistence\Doctrine\MongoFooRepository
Owner

webdevilopers commented Jun 27, 2016

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:

  • Acme\Infrastructure\Persistence\Doctrine\MysqlFooRepository
  • Acme\Infrastructure\Persistence\Doctrine\MongoFooRepository

@webdevilopers webdevilopers referenced this issue Jun 27, 2016

Open

[WIP] Undoctrine doctrine specification #82

17 of 31 tasks complete
@MacDada

This comment has been minimized.

Show comment
Hide comment
@MacDada

MacDada Jun 27, 2016

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.

MacDada commented Jun 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Aug 11, 2016

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.

sstok commented Aug 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Sep 25, 2016

Owner

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:

Owner

webdevilopers commented Sep 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Nov 14, 2016

Owner

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

Owner

webdevilopers commented Nov 14, 2016

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

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 17, 2017

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...

deeky666 commented Jan 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Jan 17, 2017

Owner

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!

Owner

webdevilopers commented Jan 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ScreamingDev

ScreamingDev Mar 22, 2017

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.

ScreamingDev commented Mar 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@webdevilopers

webdevilopers Mar 22, 2017

Owner

@sourcerer-mike Have you tried @Happyr Doctrine Specifications as showcased in #17 ?

Owner

webdevilopers commented Mar 22, 2017

@sourcerer-mike Have you tried @Happyr Doctrine Specifications as showcased in #17 ?

@yvoyer

This comment has been minimized.

Show comment
Hide comment
@yvoyer

yvoyer Mar 27, 2017

Collaborator

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:

  • Return an array of the inner objects using Collection::getValues(), when the inner object is a value object of your domain.
  • Return a map of the Identity using Collection::map() (when working with aggregate that should not be know from outside the aggregate)

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
Collaborator

yvoyer commented Mar 27, 2017

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:

  • Return an array of the inner objects using Collection::getValues(), when the inner object is a value object of your domain.
  • Return a map of the Identity using Collection::map() (when working with aggregate that should not be know from outside the aggregate)

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

This comment has been minimized.

Show comment
Hide comment
@J7mbo

J7mbo May 5, 2017

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?

J7mbo commented May 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Federkun

Federkun May 6, 2017

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

Federkun commented May 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius May 6, 2017

Ocramius commented May 6, 2017

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