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

Refactor Globals.cs to use Dependency Injection #3153

Open
5 tasks done
SkyeHoefling opened this issue Oct 16, 2019 · 16 comments
Open
5 tasks done

Refactor Globals.cs to use Dependency Injection #3153

SkyeHoefling opened this issue Oct 16, 2019 · 16 comments

Comments

@SkyeHoefling
Copy link
Contributor

SkyeHoefling commented Oct 16, 2019

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

  • Chrome
  • Firefox
  • Safari
  • Internet Explorer
  • Edge

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 SkyeHoefling changed the title Refactor Globals.cs to use Refactor Globals.cs to use Dependency Injection Oct 16, 2019
@SkyeHoefling
Copy link
Contributor Author

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

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

bdukes commented Oct 17, 2019

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

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

valadas commented Nov 15, 2019

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

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

@tpperlman The recommendation will be to use the new INavigationProvider interface with Dependency Injection for your custom solutions - #3160

@tpperlman
Copy link

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

Yes!

@valadas
Copy link
Contributor

valadas commented Jan 11, 2020

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

valadas commented Jan 11, 2020

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

@Constantine-Ketskalo
Copy link

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

bdukes commented May 14, 2020

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

Thank you so much, @bdukes.

@valadas valadas removed this from the 10.0.0 milestone Jul 28, 2020
@valadas valadas added this to the Future: Major milestone Jul 28, 2020
@moto100
Copy link

moto100 commented Mar 7, 2022

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

valadas commented Mar 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment