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

Fixes DDRMenu Razor Bug with Dependency Injection #2998

Merged
merged 2 commits into from
Sep 17, 2019

Conversation

SkyeHoefling
Copy link
Contributor

@SkyeHoefling SkyeHoefling commented Sep 17, 2019

Fixes: #2997

Summary

The DDRMenu supports multiple types of rendering engines. The razor engine which uses .cshtml files leverages the DNN Razor3 Module Platform. When we implemented Dependency Injection for 9.4.0 this was added to every module platform including the Razor3 Module Pattern (which is used by DDRMenu Razor).

The problem

When running a page that uses DNN 9.4.0 the Razor Menu would completely disappear, which is documented in #2997. This was discovered when installing the default theme for nvQuickTheme.

Screenshot

Copied screenshot from #2997 for convenience
image

The Fix

In DNN 9.4.0 we updated the constructor of any Razor3 module's DotNetNukeWebPage<T> to resolve the model if it is included in the IoC DependencyProvider. This provides simple DependencyInjection for this deprecated module pattern.

As part of this change, we decided to create a default instantiation of the Model if it isn't registered in the DependencyProvider. This default instantiation causes the crash.

public abstract class DotNetNukeWebPage<TModel> :DotNetNukeWebPage where TModel : class
{
    // unchanged code

    public DotNetNukeWebPage()
    {
        var model = Globals.DependencyProvider.GetService<TModel>();
        Model = model ?? Activator.CreateInstance<TModel>();
    }
}

Removing the default instantiation appears to fix the problem

Model = model; // Removing 'Activator.CreateInstance<TModel>();`

Testing

I have followed the exact same testing steps highlighted in #2997. I do not have any other Razor3 modules to test. We should really test a non DDRMenu Razor3 module to validate it still works

My List of Modules to Test

  • DDRMenu Razor3
  • Razor3 Module (Using Dependency Injection)
  • Razor3 Module (Not Using Dependency Injection)

… by mistake in 9.4.0. The page needs to handle both usage of Dependency Injection and the PageContext.Model
@david-poindexter david-poindexter added this to the 9.4.1 milestone Sep 17, 2019
@SkyeHoefling
Copy link
Contributor Author

I am going to be attaching modules we can use to test this, please don't merge until I mark all my items as tested

@SkyeHoefling
Copy link
Contributor Author

Test Pass - Razor3 Dependency Injection

I created a small DI module using Razor 3 and it appears to not cause any problems on the page using the DDRMenu Razor3 impl.

I attached the module for anyone to use on their site to verify as well

Dnn.DependencyInjection.Samples.Razor3_00.00.02_Install.zip

@SkyeHoefling
Copy link
Contributor Author

Test Undecided - Simple Razor Module

I am not 100% sure what the rules are going to be for this test case

Dnn.Simple.Samples.Razor3_00.00.02_Install.zip

Background

The Activator code was added to force a default instantiation of the Model. As far as I remember in some Razor3 Modules you would need to manually instantiate the Model in the .cshtml view. I am not 100% sure what we want for the logic

Manual Instantiation

@using DotNetNuke.Web.DDRMenu;
@using System.Dynamic;
@inherits DotNetNuke.Web.Razor.DotNetNukeWebPage<MyModel>

@{
    Model = new MyModel
}

Auto Instantiation

In this example the code should automatically create an instance of MyModel

@using DotNetNuke.Web.DDRMenu;
@using System.Dynamic;
@inherits DotNetNuke.Web.Razor.DotNetNukeWebPage<MyModel>

Options

If we decide Auto Instantiation, we will need to add additional rules to handle the problem with DDRMenu. If we decide to force the user to manually instantiate their model then we can go with the PR as implemented

@david-poindexter
Copy link
Contributor

@ahoefling based on this blog post it looks like manual instantiation was the pattern recommended.

donker
donker previously approved these changes Sep 17, 2019
Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the menu

mitchelsellers
mitchelsellers previously approved these changes Sep 17, 2019
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahoefling I think at this point this is the "right" solution, granted not the full solution for the future of DNN Platform.

The prior architecture change of manual instantiation is NOT a best practice, but it is the pattern that was supported. For DNN 9.4.1 I think we need to restore this functionality as-is, and will then adjust as we learn more in 10.x to improve the pattern to modern and current standards of implementation.

I'll hold on merging though, until you mark cleared the other types. but I believe this is a solid workaround.

@SkyeHoefling
Copy link
Contributor Author

Please do not merge this change

I just did a pair coding session with @donker and we uncovered some more details on this. I will update the thread later today with more details.

I have a change that will make all the scenarios listed above pass and work together on the same page, which was the original desire to adding the Activator.CreateInstance call

@SkyeHoefling
Copy link
Contributor Author

SkyeHoefling commented Sep 17, 2019

Quick History

DNN has many different razor rendering engines which are different than the actual module platforms that use them

  • Razor3 Web Pages != MVC (Razor) != Razor Pages != Blazor

There are 2 different module platforms in DNN that use Razor Engines

  • Razor3 Modules (NOT RazorHost)
  • MVC Modules

The Razor Engine

There is a project called DotNetNuke.Web.Razor which many 3rd party modules have historically used to render a .cshtml file, this includes the DDRMenu. As part of leveraging some code that probably should have been marked internal we now are in a position where the DotNetNuke.Web.Razor.RazorEngine needs to support multiple usages.

The APIs

