Dnn.platform: Remove DotNetNuke.Library Circular Dependencies

[ahoefling]

Description of problem

The core library DotNetNuke.Library utilizes C# Reflection to circumvent a circular dependency problem in the platform. Since DotNetNuke.Library can't reference another library (such as DotNetNuke.Web.Mvc) that references itself DotNetNuke.Library. There is code scattered throughout the platform that uses C# Reflection which loads the assembly at runtime. While this works it creates side-effects in the platform that are prone to runtime crashes and performance slow-downs.

An example of how we are using reflection in the ModuleControlFactory
https://github.com/dnnsoftware/Dnn.Platform/blob/c1a352acb1ae7f52549d1e605822f56ae837ed45/DNN%20Platform/Library/UI/Modules/ModuleControlFactory.cs#L58-L64

Description of solution

My plan/proposal is to extra shared contracts so the different Module Libraries don't need to reference the entire DotNetNuke.Library Project. A new project would be created called DotNetNuke.Library.Contracts. All shared interfaces and contracts required for the Module Libraries would be moved to the new DotNetNuke.Library.Contracts project. This would then be referenced by both DotNetNuke.Library and the Module LIbraries. This would then remove the circular dependency and allow us to re-write the code above to

c# case ".mvc": controlFactory = new MvcModuleControlFactory(); break;

• New Project: DotNetNuke.Library.Contracts
• Module Library: References DotNetNuke.Library.Contracts
• DotNetNuke.Library References: DotNetNuke.Library.Contracts

Reasoning for Change

Creating a contracts project is fundamental to creating the building blocks of a wrapper that removes the dependency on System.Web. After completing this feature we would be able to create a wrapper around System.Web which will be a big step towards a .NET Core migration.

Description of alternatives considered

Leaving things the way they are makes a migration off of System.Web a very large undertaking. By making changes now that create abstractions around System.Web it will make the migration easier to manage in smaller chunks.

Screenshots

N/A

Additional context

N/A

I would like to include this change as part of the Razor Pages Feature. It is not required, but highly recommended.
Related: #2599

[mitchelsellers]

The biggest question that I have for you with this is that wouldn't this require the use of a DI container as well to truly implement. I understand the moving of the Interfaces for the contract to an external assembly, however, the DotNetNuke.Library project still needs the ability to reference/instantiate the MvcModuleControlFactory() which would be in the non-referenced assembly?

Or am I missing something here? I'm all for more responsible loading of types, especially when it comes to reflection.

[ahoefling]

wouldn't this require the use of a DI container as well to truly implement

Not Necessarily, but a nice to have

My proposal moves all required interfaces to a Contracts library, which would allow DotNetNuke.Library to have a project reference to the different module libraries. The whole idea of the Contracts library is to break the circular dependency and have the other libraries reference the shared code via that library.

This proposal is a baby-step towards Dependency Injection. I am recommending small incremental steps to prevent de-stabilization of the platform and easier chunks of work to accomplish.

Does this make sense and answer your question?

[mitchelsellers]

Sort of....would you then envision that the actual implementation of MvcModuleControlFactory would live in the contracts library, and it would then reference the MVC library for the bits it needed?

[ahoefling]

There are only really a few interfaces that the ControlFactory and Control use

• IControl (System.Web)
• IModuleControlFactory (DotNetNuke.Library)
• IActionable (DotNetNuke.Library)
• ModuleControlBase (DotNetNuke.Library)
• Control (System.Web)
• IModuleControl (DotNetNuke.Library)

If we move the necessary classes into a contracts library it will allow DotNetNuke.Library to reference the Module Projects directly which will allow us to instantiate objects in the ModuleControlFactory without reflection

[mitchelsellers]

Ah, that's the missing piece that I didn't get

DotNetNuke.Library -> DotNetNuke.Contracts
DotNetNuke.Library -> DotNetNuke.Web.Mvc
DotNetNuke.Web.Mvc -> DotNetNuke.Contracts

Correct?

[ahoefling]

That is correct

[mitchelsellers]

Ok, I guess the real impact/question here is what items will be moving assemblies and namespaces, as that would be a breaking change and technically subject to the N+1 version change requirement.

Can you outline what elements would move?

[ahoefling]

My proposal here does not change any namespaces to limit any breaking changes. The code would just live in another assembly.

Do you think it would make sense to identify contracts and flag them as a breaking change and migrate namespaces in N+1?

[mitchelsellers]

At this point, I think that the best recommendation would be to accomplish the following unless any other member of @dnnsoftware/tag has a differing opinion.

• Flag the implementations that exist within DotNetNuke.Library as Obsolete, further note on this below.
• Introduce the new assembly, and duplicate the items that are being moved, into the new namespace
• Update the usage/implementation within the DNN Core

I would suggest this attribute for all removed members.

[Obsolete("This implementation has moved to DotNetNuke.Library.Contracts. Scheduled removal in v11.0.0.")]

Additionally, due to the current deprecation process, I believe that we SHOULD include this in 9.4. My reasoning for this is as follows.

1. This is a major performance improvement. Some initial benchmarking that I prototyped this afternoon shows a reduction of about 15ms per module load when extrapolated across a large installation this is huge.
2. This implementation, although public in the declaration, is really a framework component and shouldn't be leveraged by external resources.
3. The 9.4.x release line is planned to be the stable release before we remove another batch of Obsolete API's in 10.x. As such, let us get this performance fix in now.

Additionally, alongside this, I am going to propose a revision to the deprecation policy for internal framework components.

The only question I would ask is regarding the name. Is DotNetNuke.Library.Contracts the proper name? Or should it be more specific such as DotNetNuke.Library.PipelineContracts or otherwise?

[ahoefling]

I am changing direction on this slightly, now we are going to create a DotNetNuke.ModulePipeline project which contains the Module Loading Pipeline for the platform.

Project References as Follows:

• DotNetNuke.Library -> DotNetNuke.ModulePipeline
• DotNetNuke.ModulePipeline -> DotNetNuke.Web.Mvc
• DotNetNuke.Web.Mvc -> DotNetNuke.Library

DotNetNuke.Library has a spider web of cycles in it that depend on each other which makes the original proposal impossible with a Contracts project.

As a follow up we should create other issues to start separating out the different areas of DotNetNuke.Library:

• Entities
• Framework
• Common
• Modules
• UI
• Etc.