U4-11427 : Remove property injection #2690

lars-erik · 2018-06-13T07:06:05Z

(Full discussion on tracker under ID from this PR's title)
(see original issue)

HttpModules able to get injection for now. Will continue to see about controllers etc.

…ed) gets dependencies injected through ContainerUmbracoModule.

Notes on the UmbracoModule. Need to abstract resolving via Current.Container to remove LightInject refs.

More notes / fixme.

…r tests.

Shazwazza

Left a couple comments but the gist is that the removal of property injection should also mean there are no setters for those properties since they are set only in the ctor. I think any fixme remove ctor's for our built in controllers should be removed indeed. And for the empty protected ctor's, don't we want to call base or this and populate the dependencies based on Current?

I know this is a WIP so things are in flux :) looks great so far!

Shazwazza · 2018-06-20T05:01:05Z

src/Umbraco.Web/Mvc/PluginController.cs

@@ -36,37 +35,31 @@ public abstract class PluginController : Controller, IDiscoverable
        /// <summary>
        /// Gets or sets the Umbraco context.
        /// </summary>
-        [Inject]
        public virtual UmbracoContext UmbracoContext { get; set; }


These should not have setters

Thanks. Missed it.

Shazwazza · 2018-06-20T05:01:57Z

src/Umbraco.Web/Mvc/UmbracoAuthorizedController.cs

@@ -10,5 +16,13 @@
    [UmbracoAuthorize]
    [DisableBrowserCache]
    public abstract class UmbracoAuthorizedController : UmbracoController
