Eloquent Repository

try

use App\User as User;

Why a singleton?

Look to your EloquentRepository __construct and the return's of the singleton, I think they don't match.

@luddinus I'm going by what he suggests in his videos.

The way you defined EloquentUserRepository constructor method make passing a Model obligatory, so on the DatabaseServiceProvider, pass a new User model along with the constructor:

$eloquentRepo = new EloquentUserRepository(new User);

This will do the trick.

@fraserk Are you sure? He doesn't do that in his.

@rodrigo.pedra Can I do it a different way so that I don't have to do it in the DatabaseServiceProvider?

For some reason phpStorm8 is squawking over two parts of the register method. I don't know if this has anything to do with my current code or not.

$this->app->singleton(UserRepository::class<-----------says class name constant is available in PHP 5.5, function(){
            $eloquentRepo = new EloquentUserRepository <----------- says Requred parameter $model missing;

            $cachingRepo = new CachingUserRepository(
                $eloquentRepo, $this->app['cache.store']
            );

            return new LoggingUserRepository($cachingRepo, $this->app['log']);
        });

Yes you can, you could use the static methods from the User model, but I think this approach is better.

I think the video you are watching is this one: [ https://laracasts.com/lessons/decorating-repositories ], right?

Having an empty model to access table-level methods (->find(), '->get()`, etc.) is not a bad thing, it actually makes your class more clear about its dependencies.

The ConversationsRepository does not need to take any parameter in the constructor, because the getAll() method returns a plain array. But if you look around minute 12:50, you'll see Jeffrey decorates the Repository by passing the Eloquent one as a parameter to the Caching one.

Let's stick to your doubt, actually you can do both ways:

    /**
     * Passing the empty model as a constructor's parameter
     * 
     * Pros: 
     *  1. Easier to test, as you have explicit dependencies
     *  2. Easier to reuse, as you could pass a sub-class of User as the constructor
     *  
     * Cons:
     *  1. By doing so, you should pass a model when registering in a service provider or instantiating:
     * 
     *      $instance = new EloquentUserRepository( new User );
     * 
     * So Laravel's know how to instantiate it correctly
     *
     */
    class EloquentUserRepository {
        protected $model;

        public function __construct( User $model )
        {
            $this->model = $model
        }
        
        
        public function getAll() {
            return $this->model->all();
        }
    }

/**
 * No constructor's argument
 * 
 * Pros: 
 *  1. Easier to write, you delegate the inner work to the eloquent magic
 *  2. No need to bother with Providers and other bureaucracies.
 *  
 * Cons:
 *  1. hard to test, as you'll need to mock the dependencies for each test, 
 *     and you cannot predict which static classes are being used in each method
 *  2. Pretty much not reusable
 */
class EloquentUserRepository {
    public function __construct( )
    {
    }
    
    
    public function getAll() {
        // use static method
        return User::all();
    }
}

As you can see both ways leads to the same results, I prefer the first one as it leads to better code that is easier to maintain in the long run. But as @JeffreyWay frequently says in his videos, there is no single way to do it, and if it is a small project, there is no problem at all to delegate to the framework some tasks that it does very well.

@rodrigo.pedra

That's a great response. However I also have a base repository file that the EloquentUserRepository extends.

And now I'm also to the point where I don't know if I still need all of these or if I'm doubling up on something. Can you help straighten me out to see if I'm getting the wrong idea somewhere.

<?php

namespace App\Repositories;

abstract class EloquentRepository {

    protected $model;

    function __construct($model)
    {
        $this->model = $model;
    }

    public function getAll()
    {
        return $this->model->all();
    }

    public function getById($id)
    {
        return $this->model->findOrFail($id);
    }

}

<?php namespace App\Providers;

use App\Repositories\Users\LoggingUserRepository;
use Illuminate\Support\ServiceProvider;
use App\Repositories\Users\UserRepository;
use App\Repositories\Users\EloquentUserRepository;

class DatabaseServiceProvider extends ServiceProvider {

    /**
     * Bootstrap the application services.
     *
     * @return void
     */
    public function boot()
    {
        //
    }

    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        $this->app->singleton(UserRepository::class, function(){
            $eloquentRepo = new EloquentUserRepository;

            $cachingRepo = new CachingUserRepository(
                $eloquentRepo, $this->app['cache.store']
            );

            return new LoggingUserRepository($cachingRepo, $this->app['log']);
        });
    }

}

<?php

namespace App\Repositories\Users;

interface UserRepository
{
    public function getAll();

}

<?php

namespace App\Repositories\Users;

use App\User;
use App\Repositories\EloquentRepository;

class EloquentUserRepository extends EloquentRepository implements UserRepository
{
    protected $model;

    public function __construct(User $model)
    {
        $this->model = $model;
    }

}

@xtremer360 , sorry for taking so long, I moved this weekend and got internet installed at home just today :)

