NEW Adding constructor auto-DI for classes resolved through Injector #8564

ScopeyNZ · 2018-11-04T07:19:42Z

I can RFC whether this is a feature that the core team is interested in for 4.4 if wanted.

This adds auto-DI for constructors of classes resolved through Injector. This works by checking the constructor docblock for a tag @Injectable and then using typehint reflection to resolve dependecies. There's some docs in the PR.

Having dependencies expressed in the constructor is a good way to indicate a classes dependencies in a clear way - producing expressive and testable classes. I think we should be encouraging this sort of DI, and perhaps make the @Injectable tag not required in 5.0

sminnee · 2018-11-05T10:08:17Z

My main concern with this would be performance impact – Injector creation can be used 10,000s of times on a single request. Have we done any profiling?

robbieaverill

Looks like a very cool change! Because this has a high likelihood of becoming an important part of the API, I think it'd be good to get the @silverstripe/core-team across it. Also @sminnee's comment about profiling is valid too, we'd need to make sure this can be performant

src/Core/Injector/MethodInjector.php

robbieaverill · 2018-11-05T10:23:43Z

src/Core/Injector/MethodInjector.php

+
+    /**
+     * MethodInjector constructor.
+     * @param ReflectionMethod $method


PSR-5 (when accepted) will require an empty line above this

robbieaverill · 2018-11-05T10:25:06Z

tests/php/Core/Injector/MethodInjectorTest.php

+
+    public function testTaggedInjectableConstructors()
+    {
+        Config::nest()->set(Injector::class, TestDependency::class . '.testTag', AlternateTestDependency::class);


I think use Config::modify() here

ScopeyNZ · 2018-11-05T20:50:44Z

My main concern with this would be performance impact – Injector creation can be used 10,000s of times on a single request. Have we done any profiling?

It adds time, but we're talking <1s on >1 million instantiations. Every little bit counts though - I'll add some caching per class constructor that should alleviate most concerns.

I could also add config to disable it completely?

sminnee · 2018-11-05T20:53:56Z

It adds time, but we're talking <1s on >1 million instantiations. Every little bit counts though - I'll add some caching per class constructor that should alleviate most concerns.

One Thousand Milliseconds is a lot of time! :-P

Can we get a bit of data on, say, one of the expensive /treeview renders that we were using for testing our CMS NFRs

How long it took before
How long it took after
How many Injector-based creates are being called
Perhaps a synthetic test to see how long that many injector-based creates would take

If we introduce caching we'd probably need to understand how many different cache entries are used in the above

maxime-rainville · 2019-01-25T10:20:23Z

I ran a profile locally using our beloved key project. The page I used has 63 content blocks. I'm working off the assumption that this will be representative of an average slow request. I didn't set out to create a request with lots of injector calls.

The response time went up 2.85% (+ 207ms). SilverStripe\Core\Injector\Injector get() got call 7 more times. This leads to an increase response time of 128ms.

maxime-rainville · 2019-01-25T10:24:05Z

On another note, I'm not sure about this reading the @Injectable from the PHP doc. To my knowledge, that would be the only context where comments alters the behaviour of your code in SilverStripe. I'm not necessarily against the idea, but I think it might end up confusing some people the first time they come across it.

ScopeyNZ · 2019-01-25T11:07:31Z

Hmmn that's not as bad as I thought it could be. You're right that it could make more sense in SilverStripe to do this with private static.

tractorcow · 2019-01-28T19:53:06Z

What about this as an implementation solution: Instead of resolving the annotations at resolve time, you could add another config transformer that would read all class annotations on flush, and transliterate these to the config equivalent, and cache them there.

By merging these into config on the Injector class, you can also override these defaults with YML, and you get the benefit of config caching (i.e. no performance hit except on flush).

tractorcow · 2019-01-28T19:54:44Z

/**
 * Reads annotations for all classes and maps them to config injections on the
 * `SilverStripe\Core\Injector\Injector` class key
 */
class AnnotationTransformer implements TransformerInterface {
}

ScopeyNZ · 2019-01-28T22:51:14Z

That's a good idea. I'll put aside some time to look at this soon. For this PR I might switch to using existing configuration facets but I'm interested in pursuing the annotation configuration idea in a separate PR/RFC.

maxime-rainville · 2019-03-28T23:15:08Z

@ScopeyNZ Did you have a change to have another look at this?

