Repositories and table gateways

[RalfEggert]

I merged the changes from @mtymek now. I like the addition of the repository but I think the way it is implemented is still suboptimal.

From my point of view the repository uses the table gateway or another data source and requests data or sends data to the persistence layer. The repository shouldn't really know how the data source / table gateway is using it. If it is a database, a web service or the file system.

In the current state, we have a mixture of SQL knowledge in both table gateway and repository.

I would prefer both. An AlbumTable that extends TableGatewayand provides the generation of the SQL objects. And an AlbumRepository that uses the AlbumTable for persistence by calling its methods and adding logic like the decision for an insert or an update.

Thoughts?

[geerteltink]

I hardly use ZendDB, mostly Doctrine. Doctrine is using only the repository for custom SQL. So in that case the repository knows exactly what the data source is.

What I'm missing is a service class. That class shouldn't know about the data source of the repository. But just pass the Album object to the repository. Preferably use a AlbumRepositoryInterface and you can easily swap between ZendDbAlbumRepository, DoctrineAlbumRepository or ExternalApiAlbumRepository.

[harikt]

One of the good read is http://shawnmc.cool/the-repository-pattern . I think the class works as expected.

One thing missing is only the interface . So you can have doctrine-dbal implementation or any implementation.

[codeliner]

awesome. Getting rid of this *Table classes (or at least move them to infrastructure layer) is a very good improvement. The suggestions by @xtreamwayz are definitely the way to go.
If you want inspiration take a look at my ddd-cargo-sample (new version is based on expressive, too):

• Cargo Entity
• Cargo Repository Contract
• Doctrine Cargo Repository
• Booking Application Service

A full DDD approach is of course not needed for the album tutorial, so skipping the application service would be fine, but hiding database stuff behind an interface should be part of every application.

[RalfEggert]

Thanks for the feedback so far. Then this implementation might be a good way to continue.

[mtymek]

Also in line what I was thinking. I will extract AlbumRepositoryInterface and make everything depending on it, instead of concrete implementation.

[mtymek]

BTW, which one should I use?

interface AlbumRepositoryInterface

or

interface AlbumRepository

I have a mixed feeling about this - for my projects I tend to add "Interface" suffix, but I'm not sure which is better.

[RalfEggert]

I personally would always prefer AlbumRepositoryInterface because you don't need to think if you have a type hint or a doc block comment. Just compare

    /**
     * @var AlbumRepository
     */
    private $albumRepository;

    /**
     * @param AlbumRepository $albumRepository
     */
    public function __construct(
        AlbumRepository $albumRepository
    ) {
        $this->albumRepository = $albumRepository;
    }

with

    /**
     * @var AlbumRepositoryInterface
     */
    private $albumRepository;

    /**
     * @param AlbumRepositoryInterface $albumRepository
     */
    public function __construct(
        AlbumRepositoryInterface $albumRepository
    ) {
        $this->albumRepository = $albumRepository;
    }

In the second example you know it is an interface at the first glance. In the first example you need to know or you need to look it up.

[geerteltink]

I lot of projects I have seen are using AlbumRepository as the interface name. I guess it's more a personal preference. If you implement the interface in ZendDbAlbumRepository or DoctrineAlbumRepository, you could leave the Interface suffix since you know it's an interface. Besides that, I don't think you need to know what it is you are injecting. Both the interface or the class implementing the interface should have the same public functions. Right?

[sandrokeil]

Using the interface suffix makes it easier for beginners, but I like the approach not to use the interface suffix.

+1 for interface AlbumRepository and use it as TypeHint everywhere and create concrete implementation classes like ZendDbAlbumRepository, DoctrineAlbumRepository or ExternalApiAlbumRepository together with a service class, as already mentioned by @xtreamwayz

[mtymek]

Done - PR created.

[mtymek]

I'd also like to make repository more strict:

