Larastan: Rule for checking container bindings

[canvural]

With the Octane release, there was a suggestion from @olivernybroe if we can check the container bindings for Octane compatibility.

In this issue, I want to discuss and gather some information about what can we do in Larastan.

Container bindings

From the Octane docs:

In general, you should avoid injecting the application service container or HTTP request instance into the constructors of other objects.

And the recommended solution is to either don't use singleton binding or pass a closure instead of the app instance. The same thing applies to request and config injections.

This can be a rule I think. The hardest thing is here to avoid false positives. So we need a lot of examples that we can test on.

disclaimer: I'm not entriley sure if this rule can be written. Just brainstorming at this point.

[canvural]

Ok I have a simple version running now. Basically, the checks that are done are:

• Rule is based on the MethodCall node. So runs for every method call. First thing checked is if the called method name is singleton
• Then we check if this singleton method is called on Illuminate\Contracts\Container\Container\Application
• Then we check if the second argument is a closure. singleton can accept Closure|string|null
• Then we check the closure's arguments. It should at least have one argument. That is the Application instance. We make a note of the argument variable name. We will use it later.
• Then we check the body of the closure for new ANY_CLASS_NAME($app) We also make sure that the application instance passed to the constructor by comparing the variable names from the previous step.
• If we found anything, we report the error.

So, some questions are:

• Should we also check the namespace we are in, and possibly restrict the check if we are in App\Providers namespace for example?
• What are the things to check other than constructors in the closure?

cc @olivernybroe, @nunomaduro

[olivernybroe]

@canvural let's not check namespace, as Laravel package creators will not have that namespace for this service provider.
We could check if it's inside a class which extends service provider class?

[canvural]

@olivernybroe I thought about it more, and now I'm not sure if those checks are necessary. People can call $app->singleton anywhere, not just the service provider (even though they should do there) And I don't see a downside of not checking if it called in service provider.

But what I'm confused about is that in Twitter @themsaid was saying we should use the $app passed into the closure instead of $this->app But docs are saying even using the $app passed into the closure is wrong. fn () => Container::getInstance() or similar should be used. Which one is correct? :sweat_smile:

[nunomaduro]

@canvural Mohamed told me that we can take a look at https://divinglaravel.com/laravel-octane-bootstrapping-the-application-and-handling-requests > Things To Consider.