Created INavigationManager to replace Globals.NavigateURL

[SkyeHoefling]

Closes: #3159

Summary

Refactored Globals.NavigateURL() to a new interface INavigationManager and updated ALL references to use dependency injection.

Note

This is a first attempt, and we may need to make several revisions to this or target 10.0 instead of 9.x.

Tasks

Below is a list of tasks that I need to complete prior to us accepting the PR. I will continue to edit them as we review the code changes:

• Fix failing unit tests
• Verify installer works
• Verify DNN Happy Path - General workflow, login, logout, page management, etc.
• Create DotNetNuke.Abstractions library to contain new common registrations such as INavigationManager
• Create nuspec file for DotNetNuke.Abstractions so it can be registered as a NuGet library
• Update all .ascx.cs injections to be private readonly instead of protected
• Remove extra xml records added by Visual Studio (.csproj, .config files)
• Add new unit tests for DotNetNuke.Common.NavigationManager

All Tasks Completed

[bdukes]

Why is this needed? The only internal type I'm seeing is NavigationManager, which none of these are directly referencing…

[SkyeHoefling]

The Globals.DependencyProvider (our DependencyInjection Container) is internal by design. However the entire platform doesn't fully support Dependency Injection and there are many areas where we need to manually resolve dependencies using the Service Locator Pattern

IMyCutomType myInstance = Globals.DependencyProvider.GetService<IMyCustomType>();

I am not sure the best way to approach this problem as to make this change I need all of those InternalsVisibleTo declarations otherwise we won't have access to the DependencyProvider.

@mitchelsellers and I have discussed in the past that we do not want to make the DependencyProvider public and it is internal to the platform.

[bdukes]

Gotcha, that works for me.

[SkyeHoefling]

This is creating Technical Debt for the platform. I don't like that I had to create all of these statements to allow classes to access Globals.DependencyProvider but it is a necessary evil. There is no way possible that we could migrate the entire platform to Dependency Injection in 1 Pull Request. That type of change is just way too massive and to consider it would require several weeks to review, at best.

As we proceed forward with more Dependency Injection changes we can re-evaluate these and remove them as needed. For example when the entry point of a certain workflow is being resolved by the container, that will allow us to use constructor injection further down the call stack. At that point we will be able to remove this, but not until then.

Ideally, there will never be any usage of Globals.DependencyProvider anywhere, but we are no where near that stage with our Dependency Injection usage

[SkyeHoefling]

@bdukes I updated the code with a notable change of migrating INavigationManager to a new .NET Standard 2.0 project called DotNetNuke.Abstractions. A small problem with this migration is the dependency on PortalSettings. This demonstrates the tight-coupling that top-level code has with bottom-tier code and vice-versa which makes these changes very difficult.

My proposed solution is to create a new IPortalSettings in the DotNetNuke.Abstractions project. Then I updated the actual PortalSettings in DotNetNuke.Library to implement the class and updated code throughout. The reach of this change is pretty significant, be sure to take a close look at the following items:

• IPortalController - I updated the GetCurrentPortalSettings() to be deprecated in lieu of changing the signature. If we were to change the signature to return IPortalSettings it will create a ripple effect and that is a much larger change. There is now a new method on the interface called GetCurrentSettings() which returns the IPortalSettings
• Updated several usages of PortalSettings to IPortalSettings - I believe these changes are not breaking changes, even for public APIs, this is because PortalSettings now inherits from IPortalSettings
• Removed [Browsable(false), DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)] from the interface and implementation as recommended
• I added IPortalController as a Dependency Injection registration so we can easily resolve it in the NavigationManager. This is the first instance Dependency Injection in the platform code where we have a dependency resolving another dependency. It is finally paying off 🎉

[SkyeHoefling]

I fixed the failing unit tests. I needed to create a mock of IServiceProvider and INavigationManager and manually assign it to the Globals.DependencyProvider. The difficult part to this is handling the state of Singletons. DNN uses the Singleton pattern throughout the codebase, the usage of this pattern was caching state between test classes which demonstrates poor unit test isolation.

As we move forward, hopefully we can migrate these tests to use interfaces instead of the singleton pattern

[SkyeHoefling]

Running ./build.ps1 -Target CreateInstall isn't working and I am waiting for build changes to be finalized from #3137. I have custom built an installer for this Pull Request but our installer files are too large to upload to GitHub. I have ran through the installer I have and this change does install correctly and basic DNN functionality appears to work.

If anyone wants to help verify this change I can email you the installer file or share it in one of the many slack channels we have in the DNN Community.

[SkyeHoefling]

Since this is a large Pull Request, I saw it best to rebase on the latest changes. This is a common practice I have done on large PRs for other .NET open source projects (CoreFX).

The build updates appear to be working and I can now build this solution locally and get an installer that works for testing. Once the build completes check out the installer and see if it works for you as well.

I would love to find problems with the changes now so we can address them during the code review

[SkyeHoefling]

@mitchelsellers @bdukes
I have pushed some more changes to address some of our requested changes. Right now this PR is just about ready to merge. If you guys can review again and provide any additional feedback.

My next steps at this point:

• Test the installer again (earlier tests proved to be successful)
• Clean up PR to exclude extra content in .config and .csproj

[SkyeHoefling]

@bdukes @mitchelsellers
I have tested the installer and marked all tasks completed. I believe this PR is ready to be merged.

Unless you have additional feedback

[mitchelsellers]

LGTM - Need to be sure to test this during RC

[valadas]

Looks good to me