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

5.x with attribute injection #210

Closed
wants to merge 23 commits into from
Closed

5.x with attribute injection #210

wants to merge 23 commits into from

Conversation

frederikbosch
Copy link
Contributor

@frederikbosch frederikbosch commented May 14, 2024

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:

  • All attributes define lazy injections
  • Attributes will have the lowest precedence, e.g. ->params[FakeConstructAttributeClass::class]['fakeService'] = ... will overwrite the attribute.

Limitations:

  • You cannot pass params to #[Instance] attributes. If you want to overwrite constructor parameters of an injected instance, you would have to change to an #[Service] attribute or reside to inject by code.
$container->params[FakeConstructAttributeClass::class]['fakeService'] = $container->lazyNew(MyClass, [
    'param1' => $container->lazyGet('service')
]);
  • You cannot pass params to #[Service('service', 'method')] attributes. You would have to reside to the method using code, or define an additional service and then use #[Service('new.service)].
$container->set('new.service', $container->lazyGetCall('service', 'method', $container->lazyNew(OtherClass::class))

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

@pmjones
Copy link
Member

pmjones commented May 14, 2024

@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.

@harikt harikt requested a review from koriym May 15, 2024 04:20
@frederikbosch
Copy link
Contributor Author

frederikbosch commented May 15, 2024

@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
Copy link
Member

koriym commented May 15, 2024

@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
Copy link
Contributor Author

frederikbosch commented May 15, 2024

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
Copy link
Contributor Author

frederikbosch commented May 15, 2024

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
Copy link
Contributor Author

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.

@frederikbosch frederikbosch requested a review from harikt May 15, 2024 10:06
@frederikbosch frederikbosch mentioned this pull request May 15, 2024
7 tasks
@harikt
Copy link
Member

harikt commented May 16, 2024

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

@harikt
Copy link
Member

harikt commented May 16, 2024

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
Copy link
Member

koriym commented May 16, 2024

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.

@@ -47,10 +49,10 @@ class LazyGet implements LazyInterface
* @param string $service The service to retrieve.
*
*/
public function __construct(ContainerInterface $container, $service)
public function __construct(string $service, ?ContainerInterface $delegatedContainer = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPDoc needs to be modified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several others.

@koriym
Copy link
Member

koriym commented May 16, 2024

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 frederikbosch marked this pull request as draft May 16, 2024 08:52
@frederikbosch
Copy link
Contributor Author

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

@koriym
Copy link
Member

koriym commented May 16, 2024

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

@frederikbosch
Copy link
Contributor Author

Great idea to remove it, see #217

@frederikbosch
Copy link
Contributor Author

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.

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.

4 participants