Skip to content

Instantly share code, notes, and snippets.

@vladyslavstartsev
Last active January 16, 2019 16:44
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save vladyslavstartsev/498e3e5ba92b4f44c69bc8671de99a3c to your computer and use it in GitHub Desktop.
Save vladyslavstartsev/498e3e5ba92b4f44c69bc8671de99a3c to your computer and use it in GitHub Desktop.
laravel facades discussion

Found an example there is a thing in laravel called FormRequest. Sometimes, I need extra validation. So I need to use some service there somehow. What options I have

  1. facade
  2. service locator via app() global function
  3. I also have instance of container there via $this->container

which one is the best here? I suppose instance of container

I can't do constructor injection. Why? Because for me, it seams that there are too many arguments on the __construct method it look like this image

Am I wrong with this one?

Also they (FormRequests) help me to make controllers slim. I get all the extra data there (for example get user by email, I mean not the current session user, but the one that I, for example, edit) so I don't need to do this in other place.

what I also can do is to make some separate method, name it, for example, afterValidationRequestDataSetter, on the form request, call it at the end of all operations like $this->container->call([$this, 'afterValidationRequestDataSetter']) and it will inject all the dependencies that I need (I suppose, have not tested yet), but that

  1. is not constructor injection

  2. looks a bit like a magic since you can't do search on who calls this method (especially, it will be hard for new people on the project)

what is your ides about that?

p.s. not laravel zombie-fan and not forcing people to use facades

@Ocramius
Copy link

Sometimes, I need extra validation. So I need to use some service there somehow.

Inject the service needed for said validation.

What options I have

  1. facade
  2. service locator via app() global function
  3. I also have instance of container there via $this->container

which one is the best here? I suppose instance of container

None of these is viable: if you need SomeValidator, it should be explicitly declared by the public API of the unit that you are working with, either as constructor argument or additional method argument.

I can't do constructor injection. Why? Because for me, it seams that there are too many arguments on the __construct method it look like this

Yes you can do constructor injection: avoiding constructor injection to then add runtime service lookups is just hiding complexity, and building up false confidence. Each runtime lookup is a late failure, a hard-to-debug endpoint, and added hidden complexity. If it is complex, then you can "divide et impera" into smaller units, but do not hide dependencies for the sake of it.

Am I wrong with this one?

Yes: if it is complex, it is complex. If you add service location at runtime, you are not just having just complexity, but also runtime chaos (chaos is another degree worse than complex).

Also they (FormRequests) help me to make controllers slim. I get all the extra data there (for example get user by email, I mean not the current session user, but the one that I, for example, edit) so I don't need to do this in other place.

A request is not a service, so it shouldn't retain runtime services in its internals: it should be built by either a factory (which has all required services), or by a named constructor (which is just a simpler factory) that receives the required services (including validators) at runtime.

what I also can do is to make some separate method, name it, for example, afterValidationRequestDataSetter, on the form request, call it at the end of all operations like $this->container->call([$this, 'afterValidationRequestDataSetter']) and it will inject all the dependencies that I need (I suppose, have not tested yet), but that

  1. is not constructor injection
  2. looks a bit like a magic since you can't do search on who calls this method (especially, it will be hard for new people on the project)
    what is your ides about that?

Argh, please don't do any of that. You are introducing multiple problems:

  1. object ($this, I suppose) now has invalid state until afterValidationRequestDataSetter. Objects should always be in a valid state, because you cannot predict if any of their public API will be hit between state mutations.
  2. object relies on a service locator
  3. object relies on a stringly-typed API (you are passing a random name to the container call)
  4. you are running a simple method call through a complex framework
  5. lots of runtime overhead
  6. extremely tricky to debug

@vladyslavstartsev
Copy link
Author

<?php

declare(strict_types=1);

namespace App\Api\Http\Requests;

use App\Doctrine\Entity\UserApiSetting;
use App\Doctrine\Repositories\UserApiSettingRepository;
use Illuminate\Contracts\Validation\Validator;
use Illuminate\Foundation\Http\FormRequest;
use LaravelDoctrine\ORM\Facades\EntityManager;

class ApiGetUserSettingsRequest extends FormRequest
{
    public function authorize(): bool
    {
        return true;
    }

    public function rules(): array
    {
        return [
            'limit' => 'integer|min:1|max:10000',
            'offset' => 'integer|min:0|max:10000',
        ];
    }

