Rector: [Laravel DI] Create static to constructor injection level

[TomasVotruba]

Source

• https://medium.freecodecamp.org/moving-away-from-magic-or-why-i-dont-want-to-use-laravel-anymore-2ce098c979bd by @mrpixeldream
• https://laravel.com/docs/5.7/facades#facades-vs-dependency-injection
• https://gist.github.com/barryvdh/bb6ffc5d11e0a75dba67
• https://github.com/laravel/framework/blob/e32e92706fe3ba26aef496d23a452750a4653725/src/Illuminate/Foundation/helpers.php#L545

[TomasVotruba]

@mrpixeldream Hi, thanks for awesome post! :+1:

Could you please help me with Request::* to $request->* migration?
I didn't find Request::validate() method anywhere in Laravel code (I don't use Laravel).

Diff with FQN names would be the best

[nschoellhorn]

Thank you very much for using my post as an inspiration for your project. Nice work there!

It's registered as a "macro" in the FoundationServiceProvider: https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Providers/FoundationServiceProvider.php#L50

Maybe that helps you already. If not or if you need anything else, let me know and I'll check tomorrow. Heading to bed now ✌️

[barryvdh]

About the Example in PR #1140; DI can be used on both the constructor and the method itself. Wouldn't it make more sense to add the instances to the method itself (eg. store, index), because otherwise all dependencies need to be resolved for each method?

[TomasVotruba]

@barryvdh I understand. That could be again resolved in standalone Rector.

[TomasVotruba]

It's registered as a "macro" in the FoundationServiceProvider: laravel/framework:src/Illuminate/Foundation/Providers/[email protected]#L50

I would never figure that out, thank you :+1:

Thank you very much for using my post as an inspiration for your project. Nice work there!

It's easy with great input and carefully selected words, great job to you sir.

Heading to bed now

Of course :+1:

[nschoellhorn]

I would never figure that out, thank you 👍

That's basically why I wrote the article, haha. It makes it easy to use but as soon as you try to figure out what's going on, it screws you over.

Wouldn't it make more sense to add the instances to the method itself (eg. store, index), because otherwise all dependencies need to be resolved for each method?

I don't think that it should be added to the controller methods. Putting it in the constructor is just fine, since you return a response from each controller method anyways, so you don't need to duplicate that in all those methods but have it centralized instead. One could even think about extracting an abstract controller class where this is pulled up into since you will most likely even need it in every controller.

But if it's in a separate Rector, the user could decide what to use anyways, so you might do that as well. I don't think it makes much sense though.

[barryvdh]

I think for the factories etc it's probably fine, but the Request argument belongs to the controller method (in my opinion), so perhaps remove it from this one and do the request things all in #1143?

[TomasVotruba]

@mrpixeldream :+1: I have nothing to add.

@barryvdh This is just generic issue to keep the idea. Better chat under the PRs, e.g. #1143, could you help me there?

[nschoellhorn]

Ah I think either @barryvdh or me mixed something up. I agree on keeping the Request in the methods because it's not really a dependency but rather data that flows through the application. The ResponseFactory should go in the constructor though, since it's a real dependency and needed in all methods. I hope that clears it up.

[barryvdh]

Yeah that's wat I meant, the Request facade is a bit different then the rest.

[TomasVotruba]

I need code example to understand you :D

[barryvdh]

So basically a controller, using the Request facade like this:

public function store() {
    $name = \Request::get('name');
}

or this:

public function store() {
    $name = \Illuminate\Support\Facades\Request::get('name');
}

Could be converted to this:

public function store(\Illuminate\Http\Request $request)
{
    $name = $request->get('name');
}

instead of (with the current PR) this:

private $request;

public function __construct(\Illuminate\Http\Request $request)
{
    $this->request = $request;
}
public function store()
{
    $name = $this->request->get('name');
}

[barryvdh]

Also note that Facades are usually aliased also, so a Facade exist is a class_alias version in the root namespace (so \View instead of Illuminate\Support\Facades\View), see https://github.com/laravel/laravel/blob/3001f3c6e232ba7ce2ecdbdfe6e43b4c64ee05ad/config/app.php#L191-L229

[barryvdh]

Although the above issue might be a separate Rector -> Convert all Facades to the FQN (\View to Illuminate\Support\Facades\View), so even if you like Facades, you deal with the actual Laravel classes instead of dynamically generated class_aliases

[nschoellhorn]

Yes, I'd agree with that. Sounds perfectly reasonable.

[TomasVotruba]

So there are actually 3 Request classes? Wtf?

[barryvdh]

No, there is 1 Request class. The Request facade (Illuminate\Support\Facades\Request) is just a service locator, like all facades. So all static calls to any Facade are passed to the actual instance. See https://github.com/laravel/framework/blob/5.8/src/Illuminate/Support/Facades/Facade.php and https://github.com/laravel/framework/blob/5.8/src/Illuminate/Support/Facades/Request.php (the last one is only defining which object it refers to, request in this case)

So Illuminate\Support\Facades\Request::get() is basically app('request')->get().

And because before Laravel 5.0, namespaces weren't used in many apps, they created aliases in the root namespace, using class_alias. So Request is just a class_alias for the Request facade. (See https://github.com/laravel/framework/blob/d0a33d4a398f2bd23c2689aac473a3453ece76cc/src/Illuminate/Foundation/AliasLoader.php#L65-L82 )

[barryvdh]

Oh and because now there are namespaces, there is also a request() helper https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/helpers.php#L676-L698 ;)

[TomasVotruba]

I'm sorry, I got lost in this :sob:

Could you propose PR to fix what you talk about?

[azdanov]

I wonder if some combination of FunctionReplaceRector and FacadeStaticCallToConstructorInjectionRector/RequestStaticValidateToInjectRector can help with global function helpers, such as request().

Before:

<?php

namespace App\Http\Controllers;

class WelcomeController extends Controller
{
    public function index()
    {
        return view('welcome', [
            'foo' => request()->get('foo'),
        ]);
    }
}

After:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;

class WelcomeController extends Controller
{
    public function index(Request $request)
    {
        return view('welcome', [
            'foo' => $request->get('foo'),
        ]);
    }
}

[TomasVotruba]

It can. Please create a new issue, this one is mixture of too many.

Closing as PR for initial issue was merged.