ScopeyNZ · 2019-03-28T23:16:40Z

@ScopeyNZ Did you have a change to have another look at this?

Haha it's on my list of "before 4.4 is feature-frozen" - So I will endeavour to look over it within the next couple of weeks.

robbieaverill · 2019-04-18T02:31:59Z

ping @dnsl48, we were talking about SilverStripe DI earlier =D

dnsl48 · 2019-04-18T03:28:42Z

yeah, I definitely love the idea! Though, as already discussed above, needs some polishing :)

dhensby

This looks cool - I'd like to see a bit more inline documentation for MethodInjector, though

@Injectable

…o injected - Adding a new MethodInjector that uses a ReflectionMethod as a constructor argument and injects type hinted classes with Injector - New MethodInjector has an API that will check a docblock for an @Injectable tag - Updating Injector to check constructors for an @Injectable tag and auto-inject dependencies if tagged - Adding tests & docs for constructor DI

…uced in silverstripe#9006

ScopeyNZ · 2019-05-24T01:35:34Z

Worked on this a little on hackday - going off the suggestion from @tractorcow to have an annotation based config transformer. PRs open on config and framework for that. This has been rebased onto the framework PR (#9006).

Still need to run through docs again and do some performance testing.

kinglozzer · 2019-09-16T08:54:37Z

tests/php/Core/Injector/MethodInjectorTest/InjectableConstructorTagged.php

+     * @param TestDependency $protectedDependency
+     * @param array $additionalParams
+     * @Injectable
+     * @Inject(TestDependency,testTag)


If this annotation is used for injector “tags” like CacheInterface.permissions for example, should we use dots in the annotation rather than commas? Sorry if I’m misunderstanding the use case here 😅

Yeah this is a good point. I think my idea here when I wrote this was that it'd be similar to Doctrine annotations, which are based of classes. The dot notation makes more sense in the SS ecosystem though.

Cheddam · 2020-07-23T04:22:26Z

@ScopeyNZ Is this something you're still keen to push across the line? Looks like PHP 8 will have native attributes, so it could be worth awaiting and exploring those?

ScopeyNZ · 2020-07-23T17:02:45Z

Yeah as far as this is concerned it was pretty much good to go when I left it. There was a case for showing some proof that there's no significant effect on performance, which I haven't done. I'll run through this stuff when I get some time and put updates on it.

PHP 8 native attributes are definitely nice - and a great feature for this PR, but considering how long it took for PHP 7 to be a requirement, it would be frustrating to have something similar blocking this feature.

maxime-rainville · 2020-09-01T23:47:06Z

I haven't done a detailed review, but I'm happy to approve the approach in principal.

@brynwhyman I think this is a high value PR and that a lot of work as gone into implementing it. If the rest of the Core Committers agree this new API is worthwhile, I think it would be worth adding this the the CMS Squad board to take it over the line.

robbieaverill · 2020-09-01T23:52:30Z

I like the idea and I think the DI system in Silverstripe could use some attention.

I'm not super sold on using annotations for this. One reason is that it's not really part of the API, rather comments. Another reason is that IDEs probably won't autocomplete class names when you're using annotations.

ScopeyNZ · 2020-09-02T00:59:28Z

The annotations are there as a "switch" to enable auto-DI for constructors, as I wasn't sure that suddenly doing this for any class resolved from the container was safe. Thinking about it now though, I think Injector will just error if you don't provide a required dependency? The problem would be how we tell Injector that it's safe to inject an optional dependency.

The other purpose of the annotations is to inject objects in the container that have a specific tag. Not really sure of any other appropriate way to do this. Some frameworks (eg Laravel) don't let you do this.

GuySartorelli · 2022-07-07T01:32:34Z

Note: The documentation portion should not be merged here. I have started the process of migrating docs to a new repository so the docs will need to be removed from this PR and a new PR raised in the new repo instead.

GuySartorelli · 2023-12-14T03:35:59Z

I'm going to close this - I think if we were to ever merge something like this today it would be using attributes rather than annotations

robbieaverill reviewed Nov 5, 2018

View reviewed changes

robbieaverill added change/minor affects/v4 labels Nov 5, 2018

dhensby reviewed Apr 21, 2019

View reviewed changes

ScopeyNZ added 3 commits May 24, 2019 13:23

NEW Allow use of new AnnotationTransformer for configuration

c3e60d9

NEW Refactor method injection to work off of annotation config introd…

66974a7

…uced in silverstripe#9006

ScopeyNZ force-pushed the pulls/4.4/constructor-di branch from fe291d1 to 66974a7 Compare May 24, 2019 01:33

ScopeyNZ mentioned this pull request Sep 16, 2019

NEW Allow use of new AnnotationTransformer for configuration #9006

Closed

kinglozzer reviewed Sep 16, 2019

View reviewed changes

ScopeyNZ mentioned this pull request Sep 1, 2020

NEW Add AnnotationTransformer to allow configuration through PHP docblock annotations silverstripe/silverstripe-config#35

Closed

chillu added impact/high type/enhancement labels Nov 18, 2020

emteknetnz removed change/minor labels Jun 20, 2022

GuySartorelli mentioned this pull request Jul 7, 2022

RFC: Move documentation out of framework into a dedicated repo #10215

Closed

7 tasks

GuySartorelli closed this Dec 14, 2023

GuySartorelli deleted the pulls/4.4/constructor-di branch December 14, 2023 03:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Adding constructor auto-DI for classes resolved through Injector #8564

NEW Adding constructor auto-DI for classes resolved through Injector #8564

ScopeyNZ commented Nov 4, 2018 •

edited

Loading

sminnee commented Nov 5, 2018

robbieaverill left a comment

robbieaverill Nov 5, 2018

robbieaverill Nov 5, 2018

ScopeyNZ commented Nov 5, 2018

sminnee commented Nov 5, 2018

maxime-rainville commented Jan 25, 2019

maxime-rainville commented Jan 25, 2019

ScopeyNZ commented Jan 25, 2019

tractorcow commented Jan 28, 2019

tractorcow commented Jan 28, 2019

ScopeyNZ commented Jan 28, 2019

maxime-rainville commented Mar 28, 2019

ScopeyNZ commented Mar 28, 2019

robbieaverill commented Apr 18, 2019

dnsl48 commented Apr 18, 2019

dhensby left a comment

ScopeyNZ commented May 24, 2019

kinglozzer Sep 16, 2019

ScopeyNZ Sep 2, 2020

Cheddam commented Jul 23, 2020

ScopeyNZ commented Jul 23, 2020

maxime-rainville commented Sep 1, 2020

robbieaverill commented Sep 1, 2020

ScopeyNZ commented Sep 2, 2020

GuySartorelli commented Jul 7, 2022 •

edited

Loading

GuySartorelli commented Dec 14, 2023

NEW Adding constructor auto-DI for classes resolved through Injector #8564

NEW Adding constructor auto-DI for classes resolved through Injector #8564

Conversation

ScopeyNZ commented Nov 4, 2018 • edited Loading

sminnee commented Nov 5, 2018

robbieaverill left a comment

Choose a reason for hiding this comment

robbieaverill Nov 5, 2018

Choose a reason for hiding this comment

robbieaverill Nov 5, 2018

Choose a reason for hiding this comment

ScopeyNZ commented Nov 5, 2018

sminnee commented Nov 5, 2018

maxime-rainville commented Jan 25, 2019

maxime-rainville commented Jan 25, 2019

ScopeyNZ commented Jan 25, 2019

tractorcow commented Jan 28, 2019

tractorcow commented Jan 28, 2019

ScopeyNZ commented Jan 28, 2019

maxime-rainville commented Mar 28, 2019

ScopeyNZ commented Mar 28, 2019

robbieaverill commented Apr 18, 2019

dnsl48 commented Apr 18, 2019

dhensby left a comment

Choose a reason for hiding this comment

ScopeyNZ commented May 24, 2019

kinglozzer Sep 16, 2019

Choose a reason for hiding this comment

ScopeyNZ Sep 2, 2020

Choose a reason for hiding this comment

Cheddam commented Jul 23, 2020

ScopeyNZ commented Jul 23, 2020

maxime-rainville commented Sep 1, 2020

robbieaverill commented Sep 1, 2020

ScopeyNZ commented Sep 2, 2020

GuySartorelli commented Jul 7, 2022 • edited Loading

GuySartorelli commented Dec 14, 2023

ScopeyNZ commented Nov 4, 2018 •

edited

Loading

GuySartorelli commented Jul 7, 2022 •

edited

Loading