Ok, let's see if we can nail it.

As your EloquentUserRepository overrides the EloquentRepository's constructor, the best way to go is to call its parent constructor, so for this file you could do like this:

<?php namespace App\Repositories\Users;

use App\User;
use App\Repositories\EloquentRepository;

class EloquentUserRepository extends EloquentRepository implements UserRepository
{
    public function __construct(User $model)
    {
        // no need to duplicate the protected attribute initialization
        // as the parent class already does that
        parent::__construct( $model );
    }
}

The Service Provider is a pattern where, if the framework has dependency injection, it can automatically provide the correct instance of a required class, so we use the register (...) method to let Laravel know how it should correctly instantiate a class when it is requested. As your EloquentUserRepository class requires an argument on instantiation (__construct()'s argument) we should provide that while registering, so change the DatabaseServiceProvider to this:

<?php namespace App\Providers;

use App\Repositories\Users\LoggingUserRepository;
use Illuminate\Support\ServiceProvider;
use App\Repositories\Users\UserRepository;
use App\Repositories\Users\EloquentUserRepository;

use App\User; // **Added!**

class DatabaseServiceProvider extends ServiceProvider {

    /**
     * Bootstrap the application services.
     *
     * @return void
     */
    public function boot()
    {
        //
    }

    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        $this->app->singleton(UserRepository::class, function(){
            // note we 'imported' the class' full namespace with **use** on top of the file
            $eloquentRepo = new EloquentUserRepository( new User ); 

            $cachingRepo = new CachingUserRepository(
                $eloquentRepo, $this->app['cache.store']
            );

            return new LoggingUserRepository($cachingRepo, $this->app['log']);
        });
    }
}

For more info on how to use Service Providers see this laracast: [ https://laracasts.com/lessons/service-providers-decoded ]

Regarding your PHPStorm errors (I assume you are using version 8):

1. class name constant is available in PHP 5.5 => Go to "Settings" > "Language & Frameworks" > "PHP" > Change "PHP Language Level" to 5.5
2. Required parameter $model missing => This should be fixes with the changes we made in this post, if PHPStorm still complains about it, see this laracast: [ https://laracasts.com/series/how-to-be-awesome-in-phpstorm/episodes/15 ] and check this package: [ https://github.com/barryvdh/laravel-ide-helper ]

Check the whole laracasts' series: [ https://laracasts.com/series/how-to-be-awesome-in-phpstorm ]

Hope it helps!

Well for the most part does. I was able to fix #1 of the fixes for PHP Storm 8. I'll try and work on number 2 later. I was also able to make the other neccessary changes however you forgot to add in this at the top of the DatabaseServiceProvider.

use App\Repositories\Users\CachingUserRepository;

However now I'm getting

Argument 2 passed to Illuminate\Log\Writer::info() must be of the type array, object given, called in /Users/me/Code/backstagenew/app/Repositories/Users/LoggingUserRepository.php on line 23 and defined

<?php

namespace App\Repositories\Users;

use Illuminate\Contracts\Logging\Log;

class LoggingUserRepository implements UserRepository {

    private $repository;

    private $log;

    public function __construct(UserRepository $repository, Log $log)
    {
        $this->repository = $repository;
        $this->log = $log;
    }

    public function getAll()
    {
        $users = $this->repository->getAll();

        $this->log->info('Users', $users);

        return $users;
    }
}

Change:

$this->log->info('Users', $users);

to:

$this->log->info('Users', $users->toArray());

I'm about to make it as solved by can you tell me why compared to the video on Decorating Repositories theres like 3 changes to mine that Jeffrey Way didn't have to do to get his to work but I do to mine?

Also when I need to change how a method is used for my EloquentUserRepository wouldn't I do it this way.

<?php

namespace App\Repositories\Users;

use App\User;
use App\Repositories\EloquentRepository;

class EloquentUserRepository extends EloquentRepository implements UserRepository
{
    public function __construct(User $model)
    {
        // no need to duplicate the protected attribute initialization
        // as the parent class already does that
        parent::__construct( $model );
    }

    public function getAll()
    {
        return 'testing';  <--------------Just to see what happens but still returns the object user array.
    }
}

@rodrigo.pedra Can you shed some insight?

working on it, wait a sec

Let's go by steps:

If you compare with the video, although the EloquentConversationRepository is named with a Eloquent prefix, it does not use any Eloquent models, for the sake of simplicity, @JeffreyWay returns a plain array.

The problem actually is not by your EloquentUserRepository taking a parameter, if it was a simple binding with no decorators, Laravel's IoC would handle it flawlessly, like so:

//********************************
// DatabaseServiceProvider.php

/* ... omitted for brevity ... */

    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        // DbUserRepository still expects a App\User $model argument
        $this->app->singleton( UserRepository::class, DbUserRepository::class );
    }

/* ... omitted for brevity ... */

//********************************
// routes.php

    $router->get('test', function ( App\Repositories\User\UserRepository $repo ) {
        return $repo->getById(1); // works!
    });

The difference occurs when you starts decorating, as you end not using a class name in the binding but a Closure:

//********************************
// DatabaseServiceProvider.php

/* ... omitted for brevity ... */

    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        // DbUserRepository still expects a App\User $model argument
        $this->app->singleton( UserRepository::class, function () {
            
            // don't work!! Missing argument!
            $eloquentRepo = new EloquentUserRepository; // Laravel's IoC can't reach here

            $cachingRepo = new CachingUserRepository(
                $eloquentRepo, $this->app['cache.store']
            );

            return new LoggingUserRepository($cachingRepo, $this->app['log']);
        } );
    }