• fetchSingleAlbum should return entity OR throw exception if it does not exist
• saveAlbum and deleteAlbum should not return anything. If entity cannot be saved - they should throw an exception.

This will simplify all action classes using this methods.

Thoughts?

[sandrokeil]

@mtymek insertAlbum and updateAlbum should also return nothing and should throw an exception if something goes wrong.

+1 for fetchSingleAlbum should return entity or throw an exception if it does not exists

[geerteltink]

saveAlbum and deleteAlbum should not return anything. If entity cannot be saved - they should throw an exception.

I understand this part. There is an error and thus an exception when it can't be saved. If there is no exception, it worked.

fetchSingleAlbum should return entity OR throw exception if it does not exist

Why should this throw an exception? Why not just return null?
I don't think that that it should be an exception if something is not there. It's just not there and thus a 404. A simple if (!$album) { throw 404; } would do. It could be the responsibility of the controller or service class to throw an exception if you like.

Just saying this in case you have intentions in adding a DoctrineAlbumRepository. If you do a find($id) it returns null, no exception. It's been a long time since I used ZendDb, but what I remember is that it didn't return an exception if it didn't find an object. I might be wrong though.
But if I remember it correctly, why change its behavior?

[harikt]

I agree with @xtreamwayz here. I have recently noticed some article that talks about throwing exception is not good. ( Don't remember the link ) .

It seems to me as a personal preference though.

[RalfEggert]

To be honest, I am still not convinced about the interface name without the Interface suffix. We should really keep the beginners in mind and help them to understand everything. Yes, in the tutorial text we could discuss why AlbumRepository for the interface is better than AlbumRepositoryInterface. People are looking into the tutorial to understand how Zend\Expressive and all of this middleware stuff works. The underlying software and model design can always be improved but maybe that can be another part or even another tutorial for advanced users.

So, please keep your focus on the beginners.

[mtymek]

As per Ralf's request, I changed AlbumRepository to AlbumRepositoryInterface.
I removed @return annotations from saveAlbum and deleteAlbum methods, where we all agree on behavior.

[mtymek]

@xtreamwayz,

fetchSingleAlbum should return entity OR throw exception if it does not exist

Why should this throw an exception? Why not just return null?

My understanding is that trying to fetch non-existent entry is not expected in application - it should never happen unless user is typing something weird in the URL. Unexpected situation = exception.
In applications I write I tend to throw Domain\EntityNotFoundException, and catch it somewhere in my error handler, in order to display nice 404 page. This reduces LOC in action class, while still looking good for end user.

[RalfEggert]

Thanks for the change of the interface name. Could you provide an example how you would handle the Not Found Exception in the Error handler?

[mtymek]

Here's how it could work in Expressive:

namespace App;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;

class Domain404Middleware
{
    public function __invoke($err, $request, $response, callable $next)
    {
        if (!$err instanceof EntityNotFoundException) {
            // different type of error - not our concern, 
            // pass it to the next error handler 
            return $next($request, $response, $err);
        }

        // Trick: passing empty response with no error information will 
        //  force FinalHandler to show default 404 page.
        // Alternatively - you can do something else here, like rendering 
        // non-default 404 view script.
        return $next($request, $response);
    }
}

You have to register it as an error middleware:

return [
    'middleware_pipeline' => [
        'post_routing' => [
            [
                'middleware' => Domain404Middleware::class,
                'error' => true,
            ]
        ]
];

Having this in place means you don't need to worry about 404 inside your action classes - you can focus on what's important. As I wrote, attempting to show non-existent entity is not a normal situation, so IMO handling this case can be driven by exceptions.

Having said that, it may be too difficult to understand for the beginners, blurring information in your tutorial. Your call if you want to have it 😄

[weierophinney]

My understanding is that trying to fetch non-existent entry is not expected in application - it should never happen unless user is typing something weird in the URL. Unexpected situation = exception.
In applications I write I tend to throw Domain\EntityNotFoundException, and catch it somewhere in my error handler, in order to display nice 404 page.

That's using exceptions to implement the logic flow, which is an anti-pattern.

Fetching a non-existent entry is expected, particularly when you have a route in the form of /resource[/:identifier]; if the value for :identifier does not map to an album, that's expected, and should return a 404. The question is: how do I identify such a situation?

I see four options:

• As you suggest, raising an exception. I don't like this option, as it's expected (not exceptional) behavior.
• Have multiple return types; return an AlbumEntity when it's found, return something else when it isn't. This requires the consumer to branch based on return type, however.
• Introduce a method into the AlbumEntity along the lines of isNotFound() or similar. Consumers would then branch on that:

  $album = $repository->fetchAlbum($identifier);
  if ($album->isNotFound()) {
      // 404 condition!
  }

• Introduce a hasAlbum() method. This would attempt to fetch the album, and return a boolean true when found, and a boolean false when not. In the case that it is found, the repository pattern indicates that the entity should be cached — which means that a later call to retrieve the album would not incur additional runtime overhead.

  if (! $repository->hasAlbum($identifier)) {
      // 404 condition!
  }
  $album = $repository->fetchAlbum($identifier);

The last two would be trivially easy to accomplish, and still be easy for beginners to understand. No additional middleware would be necessary to accomplish them, either.

[harikt]

Have multiple return types; return an AlbumEntity when it's found, return something else when it isn't. This requires the consumer to branch based on return type, however.

When passing multiple return types , the possible values can be null / AlbumEntity . And doesn't the hasAlbum , isNotFound also have a if else check? or probably I didn't get what you mean by This requires the consumer to branch based on return type @weierophinney . Could you expand a bit ?

[RalfEggert]

I had a bad feeling about throwing an exception if an entity is not found. Good to know that it is even an anti-pattern. Currently we have the second option that @weierophinney suggested. Personally, I would go for the fourth option because I think an entity should never have an invalid state.

@mtymek How should we proceed? Should I change the code base to implement the results of this discussion or would you want to amend your PR?

[RalfEggert]

Anyway @weierophinney what do you think about the repository <-> table gateway connection?

1. Should the AlbumRepository create an instance of Zend\Db\TableGateway\TableGateway within its factory and call the getSql() method to build SQL statements from the outside of the table gateway like most people agreed on?
2. Or should we have an AlbumTableGateway class which extends Zend\Db\TableGateway\TableGateway and encapsulates the SQL statements within some AlbumTableGateway methods. The AlbumRepository will get AlbumTableGateway injected and call these methods?
3. Is there another solution?

[mtymek]

@RalfEggert my latest PR doesn't throw exception if fetchAlbum doesn't find an entity - it was still under discussion, so I didn't implement it. You should be able to merge it as it is.

[mtymek]

Indeed, returning null or entity is not perfect, because you have to branch your code. This is, however, most commonly seen solution, anyone will understand it. So I suggest we stick with it in the tutorial.

I know this can get off topic, but it's also pretty interesting to exchange ideas. Thanks for sharing your thoughts @weierophinney! Still, let me defend my approach.

Other solutions are not perfect either:

• isNotFound() method on AlbumEntity seems to be exactly the same as multiple return types, just hidden within entity class. Every class consuming AlbumEntity could accept "not found" album as well, leading to difficult bugs (the topic is a bit controversial though, and there's plenty to read about it, for example: http://gorodinski.com/blog/2012/05/19/validation-in-domain-driven-design-ddd/)
• hasAlbum() method on repository seems to be the least controversial, just that it means having 2 DB queries to fetch single object.

As for exceptions - my line of thinking is that if you don't show URLs with invalid entity IDs, then you don't expect your users to request them. If they "follow the rules", they should never be fetching non-existent entity. If they don't follow the rules (they edit the URL manually) - it is an exceptional situation. I know it may be a bit exaggerated explanation, but it works pretty well in real life - code is both readable and bullet-proof.

[RalfEggert]

@mtymek Ok, I will try to proceed on that. But unfortunately not today.

hasAlbum() method on repository seems to be the least controversial, just that it means having 2 DB queries to fetch single object.

No, like @weierophinney already said it should be the job of the repository not to send two DB queries to fetch a single object. After on entity is read it can be added to an identity map and read from there for any subsequent fetching for the same id.

[geerteltink]

As for exceptions - my line of thinking is that if you don't show URLs with invalid entity IDs, then you don't expect your users to request them. If they "follow the rules", they should never be fetching non-existent entity. If they don't follow the rules (they edit the URL manually) - it is an exceptional situation. I know it may be a bit exaggerated explanation, but it works pretty well in real life - code is both readable and bullet-proof.

I don't think you can expect every request to be valid. There are also situations where urls used to be valid and users did not mess around and changed id's. Just to name a few 404 situations, but not exceptions:

• Users can have bookmarked urls which are pointing to deleted albums.
• Search engines can have obsolete results.
• Or internally, if an user adds an album to his wish list and the album is deleted, I can imagine you don't want to remove it from the wishlist for the user. He might be looking for a replacement album and keeping it in the wishlist, it reminds him to search for another version of that album.
• ...

[mtymek]

Not sure if I'm convinced, but thanks for sharing your arguments anyway - I will have something to think of when implementing this pattern next time.

@RalfEggert - can you merge what we already have? Based on that I can submit PRs for next parts of the tutorial.

[RalfEggert]

Well, I implemented some changes now. Please note that depending on issue #8 another couple of changes have been implemented.

Warning

I was very resistant to advice and selfish because I added an additional table gateway class which encapsulates all SQL generating stuff and keeps it out of the repository. I know that most of you had a different opinion on that subject. But I just wanted to demonstrate how I would handle the repository <-> table gateway connection in this case. I think it is much clearer if the repository doesn't contain any SQL logic because it is part of the model layer and not of the infrastructure layer.

To cut a long story short, now the repository is part of the model layer and the table gateway is part of the infrastructure layer.

If you have really good arguments that the repo should contain SQL logic, please convince me to change it back.

[codeliner]

I prefer to define the repository contract in the model layer Model\Album\AlbumRepository(Interface) but put the implementation in the infrastructure layer Infrastructure\Persistence\ZendDbAlbumRepository implements AlbumRepository . No need to split SQL from repository logic as the repository is a "translation adapter" between model and database.

[RalfEggert]

Sounds reasonable. But it is another step into a real DDD application design which is not the focus of the tutorial. The focus are the beginners who want to learn Zend\Expressive.

There might be no need to split SQL from the repository. But I still see no reason not to keep the SQL logic where it belongs to. And it belongs to the table gateway, otherwise there would be no reason to attach the Zend\Db\Sql\Sql instance to a table gateway. And I think this scenario is easier to understand for beginners as well.

But maybe I am on the wrong trail and I don't know it.

[codeliner]

@RalfEggert I would not say you are on the wrong trail. But we should better define our audience. You brought DDD into play. Let's do some DDD and ask some questions 😄 :

1. What means beginner for you? What are the skills of a beginner?
2. Is Zend\Db the best choice? Expressive aims to remove framework borders by defining interfaces and allowing the use of different libraries, be it another container, router or template engine.
3. Wouldn't it be a good idea to teach a beginner good design from the first day on? Beginners are not stuck in their minds. They didn't learn the wrong way and now want to learn a better design approach. They just want to learn.
4. Consists our audience perhaps more of experienced developers coming from other frameworks? If so, why should they use expressive in the future?

[RalfEggert]

1. In the last 2 years I trained almost three dozens teams to get to learn ZF2. The majority were experienced PHP developers working on a legacy project which exists for years. But there was also a group of rather unexperienced PHP devs. For a refactoring of their projects they want to start using a framework. So beginner to me means an experienced PHP developer wanting to learn the Zend Framework way.
2. No, Zend\Db is not the best choice. The Zend Framework way of live regarding the big M in MVC was always not to set too much defaults for how to implement a model. I think that is the biggest advantage of the ZF towards other PHP frameworks. Not to tell the user how to implement the model. You want to build your own model structure? You want to use Doctrine? You want to use a relational database, a NoSQL database or even flatfiles? Just go for it.
3. Yes, I am a big fan of teaching beginners how to structure their applications. But who defines a good design? You? Me? The gang of four? In think, a good design for beginners is a design they can easily catch and extend if they want and need to. In this tutorial code we already have entities, repositories, table gateways, database access, hydrators and other stuff in the model part. This can be improved in a lot of ways, maybe by domain events, intelligent (and not dumb) entities, value objects, aggregates, you name it. Every design can be improved at any time. The focus on the tutorial I suggested and provided the code for are beginners to Zend\Expressive and not beginners to a good model or domain structure. We shouldn't overload the tutorial with to much stuff that is not in the focus of the training.
4. I don't know if the audience comes from experienced developers from other frameworks. I wrote my personal experiences in 1. I think we might have both. But again, the beginners tutorial to Zend\Expressive should focus on the Zend\Expressive part and not on the "perfect design" everybody agrees to. If we want to achieve that we will never come to an end.

So again, in which part is the current model design really, really bad and should be improved? Any other suggestions for improvemen can always be part of the textual part of the tutorial...

[codeliner]

Thanks @RalfEggert for your detailed answer. Much easier now to understand your goal of the tutorial. Let me answer the same questions from my point of view:

1. For me beginners are rookies. They know how to code with PHP but don't know much about software architecture. What we show them in the tutorial will influence their learning.
   They will start to copy the ideas shown in the tutorial and port them to their projects. It would be great to find a way to tell them: Implementing the model is your part.
   We don't care about your model as it should not be part of any framework.
2. Zend\Db. TBH I'm not a fan of it. I prefer Doctrine\DBAL together with Doctrine\Migrations. In some situations also Doctrine\ORM/ODM but that depends. I think many people share this opinion.
   However, I can understand if Zend says, this is a zend-expressive tutorial so we should use as much zend components as possible. But looking at zend-expressive and related blog posts about it the new Zend Framework way is to tell people: "Use the best PHP tools for your project, no matter if they are from Zend, Symfony, Doctrine or whatever vendor". So why use Zend\Db and not Doctrine for the tutorial?
3. Good design has nothing to do with DDD in the first place. DDD is much more than using Entities, Repositories and so on. Good design depends on project requirements. If a CRUD-ish App is all you need, then go with it. If you need to tackle complex problems then enterprise application patterns are a good choice. The problem with a tutorial is, that in most cases no complex problems are solved. So the implementation shown in the tutorial is kept simple. But people port this simple code to their projects and try to solve complex problems with it. In most cases this end up in unmaintainable projects and unsatisfied developers. They won't say that they did a bad job but that the framework is bad. So my POV here is: the tutorial should be implemented as it would solve a complex problem rather than a simple problem. But this is only my personal opinion.
4. Why should developers use zend-expressive? Because it provides a powerful but also lightweight application layer using a middleware approach. PSR-7 is not only supported but a fundamental part of zend- expressive which allows you to easily integrate other vendor libraries which also support PSR-7 and/or other framework interoperability standards like container-interop.

Conclusion

Having all that said, I would drop Zend\Db in favor of Doctrine\DBAL and implement the repository pattern ;)

[mtymek]

Hey Ralf,

If you want to go for something easy to learn, then do something simpler - my PR was just like that:

• AlbumListAction uses AlbumRepositoryInterface to pull the data
• ZendDbAlbumRepository is an implementation, it consumes TableGatewaythat does data fetching

Your latest version adds one more step:

• AlbumListAction uses AlbumRepositoryInterface to pull the data
• ZendDbAlbumRepository is a repository implementation, it consumes AlbumTableGatewayInterface for data fetching
• AlbumTableGateway is a gateway implementation, extending TableGateway that does actual data fetching

ZendDbAlbumRepository has one purpose: to pull albums from the database - for this type of project there's no need to split even further. Currently what it does is mostly proxying to AlbumTableGatewayInterface. Unit test for this class would look weird - this is usually a good indication that code is over-complicated.

@codeliner take into account that this is supposed to be learning material for Expressive, not DDD. While I agree with you that we should show best practices to newcomers, introducing too many layers will only confuse them. We should show good practices (SOLID, decent test coverage...) - they won't distract readers. I have a feeling that nice DDD would require too many things to be explained.
Tutorial may link to some materials on DDD.

[codeliner]

@mtymek I NEVER said we should use DDD!

IMHO the tutorial should use Doctrine\DBAL + Doctrine\Migrations instead of Zend\Db and use the repository pattern.That is all. Again, this has nothing too do with DDD!

[RalfEggert]

First to clarify, when I wrote "No, Zend\Db is not the best choice." I missed the world always. I wanted to write "No, Zend\Db is not always the best choice.".

Second, @codeliner you did bring up the DDD stuff by posting your DDD examples. Yes, you didn't say "use DDD" directly and you made a limitation of not using application services for simplification. But that was at least a bit ambiguous because it could imply to do DDD only without application services. So I guess there was a little bit of misunderstanding here.

#6 (comment)

I have seen a couple of projects which almost failed because unexperienced users started huge projects based on Doctrine. Doctrine can be a beast when it gets complicated and you don't have the know how to handle this. Of course the same can happen when using Zend\Db but it is still easier to understand with the focus on Zend\Expressive. So, to rephrase myself here: "No, Doctrine is not always the best choice."

I guess we might never agree on this. So why not do both?

The Zend Framework was always lacking tutorials. I guess it wouldn't harm anyone to write a tutorial with the same database structure and features by using a lot of external dependencies (Doctrine, Plates, PimpleDI, ...). Then we could have one tutorial which uses mostly ZF components and one which is open for all of the nice other components out there. Having two (or even more) tutorials for Zend\Expressive will really demonstrate the flexibility.

What about this idea? Who would like to write such a more open tutorial?

[codeliner]

@RalfEggert Yeah, seems like my comments are a bit ambiguous. I'm sorry for that.
But I can only repeat myself:

but hiding database stuff behind an interface should be part of every application.

I've linked the cargo sample because it implements the design we are discussing here:

• Entity
• RepositoryInterface
• Database Abstraction Layer

Doctrine can be a beast

I agree. That is the reason why I suggest to only use Doctrine\DBAL + Doctrine\Migrations but not Doctrine\ORM.

What about this idea? Who would like to write such a more open tutorial?

We (prooph) can do that next year ;), but only if it becomes part of the official tutorial. We've already open sourced two example applications using zend-expressive together with different infrastructure components.

[mtymek]

@codeliner sorry, I misunderstood you then.

[RalfEggert]

I think it might be helpful for a better understanding and structure, when I tweek the Album/Db/AlbumTableGatewayInterface a little:

• Remove the extension of Zend\Db\TableGateway\TableGatewayInterface
• Move the interface from Album/Db to Album/Model/???
• Rename it from AlbumTableGatewayInterface to Album???Interface

I am still not sure how to name the subdir and the interface, though. I thought of Album/Model/Persistence/AlbumPersistenceInterface. Any other suggestions?

Then the AlbumTableGateway could implement that interface and we have decoupled it a little more.

What do you think about that?

[codeliner]

@mtymek thx, np.

@RalfEggert Sounds very good. Regarding ns and name: Album\Model\AlbumStoreInterface?

[RalfEggert]

Done