-    { }
+    {
+        protected UmbracoAuthorizedController()


Don't we want to call : base(....) and use Current to populate the dependencies?

It will automagically call the one in UmbracoController. 😋

lars-erik · 2018-06-20T06:11:11Z

\o/

I manage to squeeze in an hour here and there.

…factory method that looks up services by name instead.

lars-erik · 2018-06-20T20:05:18Z

src/Umbraco.Core/IO/FileSystems.cs

@@ -305,7 +305,8 @@ public TFileSystem GetFileSystemProvider<TFileSystem>(Func<IFileSystem> fallback

                // fixme - switch to using container. where are these registered?

-                var fs = (IFileSystem) Activator.CreateInstance(typeof(TFileSystem), shadowWrapper);
+                //var fs = (IFileSystem) Activator.CreateInstance(typeof(TFileSystem), shadowWrapper);
+                var fs = Current.Container.GetInstance<TFileSystem>((IFileSystem)shadowWrapper);


@Shazwazza seems like you've done something like this before: seesharper/LightInject#237

Got any idea why it complains it can't resolve the dependency? We get this down, we've basically got all tight couplings solved.

See ContainerAdapter.cs line 48 for concrete impl. Basic forwarding of object array to concrete container. (No extension usage)

nvm, got the hang of it.

A bit messy, and probably leaves quite a bit of dead / useless code around that should be deleted. Needs some manual testing.

…for most regression runs.

lars-erik · 2018-06-25T22:39:45Z

src/Umbraco.Core/Runtime/CoreRuntimeComponent.cs

@@ -44,6 +44,9 @@ public override void Compose(Composition composition)
            composition.Container.Register<DatabaseBuilder>();

            // register filesystems
+
+            composition.Container.Register<IFileSystem, MediaFileSystem>((f, wrappedFileSystem) => new MediaFileSystem(wrappedFileSystem, f.GetInstance<IContentSection>(), f.GetInstance<ILogger>()));


This is most likely not covered by any tests. :/
Should look into testing/verifying all composition builders etc. later.

lars-erik · 2018-06-25T22:44:08Z

Getting there. The MediaFileSystem should now be resolved from the container, but get the ShadowWrapper thingy as a runtime argument. A few concrete IO tests fail, most likely due to less stubbed dependency needs.

Otherwise, just MembershipHelper to go. :)

Also took the liberty of adding a "Slow" category to a bunch of integrated tests. Makes it quicker to run "most". (8 vs 16 min on my older box)

…pletely Broken (tm). Need to review whole lifetime & registration of it.

(But OMG! there's nearly no test coverage of Umbraco.Web :O )

lars-erik · 2018-06-28T21:32:36Z

Boom! Done!

At least I hope so. Seems like all the tests are passing, but the Umbraco.Web assembly is sadly lacking in coverage. Guess the MembershipHelper dependencies has to be tested somewhat manually sooner or later.

There's so much more to do, but at least the first task can be ticked off. :)
It's a mouthful, but I hope you can find the time to look over it. Maybe @zpqrtbnk should have a look as well?

I'd really like to get on with abstracting all the registrations as well. Should I make it another PR based off this? Could probably rebase and force push the first one.

abjerner · 2018-06-28T21:52:38Z

@lars-erik I may not understand all of this PR, but seriously, great job 👍

zpqrtbnk · 2018-06-29T06:03:28Z

Going to review all this asap. Excellent!

…hub.com/lars-erik/Umbraco-CMS into temp8-u4-11227

lars-erik · 2018-06-29T07:27:49Z

\o/

It's interesting to notice the faintly fresher smells of the now huge constructors and weird registration cases. They probably reek a bit of feature envy and possibly SRP breaches (and more). I guess it's worth investigating when through the immediate bits.

On a sidenote - some of the scope and IO tests seem to be failing intermittently. Dunno if that's from these changes or if it's still a bit unstable?

zpqrtbnk · 2018-06-29T07:38:03Z

FWIW some merge conflicts need to be fixed - have done it locally - now going to test and review code

…Non LI req. / MS-DI)

…yinjection' into temp8-di2690

JimBobSquarePants · 2019-01-04T20:33:52Z

Yay!

HttpModules able to get injection. UmbracoModule + rest (if implement…

4c201bc

…ed) gets dependencies injected through ContainerUmbracoModule.

ghost assigned lars-erik Jun 13, 2018

ghost added the review label Jun 13, 2018

lars-erik and others added 5 commits June 14, 2018 18:46

DataTypeValidateAttribute now uses bastard injection.

a8c5cd4

Notes on the UmbracoModule. Need to abstract resolving via Current.Container to remove LightInject refs.

UmbracoModule parameterless ctor removed.

9b1bd3b

More notes / fixme.

Current.Container abstracted. Builds, but fails for collection builde…

e3d77fb

…r tests.

ScopeEventDispatcherTest needs IServiceContainer to be registered.

0e37b3b

Several property injections removed from Mvc artifacts and base classes

e4aca5f

lars-erik mentioned this pull request Jun 19, 2018

U4-11427 Inversion of the Dependency Inversion container. #2683

Closed

2 tasks

lars-erik added 2 commits June 19, 2018 18:59

Removed an extra using LI. :)

ea9ab66

Tests pass again

7af04b0

Shazwazza suggested changes Jun 20, 2018

View reviewed changes

lars-erik and others added 3 commits June 20, 2018 19:57

Removed named service reference from repo ctors. Now registered with …

db2ad87

…factory method that looks up services by name instead.

Tests build ><

590765e

Almost got MediaFileSystem in container.

34390de

lars-erik commented Jun 20, 2018

View reviewed changes

lars-erik and others added 3 commits June 26, 2018 00:26

MediaFileSystem now purely injected.

3a7d7e3

A bit messy, and probably leaves quite a bit of dead / useless code around that should be deleted. Needs some manual testing.

Added "Slow" category to several fixtures, cutting test time in half …

49e9514

…for most regression runs.

Merge branch 'temp8' into temp8-u4-11427-remove-propertyinjection

5f2ec56

lars-erik commented Jun 25, 2018

View reviewed changes

lars-erik added 2 commits June 28, 2018 22:38

Fixes constructor dependency conondrum for MediaFileSystem. It is Com…

3d2810e

…pletely Broken (tm). Need to review whole lifetime & registration of it.

MembershipHelper presumably working without property injection.

91a5605

