auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

5.x with attribute injection #210

Closed frederikbosch closed 5 months ago

frederikbosch commented 5 months ago
use Aura\Di\Attribute\Instance;
use Aura\Di\Attribute\Service;
use Aura\Di\Attribute\Value;

class FakeConstructAttributeClass
{
    private FakeInterface $fakeSetter;

    public function __construct(
        #[Service('fake.service')]
        private FakeInterface $fakeService,
        #[Service('fake.service', 'getFoo')]
        private FakeInterface $fakeServiceGet,
        #[Instance(FakeInterfaceClass1::class)]
        private FakeInterface $fakeInstance,
        #[Value('fake.value')]
        private string $string,
    ) {
    }
}

Design choices:

Limitations:

I will continue with the implementation if there is no objection towards this direction. I will take at least a week before continuing.

pmjones commented 5 months ago

@frederikbosch I think @koriym has done something like this with https://github.com/bearsunday/BEAR.Sunday -- if you have not asked him already, he may have insight/warnings/recommendations here.

frederikbosch commented 5 months ago

@pmjones Thanks for brining this to my attention. I found the manual of Ray.Di.

After having a look at that packages, I think I am going to add the possibility for client users to create their own attributes. They have upfront knowledge of their own codebase, and hence they could create specific injections.

use MyApp\Config\Attribute\Config;

class ApiConnection
{
    public function __construct(
        #[Config('api.key')]
        private string $key,
    ) {
    }
}

Then the attribute would result in the following call:

/** @var MyApp\Config\Bag $config */
$config = $di->get('config');
$config->get('api.key');

Basically, in this case, it creates an advanced ->lazyGetCall().

Also, I could consider setter injection in a similar manner as Ray.Di. First, annotate the method with #[Inject], then use the parameter attributes to indicate what to inject in those setter methods.

Finally, I don't think the attributes have to implemented feature complete. When we would release a 5.0, there is always a possibility for adding features in a 5.1 release.

koriym commented 5 months ago

@frederikbosch First of all, let me tell you that Ray.Di and its manual is a clone of Google Guice with almost all of its features. My few design decisions are limited to removing property injection, which was also controversial in Guice.

I think I am going to add the possibility for client users to create their own attributes.

This is a very good idea! Creating a custom class is more self explanatory and you can make the custom class the place of the documentation; even Guice recommends custom annotations over @Named strings.

frederikbosch commented 5 months ago

Thanks for your input @koriym. It confirms I am taking the right direction: keep the included attributes easy and small. People can always create their own attributes for more specific use cases.

And for the naming of the attributes, I try to stay close to terminology that has always been used in this package. I thought of using #[Get] instead #[Service], because you define a lazyGet in Aura.Di terminology. However, I find #[Service] vs #[Instance] vs #[Value] more clear than #[Get] vs #[Instance] vs #[Value].

frederikbosch commented 5 months ago

Next to the attributes, there have been four major changes.

  1. The Resolver now not only contains the params, types and values, but also the services. The services have been moved from the Container to the Resolver.

  2. The LazyInterface now expects a Resolver parameter when __invoke gets called. This means that the Resolver or even Container does not need to be injected of implementations of LazyInterface. This has the advantage that an attribute just returns a new instance of such an implementation when the inject method is called. Moreover, this removes many circular references when serializing the container.

  3. When constructing a Container directly, without using the ContainerBuilder, now requires a Resolver, not an InjectionFactory. This was a result of a cleanup of the first two changes.

// old code
new Container(new InjectionFactory(new Resolver(new Reflector())));

// new code
new Container(new Resolver(new Reflector()));
  1. PHP 7 dropped
frederikbosch commented 5 months ago

I would suggest to accept this code and set 5.x as the default branch. Then I will continue with the other plans I have created for 5.x.

harikt commented 5 months ago

Don't we need to change the branch to 5.x ?

harikt commented 5 months ago

I think this PR is just demonstrating the changes in 5.x to 4.x . So no need for merge. Regarding review, I think @koriym is the best person for this. In case if I have any thoughts I will write here.

Thanks again for your wonderful work.

koriym commented 5 months ago

LGTM. (I think it is good that you only support constructor injection for a start.)

However, it seems that some phpdoc changes have not kept up with the code changes - why not check the IDE or use psalm/phpstan to make corrections?

Thanks for your wonderful work.

koriym commented 5 months ago

I think this PR is just demonstrating the changes in 5.x to 4.x .

I thought so too. Why not "draft pull request" then?

frederikbosch commented 5 months ago

Good idea to include phpstan, will do that and fix the docblocks.

koriym commented 5 months ago

We dropped PHP 7.x, so we don't need yoast/phpunit-polyfills any more? (Or is OK to leave it?)

frederikbosch commented 5 months ago

Great idea to remove it, see #217

frederikbosch commented 5 months ago

Thanks for your review, input and suggestions @koriym, @pmjones and @harikt. I think the core is ready. I will try to implement the final feature (container compilation) I had in mind next week.