There are 2 APIs that are used through the platform and the 3rd party module ecosystem from DotNetNuke.Web.Razor

// Used in 3rd party modules
void Render<T>(TextWriter writer, T model);

// Used in Razor3 Modules and 3rd Party Modules
void Render(TextWriter writer);

Remember these are both deprecated in 11.x

Razor3 Module and the Razor Engine

The Razor3 Module pattern handles the render pipeline for you so all you worry about is creating your .cshtml file. Since the model is unknown at this point, DNN will try and render the .cshtml file and let the render engine process any model that may be included.

Consider the following .cshtml file

@using Dnn.Simple.Samples.Razor3.Models
@inherits DotNetNuke.Web.Razor.DotNetNukeWebPage<IndexModel>

<h1>@Model.Title</h1>

When the OnPreRender is invoked this snippet will render that file

if (! (string.IsNullOrEmpty(RazorScriptFile)))
{
var razorEngine = new RazorEngine(RazorScriptFile, ModuleContext, LocalResourceFile);
var writer = new StringWriter();
razorEngine.Render(writer);
Controls.Add(new LiteralControl(writer.ToString()));
}

From this point the RazorEngine will take the .cshtml and try and process it by passing NULL for the actual model. Seen in the line of code below:

public void Render(TextWriter writer)
{
try
{
Webpage.ExecutePageHierarchy(new WebPageContext(HttpContext, Webpage, null), writer, Webpage);
}
catch (Exception exc)
{
Exceptions.LogException(exc);
}
}

It has been a best practice in DNN .cshtml pages to specify the base page expliclity using the @inherits syntax. In our case here we explicitly set the base page via
@inherits DotNetNuke.Web.Razor.DotNetNukeWebPage<IndexModel>

We tried to add defensive code in the DotNetNukeWebPage<TModel> class to automatically instantiate the Model if it is not set. ** his cause problems if using the other render method where you pass the model to the RazorEngine**

public DotNetNukeWebPage()
{
var model = Globals.DependencyProvider.GetService<TModel>();
Model = model ?? Activator.CreateInstance<TModel>();
}

All of this actually works

RazorHost Control

I really don't know much about the RazorHost Control, but here are my findings

The RazorHost Control is NOT technically a Razor3 Module in a pure sense of the term DNN Module. It goes around the initial rendering logic and uses the RazorEngine directly. The good thing is it uses the RazorEngine exactly how the Razor3 Module does which means there isn't any real reason for concern on breaking changes.

protected override void OnPreRender(EventArgs e)
{
base.OnPreRender(e);
if (!(string.IsNullOrEmpty(RazorScriptFile)))
{
var writer = new StringWriter();
Engine.Render(writer);
Controls.Add(new LiteralControl(HttpUtility.HtmlDecode(writer.ToString())));
}
}

DDRMenu and 3rd Party Modules

There are many 3rd party modules out there including the DDRMenu (which is now a core module) that utilize the DotNetNuke.Web.Razor.RazorEngine which may not have been the intention of the code since it goes around the actual module pattern and uses the rendering layer directly.

Instead of calling the render method that passes null for the model, these modules know what their model is prior to the render sequence and pass it through. Consider the following snippet from the DDRMenu

var razorEngine = new RazorEngine(virtualPath, null, null);
razorEngine.Render<dynamic>(writer, model);

Remember ALL of this is deprecated for 11.x

The Solution

Since we need to handle these different usages of the RazorEngine we need to determine which render sequence was called from the DotNetNuke.Web.Razor.DotNetNukeWebPage which isn't easy. Since it inherits from the WebPageBase we should be able to check the PageContext.Model first and if it is instantiated use it otherwise use our model.

public new TModel Model
{
    get { return PageContext?.Model as TModel ?? _model; }
    set { _model = value; }
}

The downside to this technique is anyone using the RazorEngine WILL NOT get Dependency Injection. Which is fine in my opinion since that isn't really a module pattern, it is just using the rendering engine.

@mitchelsellers
Copy link
Contributor

@ahoefling I would agree with your assessment, using RazorEngine as a rendering engine doesn't need DI

…el; Swapped Model Accessor to check the PageContext.Model first for a model and if that is null use this classes _model instantiation
@SkyeHoefling
Copy link
Contributor Author

With the new changes all the 3 test cases highlighted above are now passing. I have a test page that uses all 3 types of modules:

  • Razor3 Module with Dependency Injection
  • Razor3 Module without Dependency Injection
  • RazorEngine from DotNetNuke.Web.Razor (DDRMenu)

Screenshot

image

Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with what Andrew stated above. We worked this issue today and verified a few divergent use cases which passed. The term "Razor3" was new to me as we never used that in the DNN ecosystem. But I'll happily defer to authority in Andrew. So my suggestion is to merge this asap so it will go out with 9.4.1.

@donker donker merged commit 4f29ba8 into dnnsoftware:release/9.4.x Sep 17, 2019
@alenxmedia
Copy link

Hello,
This issue still has not been resolved. Please reopen it.

@mitchelsellers
Copy link
Contributor

@alenxmedia this was resolved in 9.4.1, which is not yet released. Have you tried the RC and still encountering issues?

Otherwise the 9.4.1 release is coming soon

@alenxmedia
Copy link

Hello,
Yes I have tried the latest RC but that did not solve the problem.
I have opened a new issue : #3042 and created sample module to demonstrate problem.
Please take a look.

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

Successfully merging this pull request may close these issues.

7 participants