search

dnnsoftware / Dnn.Platform

DNN (formerly DotNetNuke) is the leading open source web content management platform (CMS) in the Microsoft ecosystem.
https://dnncommunity.org/
MIT License
1.02k stars 746 forks source link

Refactor Globals.cs to use Dependency Injection #3153

Open SkyeHoefling opened 4 years ago

SkyeHoefling commented 4 years ago

Description of problem

In DNN 9.4.0 we introduced Dependency Injection and it is time to start refactoring the Globals.cs file to use Dependency Injection. This is a massive undertaking and should not be attempted in 1 Pull Request. This work item should serve as an epic that we can link others back to as we solve small problems.

Description of solution

Refactor sections of Globals.cs into manageable interfaces grouped in logical chunks. For example all of the NavigateURL methods should move to an interface called INavigationManager.

Description of alternatives considered

N/A

Screenshots

N/A

Affected browser

Work Items

A table of work items of chunks that we will break out the Globals.cs into

Work Item ID Manager Description
#3159 INavigationManager All navigation methods should exist in an easy to use interface
SkyeHoefling commented 4 years ago

When we implement this throughout the platform should we be recommending GetService<T>() or GetRequiredService<T>() for resolving dependencies using the Service Locator pattern. As we migrate away from the Globals.cs we are going to need to use the Service Locator pattern quite a bit.

GetService()

If the container fails to find the dependency, the method will return null, which will most likely cause a NullReferenceException

GetRequiredService()

If the container fails to find the dependency, the container will throw an exception and stop the app right there

Most of the Globals and usage throughout the internals of DNN are required, so I tend towards GetRequiredService<T>() but it is hard to tell when one should be used over the other.

SkyeHoefling commented 4 years ago

To properly migrate from Globals.cs to using dependency injection we should mark as many APIs as [Obsolete] and deprecated as soon as we can. I would hate for us to mark APIs as deprecated that we haven't identified a good Dependency Injection Story for.

My goal is to get as many of these marked for deprecation for v11 as I don't want to have to migrate a Globals file to .NET Core and we have no idea when we are ready for the switch, but I want us to be prepared

@mitchelsellers @bdukes Do you have any thoughts on what we should do for Globals.cs as we start documenting and migrating it to Dependency Injection? Could we create an issue and submit a PR that just deprecates all APIs and then we can deal with them on a case-by-case basis?

bdukes commented 4 years ago

I think that makes sense @ahoefling. Do we need to require that these new interfaces are .NET Standard compliant (i.e. don't reference types from System.Web, et al)?

SkyeHoefling commented 4 years ago

@bdukes this is a great question! I am all for creating a new library called DotNetNuke.Common.Interfaces or something similar that we can put these interfaces in. My first PR on this I created a new namespace called DotNetNuke.Common.Interfaces and placed the interface in there.

I think this is an important change, can you leave this feedback on PR #3160 so we can set the stage right for follow-up PRs and how we want it implemented

valadas commented 4 years ago

Just for triage, this issue is not complete right, it is a big item that will be done as multiple PRs correct?

Also how much of this can we do in 9 or does it all need to go to 10. One issue was that existing modules would need to be recompiled if used APIs or dependencies get moved into a new library...

tpperlman commented 4 years ago

Hi folks, I see the latest DNN release marks Globals.NavigateURL as becoming obsolete in a future release:

[Obsolete("Deprecated" in Platform 9.4.2. Scheduled removal in v11.0.0.")]

There doesn't appear to be any recommendations in the code moving forward. Is there guidance on how we are to replace our usage of Globals.NavigateURL in 3rd party modules? I'd prefer to not suppress these messages for now unless I have to.

Thanks!

mitchelsellers commented 4 years ago

@tpperlman The recommendation will be to use the new INavigationProvider interface with Dependency Injection for your custom solutions - https://github.com/dnnsoftware/Dnn.Platform/pull/3160

tpperlman commented 4 years ago

Thanks @mitchelsellers!

So if I understand correctly the new way going forward would be to add a reference DotNetNuke.Abstractions in the project and replace entries like:

return DotNetNuke.Common.Globals.NavigateURL(SomeTabId);

with

using DotNetNuke.Abstractions; ... protected INavigationManager NavigationManager { get; } ... return NavigationManager.NavigateURL(SomeTabId);

mitchelsellers commented 4 years ago

Yes!

valadas commented 4 years ago

I guess this would be for Dnn10? Just asking for triage, will put it there but if I am wrong feel free to change or comment

valadas commented 4 years ago

If any of the labels I put don't make sense, please edit or let me know...

Constantine-Ketskalo commented 4 years ago

Hi everyone. I stumbled upon issue and as I understand it has to do with this refactoring. I checked versions 9.6.0 and 9.6.1. When I pulled source code of these release versions and tried to compile it and run it from visual studio 2019 at localhost, it fails. It throws NullReferenceException at line 585, because attempt to get instance of DataProvider returns null. How can it be fixed? chrome_rmFxtyF1pv

bdukes commented 4 years ago

@Constantine-Ketskalo that doesn't appear (to me) to be related to this issue. You'll want to review the build documentation, which describes using Cake to build packages and setup a dev site. Just running the site from Visual Studio doesn't provide a complete site install, unfortunately. /cc @donker

Constantine-Ketskalo commented 4 years ago

Thank you so much, @bdukes.

moto100 commented 2 years ago

Thanks @mitchelsellers!

So if I understand correctly the new way going forward would be to add a reference DotNetNuke.Abstractions in the project and replace entries like:

return DotNetNuke.Common.Globals.NavigateURL(SomeTabId);

with

using DotNetNuke.Abstractions; ... protected INavigationManager NavigationManager { get; } ... return NavigationManager.NavigateURL(SomeTabId);

Actually，You can get a instance of INavigationManager when your control inherit from PortalModuleBase.

navigationManager = DependencyProvider.GetRequiredService<INavigationManager>();

Or

 IServiceProvider serviceProvider2 = DependencyProvider;
 navigationManager = ((serviceProvider2 != null) ? ServiceProviderServiceExtensions.GetRequiredService<INavigationManager>(serviceProvider2) : null);

I verified them in DNN 9.10.2

valadas commented 1 year ago

In MVC you can get the services injected for you using constructor injection like described here: https://docs.dnncommunity.org/content/getting-started/development/fundamentals/dependency-injection/mvc/index.html

Note that since you need INavagationManager, this is already registered by DNN itself so for that one, you don't need to also implement IDnnStartup