/* ... omitted for brevity ... */

//********************************
// routes.php

    $router->get('test', function ( App\Repositories\User\UserRepository $repo ) {
        return $repo->getById(1); // don't work :(
    });

The difference occurs because when binding to the class' name, Laravel's IoC Container can map the class, grab its constructor and map its dependencies, but when binding to a Closure, Laravel IoC Container cannot reach inside its executing block. The Service Provider will bind the interface to whatever the closure returns, but all the bindings inside a closure should be done by the developer.

If you look in the example for deferred providers in the docs [ http://laravel.com/docs/5.0/providers#deferred-providers ], you'll see an example of this approach:

 public function register()
    {
        $this->app->singleton('Riak\Contracts\Connection', function($app)
        {
            return new Connection($app['config']['riak']);
        });
    }

As you see the parameter to the Connection class is passed explicitly by the developer inside the Closure.

tl;dr;

Your use-case is slightly different from the video. The EloquentConversationRepository's constructor or any of its parent's classes constructor does not take any parameters. Your implementation does, and it does correctly because you are handling an actual Eloquent Model, not dummy data.

@rodrigo.pedra

I'll admit that is quite a bit to understand at the moment of where my learning is at this point however it is a GREAT answer.

I'm currently trying to understand what it is I still need to do so that it can get me by so that I can come back to it after I have this section done in my assignment and possibly understand it more and apply it strongly throughout my application.

So you are actually implying I change my EloquentUserRepository constructor?

This is what I have for my current code.

<?php namespace App\Providers;

use App\Repositories\Users\CachingUserRepository;
use App\Repositories\Users\LoggingUserRepository;
use Illuminate\Support\ServiceProvider;
use App\Repositories\Users\UserRepository;
use App\Repositories\Users\EloquentUserRepository;

use App\User;

class DatabaseServiceProvider extends ServiceProvider {

    /**
     * Bootstrap the application services.
     *
     * @return void
     */
    public function boot()
    {
        //
    }

    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        $this->app->singleton(UserRepository::class, function(){
            
            $eloquentRepo = new EloquentUserRepository( new User );

            $cachingRepo = new CachingUserRepository(
                $eloquentRepo, $this->app['cache.store']
            );

            return new LoggingUserRepository($cachingRepo, $this->app['log']);
        });
    }

}

<?php

namespace App\Repositories;

abstract class EloquentRepository {

    protected $model;

    function __construct($model)
    {
        $this->model = $model;
    }

    public function getAll()
    {
        return $this->model->all();
    }

    public function getById($id)
    {
        return $this->model->findOrFail($id);
    }

}

<?php

namespace App\Repositories\Users;

use App\User;
use App\Repositories\EloquentRepository;

class EloquentUserRepository extends EloquentRepository implements UserRepository
{
    public function __construct(User $model)
    {
        // no need to duplicate the protected attribute initialization
        // as the parent class already does that
        parent::__construct( $model );
    }

    public function getAll()
    {
        return 'testing';
    }
}

<?php

namespace App\Repositories\Users;

interface UserRepository
{
    public function getAll();

}

Not really, no need to change anything. I meant that you should not worry about your code ending a bit different from Jeffrey's. The difference is just because your repositories are doing slightly different stuff. Yours is dealing with a real Eloquent Model ans his is dealing with a plain array (despite of the name). And as your repository needs a parameter in the constructor, you need to explicit instantiate it if you're using it inside a Closure.

