-
Notifications
You must be signed in to change notification settings - Fork 745
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
Created IApplicationInfo and IApplicationStatusInfo for Dependency Injection #3988
Conversation
@@ -26,27 +26,27 @@ public enum ReleaseMode | |||
/// <summary> | |||
/// Not asssigned | |||
/// </summary> | |||
None, | |||
None = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Added explicit numbers here to ensure this enum interops correctly with the new DotNetNuke.Abstractions
enum. This will prevent any accidental breaking changes if the enums are ever re-ordered or changed. When we are converting the enums it is a simple cast as the int value matches, there is no need for a complex conversion method.
|
||
/// <summary> | ||
/// Alpha release | ||
/// </summary> | ||
Alpha, | ||
Alpha = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment - https://github.com/dnnsoftware/Dnn.Platform/pull/3988/files#r471134926
{ | ||
_current = new DotNetNukeContext(); | ||
current = Globals.DependencyProvider.GetRequiredService<IDnnContext>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate this function and ask consumers to use Dependency Injection? I decided not to deprecate this function because I didn't add the remaining members to the new IDnnContext
. I think once we add the remaining members we can deprecate this method and recommend Dependency Injection.
If reviewers are in agreement I can create a work item
return this._application; | ||
} | ||
} | ||
public Application Application { get => this.applicationInfo is Application app ? app : new Application(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the class is instantiated without Dependency Injection for some reason such as unit tests, I updated a default Application
instantiation. Maybe we should update this to the DependencyProvider
@@ -316,28 +316,28 @@ public enum UpgradeStatus | |||
/// <summary> | |||
/// The application need update to a higher version. | |||
/// </summary> | |||
Upgrade, | |||
Upgrade = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment about enums - https://github.com/dnnsoftware/Dnn.Platform/pull/3988/files#r471134926
@@ -139,6 +141,10 @@ public void SetUp() | |||
ComponentFactory.Container = new SimpleContainer(); | |||
MockComponentProvider.ResetContainer(); | |||
|
|||
var serviceCollection = new ServiceCollection(); | |||
serviceCollection.AddTransient<IApplicationStatusInfo>(container => new DotNetNuke.Application.ApplicationStatusInfo(Mock.Of<IApplicationInfo>())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of the unit tests depend on File Operations that are generated from the MapApplicationPath
function. It was difficult to mock this out and it was easier to use an actual implementation of IApplicationStatusInfo
instead of a mock.
/// </summary> | ||
private static void OnDependencyProviderChanged(object sender, PropertyChangedEventArgs eventArguments) | ||
{ | ||
applicationStatusInfo = DependencyProvider.GetRequiredService<IApplicationStatusInfo>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a method to register dependencies once the provider has been configured is needed since the DependencyProvider
uses our custom LazyServiceProvider
. It is critical that the Global.cs
resolves only ONE instance of a dependency, as it can generate too many sql connections that causes DNN to stop functioning.
This finding has been documented - #3989
} | ||
|
||
internal void SetProvider(IServiceProvider serviceProvider) | ||
{ | ||
this._serviceProvider = serviceProvider; | ||
this.serviceProvider = serviceProvider; | ||
this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(serviceProvider))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added INotifyPropertyChanged
here to allow the Globals.cs
to listen for an event when the Service Provider is ready for us. Since the Globals.cs
is wrapping several Dependency Injection calls as we deprecate methods it is important that it can resolve dependencies once they are available. This is critical when using Globals.cs
at startup.
I decided on INotifyPropertyChanged
as it is a .NET Standard type and is used in the desktop/mobile space a lot for notifying objects when something has changed. If there is a better option, I am more than happy to change this
This is ready for review I added single line comments around sections of the code that needed extra explanation of why I made some changes. All unit tests are passing. I downloaded the build artifacts and validated that it does install DNN correctly for a new installation. |
I'm not sure why it shows the build is still in progress. I just checked the build server and the build for this PR is showed success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @ahoefling!
/azp run |
No pipelines are associated with this pull request. |
…rked related global apis as deprecated
…lobal methods that needed to use new way of handling application status via IApplicationStatusInfo
…obals to listen for when the provider is set and update any dependencies in the Globals class
…ement INotifyPropertyChanged (useful for unit testing). Updated unit tests to mock out IApplicationStatusInfo
…sinfo and resolve it only once
…xt and deprecated it. This will prevent a breaking change
46e521d
to
1632371
Compare
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
@bdukes I believe I have addressed all of your concerns in the latest update. Let me know if there is anything else I need to do to get this PR ready to merge |
@bdukes We good for this to be merged for 9.7.1? I don't think it will be a big/breaking issue. |
Amazing work! |
Related to #3985
Summary
This does not close the linked work item, but is required before I can submit a pull request.
While working on changes to the Client Resource Manager I noticed circular dependency between
DotNetNuke.Libary
andDotNetNuke.Web.Client
. TheClientDependencySetting
utilizes reflection to workaround the circular dependency. Dynamically loading types like this is considered an anti-pattern, especially when we can use depenency injection to extract an interface.This Pull Request extracts 2 interfaces from the Globals.cs
IApplicationInfo
IApplicationStatusInfo