I'm wondering if there's something wrong in this method of handling dependency injection.
At the end of the post there's the real question. But before, I need to explain from where that question comes from.
I have a ValidationContract Interface which provides the directives for all the validators (I'm trying to adhere as much as I can to SOLID)
<?php
namespace Gms\Contracts;
interface ValidatorContract
{
/**
* @param array $item
*
* @return mixed
* @internal param array $branch
*
*/
public function validateCreation(array $item);
/**
* @param array $item
* @param $id
*
* @return mixed
*/
public function validateUpdate(array $item, $id);
}
Then I have several specific validators:
<?php
/**
* Created by PhpStorm.
* User: caiuscitiriga
* Date: 31/08/16
* Time: 16:20
*/
namespace Gms\Validators;
use Gms\Repositories\BankRepository;
use Illuminate\Support\Facades\DB;
use Gms\Contracts\ValidatorContract;
/**
* Class BankValidator
* @package Gms\Validators
*/
class BankValidator implements ValidatorContract
{
/**
* @var BankRepository
*/
private $bankRepository;
public function __construct(BankRepository $bankRepository)
{
$this->bankRepository = $bankRepository;
}
/**
* @param array $bank
*
* @return mixed
*/
public function validateCreation(array $bank)
{
if ($this->missingRagioneSociale($bank)) {
return [FALSE, 'missing ragione sociale'];
}
if ($this->ragioneSocialeAsEmptyString($bank)) {
return [FALSE, 'can\'t leave ragione_sociale empty'];
}
if ($this->missingAbi($bank)) {
return [FALSE, 'missing abi'];
}
if ($this->invalidAbiForCreation($bank)) {
return [FALSE, 'invalid abi, already taken'];
}
return [TRUE, ''];
}
/**
* @param array $bank
* @param $id
*
* @return mixed
*/
public function validateUpdate(array $bank, $id)
{
if ($this->isUpdatingAbi($bank)) {
if (!$this->hasValidAbiForUpdate($bank['abi'], $id)) {
return [FALSE, 'invalid abi, already taken'];
}
}
if ($this->isUpdatingRagioneSociale($bank)) {
if ($this->ragioneSocialeAsEmptyString($bank)) {
return [FALSE, 'can\'t leave ragione_sociale empty'];
}
}
return [TRUE, ''];
}
/**
* @param $abi
*
* @return bool
*/
protected function hasValidAbiForCreation($abi)
{
$results = $this->bankRepository->getBankByAbi($abi);
if (!$results) {
return TRUE;
}
return FALSE;
}
/**
* @param $abi
* @param $id
*
* @return bool
*/
protected function hasValidAbiForUpdate($abi, $id)
{
$results = $this->bankRepository->getBankByAbiSkippingGivenID($abi, $id);
if (!count($results)) {
return TRUE;
}
return FALSE;
}
/**
* @param array $bank
*
* @return bool
*/
private function missingRagioneSociale(array $bank)
{
return !array_has($bank, 'ragione_sociale');
}
/**
* @param array $bank
*
* @return bool
*/
private function missingAbi(array $bank)
{
return !$this->isUpdatingAbi($bank);
}
/**
* @param array $bank
*
* @return bool
*/
private function invalidAbiForCreation(array $bank)
{
return !$this->hasValidAbiForCreation($bank['abi']);
}
/**
* @param array $bank
*
* @return bool
*/
private function ragioneSocialeAsEmptyString(array $bank)
{
return $bank["ragione_sociale"] == "";
}
/**
* @param array $bank
*
* @return bool
*/
private function isUpdatingAbi(array $bank)
{
return array_has($bank, 'abi');
}
/**
* @param array $bank
*
* @return bool
*/
private function isUpdatingRagioneSociale(array $bank)
{
return array_has($bank, "ragione_sociale");
}
}
How we can see from the constructor, this class needs a dependency, which is the BankRepository. I'm using a repository because I've read that it isn't suggested to directly use the Eloquent models.
Also the Repositories has their own contract.
<?php
namespace Gms\Contracts;
interface RepositoryContract
{
/**
* @return mixed
*/
public function getAll();
/**
* @param $id
*
* @return mixed
*/
public function find($id);
/**
* @param $id
*
* @return mixed
*/
public function destroy($id);
/**
* @param $id
* @param $data
*
* @return mixed
*/
public function update($id, array $data);
/**
* @param array $array The data to use for the creation
*
* @return mixed
*/
public function create(array $array);
}
Then the BankRepository adheres to this contract with this implementation
namespace Gms\Repositories;
use App\Gms\Models\Bank;
use Gms\Contracts\RepositoryContract;
class BankRepository implements RepositoryContract
{
public function getAll()
{
return Bank::all();
}
public function find($id)
{
return Bank::find($id);
}
public function destroy($id)
{
return Bank::destroy($id);
}
public function update($id, array $data)
{
return Bank::where('id', $id)->update($data);
}
public function create(array $data)
{
return Bank::create($data);
}
public function getBankByAbi($abi)
{
return Bank::where('abi', $abi)->get()->first();
}
public function getBankByAbiSkippingGivenID($abi, $id)
{
return Bank::where('abi', $abi)
->where('id', '!=', $id)
->get();
}
}
Back to our BankValidator, I need a quick way to manage it's dependencies, so I'm using a ServiceProvider, which registers the BankValidator to the Service Container.
<?php
namespace Gms\Providers\Validators;
use Gms\Repositories\BankRepository;
use Gms\Validators\BankValidator;
use Illuminate\Support\ServiceProvider;
class BankValidatorSP extends ServiceProvider
{
/**
* Bootstrap the application services.
*
* @return void
*/
public function boot()
{
//
}
/**
* Register the application services.
*
* @return void
*/
public function register()
{
$this->app->bind('Gms\Validators\BankValidator', function(){
return new BankValidator(new BankRepository());
});
}
}
Then, since I have multiple controllers which in the constructor wants something that adheres at the ValidatorContract no matter what, I need to register for each controller the proper Validator. For example, the BankController it will use the BankValidator. But its constructor requires only the Contract. I've seen this on laracasts.com
class BankController extends Controller
{
/**
* @var BankConverter
*/
protected $converter;
/**
* @var BankValidator
*/
protected $validator;
/**
* @var Repository
*/
private $repository;
/**
* BankController constructor.
*
* @param ConverterContract|BankConverter $converter
* @param ValidatorContract|BankValidator $validator
* @param RepositoryContract|BankRepository $repository
* @param BankResponse|ResponseContract $response
*/
public function __construct(
ConverterContract $converter,
ValidatorContract $validator,
RepositoryContract $repository,
ResponseContract $response
)
{
$this->converter = $converter;
$this->validator = $validator;
$this->repository = $repository;
$this->response = $response;
}
As you can see the constructor requires even other Contracts, like the Converter, the Response and the Repository. Since each controller uses the same "structure", I've found useful to create a Service Provider for each one of these contracts.
So I have a ValidatorServiceProvider, a ConverterServiceProvider, a ResponseServiceProvider, and a RepositoryServiceProvider.
Just to stay inline with the Validator I list below the implementation of ValidatorServiceProvider. You will find some snippets that returns a new class injecting the dependencies and some snippets that resolves the classes from the Service Container. This is because this question pops into my mind when I was refactoring all this stuff. Moving each "manual" injection of the dependencies into a single service provider and then use that. To prevent code replication.
<?php
namespace Gms\Providers;
use App\Http\Controllers\BankController;
use App\Http\Controllers\BranchController;
use App\Http\Controllers\ClauseController;
use App\Http\Controllers\ContractController;
use App\Http\Controllers\CustomerController;
use App\Http\Controllers\ServiceController;
use App\Http\Controllers\SupplierController;
use App\Http\Controllers\TagController;
use Gms\Contracts\ValidatorContract;
use Gms\Repositories\BankRepository;
use Gms\Repositories\BranchRepository;
use Gms\Repositories\ClauseRepository;
use Gms\Repositories\ContractRepository;
use Gms\Repositories\CustomerRepository;
use Gms\Repositories\TagRepository;
use Gms\Validators\BankValidator;
use Gms\Validators\BranchValidator;
use Gms\Validators\ClauseValidator;
use Gms\Validators\ContractValidator;
use Gms\Validators\CustomerValidator;
use Gms\Validators\ServiceValidator;
use Gms\Validators\SupplierValidator;
use Gms\Validators\TagValidator;
use Illuminate\Support\ServiceProvider;
class ValidatorServiceProvider extends ServiceProvider
{
/**
* Bootstrap the application services.
*
* @return void
*/
public function boot()
{
//
}
/**
* Register the application services.
*
* @return void
*/
public function register()
{
// Register the RESPONSE CLASS for the Bank controller
$this->app->when(BankController::class)
->needs(ValidatorContract::class)
->give(function () {
return $this->app->make(BankValidator::class);
});
// Register the RESPONSE CLASS for the Branch controller
$this->app->when(BranchController::class)
->needs(ValidatorContract::class)
->give(function () {
return new BranchValidator(new BankRepository(), new BranchRepository());
});
// Register the RESPONSE CLASS for the Customer controller
$this->app->when(CustomerController::class)
->needs(ValidatorContract::class)
->give(function () {
return new CustomerValidator(new BranchRepository());
});
// Register the RESPONSE CLASS for the Contract controller
$this->app->when(ContractController::class)
->needs(ValidatorContract::class)
->give(function () {
return new ContractValidator(new CustomerRepository(), new ContractRepository());
});
// Register the RESPONSE CLASS for the Supplier controller
$this->app->when(SupplierController::class)
->needs(ValidatorContract::class)
->give(function () {
return new SupplierValidator(new BranchRepository());
});
// Register the RESPONSE CLASS for the Tag controller
$this->app->when(TagController::class)
->needs(ValidatorContract::class)
->give(function () {
return $this->app->make(TagValidator::class);
});
// Register the RESPONSE CLASS for the Service controller
$this->app->when(ServiceController::class)
->needs(ValidatorContract::class)
->give(function () {
return new ServiceValidator();
});
// Register the RESPONSE CLASS for the Clause controller
$this->app->when(ClauseController::class)
->needs(ValidatorContract::class)
->give(function () {
return $this->app->make(ClauseValidator::class);
});
}
}
And finally we come to the Registration itself in the config/app.php file.
//some other code before..
/*
|----------------------------------
| GMS Service Providers
|----------------------------------
*/
// Load first the validators
Gms\Providers\Validators\ClauseValidatorSP::class,
Gms\Providers\Validators\BankValidatorSP::class,
// Load the services dispatchers
Gms\Providers\ResponseServiceProvider::class,
Gms\Providers\ConverterServiceProvider::class,
Gms\Providers\ValidatorServiceProvider::class,
Gms\Providers\RepositoryServiceProvider::class,
// CORS
Barryvdh\Cors\ServiceProvider::class,
],
Is this a good practice?
This question obviously leads to these other questions:
Are there any potential risks with this approach?
Will I fall into a pit where one service will be used before it is actually registered?
Isn't the "structure" maybe too much "fractionated"?
This feels a lot like how I had one of my early Laravel projects setup. After a while, I realized what I was doing was making my app flexible in places where it doesn't need to be flexible at the cost of making it needlessly complex. And sorry if this seems really nitpicky, I thought more detail is better then less detail.
Your repository feels like it's making things needlessly complex by just adding another layer between your controller and your model (which is actually a good thing) but at the same time, offering no additional functionality (which makes this a bad thing). I'm not aware of the context behind the advice that models shouldn't be called directly but I have a feeling it was misconstrued.
The way I see the repository pattern being used in Laravel is what is usually called a service layer. It's where all the business logic goes. Let's say for example when a new user is created, you need to text that user an activation code, and also attach a role to that user. You don't want to handle that all in your controller because having light controllers is good practice. So what you do is create a repository to handle that logic. In that repository, you would inject the User model then $this->user->create([$userParams]) (because I think this is what was meant by not using the model directly, you should inject it), and you would also likely inject whatever texting implementation you wrote, and use that to text. Then you inject your role model into that repository and use it to grab the role for your user and attach that to your new user. What you end up with is a repository that actually does stuff (instead of acting as a useless layer between controller and model), you aren't writing a bunch of functions which call the same function name on your model and your controllers are kept light.
Outside the confines of Laravel, I would say this is a good approach. If we are using Laravel though, we should leverage it because I think it offers some specialized tools which already handle what you are trying to do.
The first is I think it could greatly benefit by using Laravel's Validator class and Form Request objects.
The second, and maybe this can be overlooked beacuse I'm not sure of the reasons why they were setup this way, but it feels like each validator class is being forced into an implementation for the sake of being an implementation. It feels like they are somewhat unrelated to each other and I can't see them ever being swapped out for each other because they are working on different entities. What I mean by this is taking the last exmaple's texting service. You'd have a texting interface which contains the methods that your repositories are calling on that service. Then you would have different implementations for that interface, for example maybe a Twilio implementation. Then if you ever needed to use something besides Twilio, you'd just write a new class which implements your texting interface and tell Laravel to grab that class instead of the Twilio one whenever it needs a texting implementation and your done. You don't need to change any additional code.
I kind of touched on this already but I think it can use further clarification.
It looks like where you are using ValidatorContract for flexibility, you are actually making your app more rigid. For example, in your service provider, you are saying where your BankController needs ValidatorContract, then give it BankValidator. You should ask yourself "Will BankController ever need anything besides BankValidator for validation? Will I ever need to inject CustomerValidator into BankController?". If the answer is no, then I think you are wasting your time trying to worry about service providers and making sure they all share the same interface for this case. There's nothing wrong with just injecting BankValidator into BankController, CustomerValidator into CustomerController etc... Don't get me wrong, it's amazing really that they all share the same methods because it makes it easy on you as a developer because you don't have to remember what the different method names were for each validator. However that type of thing shouldn't need to be enforced by use of interfaces.
I don't know what the reasoning for having a bank validator is but here is an example of what I would consider a better use of contracts. Assuming different banks need different validations, it would make more sense if BankValidator was actually a contract which contains the methods BankController is trying to call on it. Then you could have different implementations of that, for example ComericaBankValidator implements BankValidator. Then when someone from comerica logs in, you'd then bind ComericaBankValidator to BankValidator. I'm sure this example is non-sensical from the context of what you are working on but hopefully it still makese sense out of context and you can see how much more flexible your app could be with this small change.