    protected function getValidatorInstance(): Validator
    {
        // yes Doctrine used here 
        $this->userId = EntityManager::getRepository(UserApiSetting::class)
            ->findOneBy(['apiKey' => $this->header('X-TXC-APIKEY')])
            ->getUser()
            ->getId();
        // I also can do this like, see https://www.laraveldoctrine.org/docs/1.3/orm/repositories 
        $this->userId = app(UserApiSettingRepository::class)
            ->findOneBy(['apiKey' => $this->header('X-TXC-APIKEY')])
            ->getUser()
            ->getId();
        // setting default values and typecasting offset/limit to int so I don't need to do this in controllers
        $this->offset = $this->offset ? (int) $this->offset : 0;
        $this->limit = $this->limit ? (int) $this->limit : 50;
        $this->someExtraValidationService = app(\App\SomeExtraValidationService::class);

        return parent::getValidatorInstance();
    }

    public function withValidator(Validator $validator): void
    {
        $validator->after(function (Validator $validator): void {
            if ($this->someExtraValidationService->validateUserId($this->userId)) {
                $validator->errors()->add(
                    'form',
                    'the userId field is not valid'
                );
            }
        });
    }
}

something like this I have in my code

@vladyslavstartsev
Copy link
Author

vladyslavstartsev commented Dec 30, 2018

Also need to mention about views and service providers (see https://laravel.com/docs/5.7/providers)

so I shouldn't use boot method on \Illuminate\Support\ServiceProvider also (it work similar to what I was trying to do with afterValidationRequestDataSetter) ? If yes, how should I inject dependencies then into service provider?

because I can't inject anything into constructor because it resolves it like this https://github.com/laravel/framework/blob/5.7/src/Illuminate/Foundation/Application.php#L636-L639

also sometimes I need services in my views (it was layouts) there are 2 ways to do that

  1. https://laravel.com/docs/5.7/blade#service-injection which basically calls app() helper function which is bad since service locator is used and in view I only need the end data and even no way to change things via service
  2. https://laravel.com/docs/5.7/views#view-composers which injects something into view (layout) but it involves some magic (at least for me) inside

which one is better?

also is that ok to inject \Illuminate\Foundation\Application (which is also a container instance at the same time) into my services?

@deleugpn
Copy link

deleugpn commented Jan 1, 2019

@vladyslavstartsev just to confirm I understand the problem: you want to extract a complex validation rule out of the Form Request, correct?

Laravel real time Facades are GREAT for the use-case you described. They are straight-forward and easy to mentally parse when reading somebody else's code. Any developer with basic understand of Laravel will know your typed Form Request Object will run it's rules method and Facades will allow you to run a process with a single line while offering DI, automatic object instantiation and mockery.

That being said, Laravel is smart enough to know that some people will cry a river over the wonders that Facades brings and for the Form Request case that was resolved by offering automatic resolution at the rules method. You can simply type-hint your dependency as an argument of the rules method and your object will be injected for your. It works almost the same way as Facades, except that it's a typed argument instead. Combined with Rule object, you can do wonders.

public function rules(ValidMysqlServerRule $mysql)
{
    return [
        'host' => $mysql
    ]
}

Then define your rule object

use App\Components\Database\ConnectionFactory;
use Illuminate\Contracts\Validation\Rule;
use PDOException;

class ValidMysqlServerRule implements Rule
{
    private $connectionFactory;

    public function __construct(ConnectionFactory $connectionFactory)
    {
        $this->connectionFactory = $connectionFactory;
    }

    public function passes($attribute, $value): bool
    {
        try {
            $this->connectionFactory->makeWithDefaultCredentials($value)->getPdo();
        } catch (PDOException $e) {
            return false;
        }

        return true;
    }

    public function message()
    {
        return 'The :attribute is not known.';
    }
}

@vladyslavstartsev
Copy link
Author

@deleugpn Hi! Thanks for your message

you want to extract a complex validation rule out of the Form Request, correct?

Sometimes yes, but sometimes

Also they (FormRequests) help me to make controllers slim. I get all the extra data there

see getValidatorInstance method

I do need validation, but only one time, I mean I don't want to create separate class for it

Laravel real time Facades are GREAT

Sorry, but real time Facades, as I think, is the worst thing ever happened. You can catch all types of run time exceptions with no idea on how to debug it.

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