(But OMG! there's nearly no test coverage of Umbraco.Web :O )

Merge branch 'temp8-u4-11427-remove-propertyinjection' of https://git…

3bd7eed

…hub.com/lars-erik/Umbraco-CMS into temp8-u4-11227

Fixes

7cad591

aaronpowell mentioned this pull request Dec 14, 2018

[Discussion] Would Umbraco benefit from a RFC process? #3871

Closed

Stephan and others added 25 commits December 17, 2018 18:55

True standalone test

35ebced

Fixes

813eccd

Merge branch temp8 into temp8-di2690

115b0cd

Merge branch origin/temp8 into temp8-di2690

bb9846a

Merge branch origin/temp8 into temp8-di2690

a60fd1b

Fix merge

f21dbb3

Merge branch origin/temp8 into temp8-di2690

b21d07a

Merge branch 'temp8' into temp8-di2690

ceab758

Version 8.0.0-alpha.56

81a9807

Upgrade to ModelsBuilder -alpha.26

a434ac8

Bugfix RuntimeState

4f7a5e3

Add container tests

7a37c87

TEMP

9fe8b5f

Refactor components

816ed84

Fix components

9b756b9

Fix components

0481c2c

Public ctor on CoreRuntimeComponent

b8a939d

Temporary downgrade of MB due to missing onlien pkg

b8191c3

Registering WebRuntimeComponent and publicizing its dep. chain

c18ed7e

Public ctor on ExamineComponent to allow construction

0455c7f

Internalize testing ctor to provide unambiguous ctor for container. (…

8e5ed6e

…Non LI req. / MS-DI)

Actually uses a scope in the ScopeBuilderCreatesScopedCollection tests

7cfe211

Fix component constructors

1bd4cba

Merge remote-tracking branch 'lars-erik/temp8-u4-11427-remove-propert…

3f1843d

…yinjection' into temp8-di2690

Refactor collection builders to be nicer with DI

f6cd134

zpqrtbnk merged commit f6cd134 into umbraco:temp8 Jan 4, 2019

lars-erik mentioned this pull request Jan 4, 2019

V8 WIP: Migrate DI abstraction to MS.DI #3955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

U4-11427 : Remove property injection #2690

U4-11427 : Remove property injection #2690

lars-erik commented Jun 13, 2018 •

edited by zpqrtbnk

Loading

Shazwazza left a comment

Shazwazza Jun 20, 2018

lars-erik Jun 20, 2018

Shazwazza Jun 20, 2018

lars-erik Jun 20, 2018 •

edited

Loading

lars-erik commented Jun 20, 2018

lars-erik Jun 20, 2018

lars-erik Jun 25, 2018

lars-erik Jun 25, 2018

lars-erik commented Jun 25, 2018

lars-erik commented Jun 28, 2018

abjerner commented Jun 28, 2018

zpqrtbnk commented Jun 29, 2018

lars-erik commented Jun 29, 2018

zpqrtbnk commented Jun 29, 2018

JimBobSquarePants commented Jan 4, 2019

U4-11427 : Remove property injection #2690

U4-11427 : Remove property injection #2690

Conversation

lars-erik commented Jun 13, 2018 • edited by zpqrtbnk Loading

Shazwazza left a comment

Choose a reason for hiding this comment

Shazwazza Jun 20, 2018

Choose a reason for hiding this comment

lars-erik Jun 20, 2018

Choose a reason for hiding this comment

Shazwazza Jun 20, 2018

Choose a reason for hiding this comment

lars-erik Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

lars-erik commented Jun 20, 2018

lars-erik Jun 20, 2018

Choose a reason for hiding this comment

lars-erik Jun 25, 2018

Choose a reason for hiding this comment

lars-erik Jun 25, 2018

Choose a reason for hiding this comment

lars-erik commented Jun 25, 2018

lars-erik commented Jun 28, 2018

abjerner commented Jun 28, 2018

zpqrtbnk commented Jun 29, 2018

lars-erik commented Jun 29, 2018

zpqrtbnk commented Jun 29, 2018

JimBobSquarePants commented Jan 4, 2019

lars-erik commented Jun 13, 2018 •

edited by zpqrtbnk

Loading

lars-erik Jun 20, 2018 •

edited

Loading