The key to your "future self" is to better understand how the IoC Container works, one good source is this book: [ https://leanpub.com/laravel ] written by Taylor Otwell.

The Container is very helpful as it handles dependencies "automagically" most of the times. Understand where this "magic" comes from will help you to better understand when you should rely on it and when you should do its work by yourself.

Your code looks fine. Does it still return a User Collection instead of the "testing" string?

@rodrigo.pedra

Well right now for some reason it is giving me the following error. That's because of you asking me to apply that in the LoggingUserRepository. How do I need to fix this now?

What I ultimately want to do is when I make a call to getAll I want it to retrieve all the records that are in the model table now obviously if I don't have it in the EloquentUserRepository then it obviously isn't being overwritten so it could just do the parent EloquentRepository.

FatalErrorException in LoggingUserRepository.php line 23:
Call to a member function toArray() on string

This is now because you are overriding the getAll() method returning the "testing" string. So when the LoggingUserRepositorydecorator calls it, it tries to call the ->toArray() on a string which is not an object.

Good news, your getAll() override is working! Comment out the logging and test the output.

You can keep the logging, of course, just remove the getAll() method from your EloquentUserRepository. Or make it return a collection not a string.

Actually if you change the LoggingUserRepository's line which logs the results to this:

$this->log->info('users', (array)$users);

It will work for both Collections and strings.

@rodrigo.pedra

When I commented out the the logging aspect of it. I get the expected results of "testing" in my browser. AWESOME!

Only thing though is I"m trying to figure out is how I get it to use the parent eloquent model.

<?php

namespace App\Repositories\Users;

use App\User;
use App\Repositories\EloquentRepository;

class EloquentUserRepository extends EloquentRepository implements UserRepository
{
    public function __construct(User $model)
    {
        // no need to duplicate the protected attribute initialization
        // as the parent class already does that
        parent::__construct( $model );
    }

    /*public function getAll()
    {
        return 'testing';
    }*/
}

<?php

namespace App\Repositories;

abstract class EloquentRepository {

    protected $model;

    function __construct($model)
    {
        $this->model = $model;
    }

    public function getAll()
    {
        return $this->model->all();
    }

    public function getById($id)
    {
        return $this->model->findOrFail($id);
    }

}

I am sorry, I did not understand...

Do you mean to use the parent class' methods? You already can do it, they are inherited when you extend the parent class:

$router->get('test', function ( App\Repositories\User\UserRepository $repo ) {
    return $repo->getById(1); // works!
});

$router->get('other', function ( App\Repositories\User\UserRepository $repo ) {
    return $repo->getAll(); // works!
});

You might be wondering why PHPStorm is not auto-completing all the methods, It is because the interface does not list it all. It will work, but the right way to go is to add all the methods you want in the interface:

<?php namespace App\Repositories\Users;

interface UserRepository
{
    public function getAll();
    public function getById($id);
}

If you want to walk the extra mile, add the docblocks, so type-hinting will be your best friend and save your "future self"'s life a lot of times!

@rodrigo.pedra

Well I've done so but the after doing so this arises. Thank you for your time. I'll update again with new code.

ErrorException in Writer.php line 146:
Argument 2 passed to Illuminate\Log\Writer::info() must be of the type array, object given, called in /Users/jeffreydavidson/Code/backstagenew/app/Repositories/Users/LoggingUserRepository.php on line 23 and defined

<?php

namespace App\Repositories\Users;

use Illuminate\Contracts\Logging\Log;

class LoggingUserRepository implements UserRepository {

    private $repository;

    private $log;

    public function __construct(UserRepository $repository, Log $log)
    {
        $this->repository = $repository;
        $this->log = $log;
    }

    public function getAll()
    {
        $users = $this->repository->getAll();

        $this->log->info('Users', $users);

        return $users;
    }

    public function getById($id)
    {
        $user = $this->repository->getById($id);

        $this->log->info('User', $user);

        return $user;
    }
}

@rodrigo.pedra Has done a great job on this with me.

Is there anyone who can assist in finishing me up?

Isn't this working for you?

$this->log->info('users', (array)$users);  // not the cast to array

Doing so should cast a string to array (the case where you overrode the getAll() method to return a string) and also any object that implements the ArrayAccess interface, this is a PHP standard interface, Eloquent models implement that by inheriting from the Illuminate\Database\Eloquent\Model class. [ http://laravel.com/api/5.0/Illuminate/Database/Eloquent/Model.html ] and Laravel Collections do also.

If you know you'll always return a Model or Collection and never a string again, use the approach we used before:

$this->log->info('users', $users->toArray());  // won't work for strings

We moved from that because you overrode the getAll() method to return a string ("testing") instead of an object.

@rodrigo.pedra

The variable users should always return a collection. I was trying to understand why it wasn't accepting an object collection as an accepted value for the log.

Is there something I can do so that it can accept an object?

That's the underlying Monolog\Logger class only accepting an array. If you're always returning a Collection, then it'll work just fine to call the toArray() method on it as described by @rodrigo.pedra.

Okay thanks for the clarification. Not that I doubted @rodrigo.pedra. I just didn't know the reason behind having to do it. Thank you to both @rodrigo.pedra and @bastiaan89.