Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Add dependency resolver attributes #49640

Closed

Conversation

Neol3108
Copy link
Contributor

Premise of this PR

This PR adds the ability to have an implementation decide where to get dependencies from without explicit binding.

I often use the container for container to resolve services/actions etc in my code to have IoC but
find e.g. giving config values to those classes cumbersome, especially if all other dependencies
can be resolved by the container itself except one (api key for example).

An alternative would be contextual binding, but I personally don't like having references to parameters
that my IDE doesn't understand. Renaming it would probably break the code unless you search for it.
Of course having automated tests would prevent mistakes like that to a certain extend, but I often
mock my dependencies instead of letting them resolve.

This issue was addressed by #46530 but very specific to config.
This PR attempts to provide a more generic solution.

Examples

Before 1:

<?php

namespace App\Providers;

use App\ApiClient;
use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    public function register(): void
    {
        $this->app->bind(ApiClient::class, function () {
            return new ApiClient(
                key: config('services.api.key'),
            );
        });
    }
}
<?php

namespace App;

class ApiClient
{
    public function __construct(
        private readonly string $key,
    ) {
        //
    }
}

Before 2:

<?php

namespace App\Providers;

use App\ApiClient;
use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    public function register(): void
    {
        $this->app->when(ApiClient::class)
            ->needs('$key')
            ->giveConfig('services.api.key');
    }
}
<?php

namespace App;

class ApiClient
{
    public function __construct(
        private readonly string $key,
    ) {
        //
    }
}

After:
No binding needed

<?php

namespace App;

use Illuminate\Config\Attributes\InjectFromConfig;

class ApiClient
{
    public function __construct(
        #[InjectFromConfig('services.api.key')]
        private readonly string $key,
    ) {
        //
    }
}

Benchmarks

My main concern with adding this would be performance due to the usage of reflection, this was
actually not that bad in my opinion. Here are a few test cases, feel free to suggest more scenarios.
Tested on a 2023 M2 MacBook Air 16GB

Test 1:

$container = new Container();

Benchmark::dd(function () use ($container) {
    $container->make(ContainerConcreteStub::class);
}, 100);

Before: 0.001ms
After: 0.002ms

Test 2:

$container = new Container();

Benchmark::dd(function () use ($container) {
    $container->make(ContainerDefaultValueStub::class);
}, 100);

Before: 0.004ms
After: 0.005ms

Test 3:

$container = new Container();

$container->bind(IContainerContractStub::class, ContainerImplementationStub::class);

Benchmark::dd(function () use ($container) {
    $container->make(ContainerNestedDependentStub::class);
}, 100);

Before: 0.007ms
After: 0.007ms

Breaking changes

As far as I can tell this does not break existing code.

Final thoughts

The performance was my main concern like I pointed out earlier. I however also don't know if this is
"too much" for the container, I have always seen the container as fairly straight forward and I think
this feature might lean more toward the "syntactic sugar"-side.

It might be worth it to add a setting to enable or disable this for people who which to avoid the
performance penalty caused by reflection. This will probably have to be set quite early in the
framework lifecycle to guaranty consistent results, not too sure.

Feel free to give feedback. Thanks

@driesvints
Copy link
Member

Please note that draft PR's aren't reviewed, thanks.

@Neol3108 Neol3108 force-pushed the dependency-resolver-container branch from 8b08312 to 3ada9ac Compare January 13, 2024 13:06
@Neol3108 Neol3108 marked this pull request as ready for review January 13, 2024 13:21
@Neol3108 Neol3108 changed the title [10.x] [POC] Add dependency resolver attributes [10.x] Add dependency resolver attributes Jan 13, 2024
@taylorotwell
Copy link
Member

Don't totally hate the idea, but not sure I want to take it on right now. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants