First code submission for MVC Pipeline #6749

donker · 2025-10-03T13:34:00Z

This PR is a first submission of the work of the MVC Pipeline team to bring the main project up to date with what has been developed in a forked repo.

…tomizations for the MVC pipeline.

Fix layout issue

- fix case

client dependecy for mvc (because it not exist in nuget)

Mvcpl2

Merge latest DNN dev commits and fix for MVC modules

Merge latest development branch from core project

Solution to get AF token to work on MVC pipeline

# Conflicts: # DNN Platform/DotNetNuke.Web.Client/packages.config

mitchelsellers · 2025-10-03T15:49:09Z

@bdukes @donker I believe we discussed that this is just a reset PR and initial review, as it is into their branch correct? High level review only?

valadas · 2025-10-03T17:21:17Z

Awesome, I'll try to get my review in this weekend...

mitchelsellers · 2025-10-07T19:20:07Z

@donker In looking at this at this moment the solution isn't building, would you be able to review this and resolve the issue?

mitchelsellers · 2025-10-07T19:22:10Z

Build/Tasks/BuildPackages.cs

+    [IsDependentOn(typeof(CreateUpgrade))]
+    [IsDependentOn(typeof(CreateNugetPackages))]
+    [IsDependentOn(typeof(GeneratePackagesChecksums))]
+    public sealed class BuildPackages : FrostingTask<Context>


What is the purpose/goal of this task addition? I'm concerned that this will be deleting a source controlled file.

@donker Can you answer this ?

Sorry ... that is my code bleed. I can delete it. It just builds the packages. But I believe the "default" task also does this.

mitchelsellers · 2025-10-07T20:02:00Z

DNN Platform/DotNetNuke.ContentSecurityPolicy/BaseCspContributor.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information
+
+namespace DotNetNuke.ContentSecurityPolicy


Is this the same as the pending CSP PR into develop?

Yes it's the same. It need to be removed from here after the pr in develop is merged, so we can remerg it in this branch.
Or some other sujestion how to manage this ?

mitchelsellers · 2025-10-08T05:20:50Z

DNN Platform/DotNetNuke.Web.Client/Providers/DnnStandardRenderer.cs

+        {
+            get
+            {
+                var settingsVersion = this.dnnSettingsHelper.AreCompositeFilesEnabled();


Given the changes to CDF (In pending state now) should this be removed?

Yes, the idee is to remove all CDF for mvc that is done here once the changes to CDF are ready to be integrated in this branch.

mitchelsellers · 2025-10-08T05:22:34Z

DNN Platform/DotNetNuke.Web.Client/MvcClientResourceManager.cs

+        private static readonly ReaderWriterLockSlim LockFileExistsCache = new ReaderWriterLockSlim();
+
+        /// <summary>Adds the necessary configuration to website root <c>web.config</c> to use the Client Dependency component.</summary>
+        public static void AddConfiguration()


Why is this beind done as a Web.Client item, shouldn't this be handled by the existing functionality? I'm concerned this is going to introduce a duplicate or race condition

Yes, you are right, it's will beter to do all the web.config modification in one place.
In reality, this class will be removed once the new CDF is used.

sachatrauwaen · 2025-10-13T12:40:43Z

@donker In looking at this at this moment the solution isn't building, would you be able to review this and resolve the issue?

@mitchelsellers I just try to build a fresh checkout of feature/mvc-pipeline-old without issuues (.\build.ps1).

Can you tell witch build you use or what kind of errors you get ?

mitchelsellers · 2025-10-13T18:15:08Z

@sachatrauwaen It is the AzureDevOps build that is failing, it looks like it is unit tests that are failing to execute.

The most recent run - https://github.com/dnnsoftware/Dnn.Platform/pull/6749/checks?check_run_id=51891737747

donker · 2025-10-14T19:18:18Z

@bdukes @donker I believe we discussed that this is just a reset PR and initial review, as it is into their branch correct? High level review only?

Yes. This is what we discussed in an earlier meeting to have the first code submission a proper PR.

valadas

Ok, I found a bit of time and reviewed about the first 1/4 of this PR.

I think we should bring this PR to the minimal viable MVC working state (even if incomplete) just with the minimum to load up an mvc theme and module.

MVC Pipeline and CSP are 2 big ticket items and bringing both in the same PR makes putthing this in a reviewable state hard IMO. The goal of this PR was to have a baseline so that we can review smaller additions in an easier way.

Other than that, it does not have to be in this PR but I would like that we add our stylecop rules to the new projects to enforce our coding standards on this huge amount of new code.

I'll try to review some more the next coming days.

Thanks @sachatrauwaen for this huge contribution!

valadas · 2025-10-15T06:37:23Z

DNN Platform/DotNetNuke.Web.MvcPipeline/Controllers/ModuleSettingsController.cs

+        private void CheckPermissions()
+        {
+            // Verify that the current user has access to edit this module
+            if (!ModulePermissionController.HasModuleAccess(SecurityAccessLevel.Edit, "MANAGE", this.Module))
+            {
+                if (!(this.IsSharedViewOnly() && TabPermissionController.CanAddContentToPage()))
+                {
+                    throw new UnauthorizedAccessException("Access denied");
+                }
+            }
+        }


This one hurts my brain a bit specially since it relates to permissions and should be very clear and intuitive.

Suggested change

private void CheckPermissions()

{

// Verify that the current user has access to edit this module

if (!ModulePermissionController.HasModuleAccess(SecurityAccessLevel.Edit, "MANAGE", this.Module))

{

if (!(this.IsSharedViewOnly() && TabPermissionController.CanAddContentToPage()))

{

throw new UnauthorizedAccessException("Access denied");

}

}

}

private void CheckPermissions()

{

// Verify that the current user has access to edit this module

if (ModulePermissionController.HasModuleAccess(SecurityAccessLevel.Edit, "MANAGE", this.Module))

{

return;

}

if (this.IsSharedViewOnly() && TabPermissionController.CanAddContentToPage())

{

return;

}

throw new UnauthorizedAccessException("Access denied");

}

Which then begs the question, should we allow the module settings control to be viewed for a shared module to content editors? It looks wrong to me unless I am missing something.

valadas · 2025-10-15T06:48:00Z

DNN Platform/DotNetNuke.Web.MvcPipeline/Controllers/ModuleSettingsViewController.cs

+            }
+        }
+
+        private void CheckPermissions()


Same concern here as my previous comment on the similar code in ModuleSettingsController.cs

Also what is the difference between ModuleSettingscontroller and MOduleSettingsViewController.cs?

valadas · 2025-10-15T06:50:41Z

DNN Platform/DotNetNuke.Web.MvcPipeline/Controllers/ModuleViewControllerBase.cs

+        public bool EditMode
+        {
+            get
+            {
+                return this.ModuleContext.EditMode;
+            }
+        }
+
+        public bool IsEditable
+        {
+            get
+            {
+                return this.ModuleContext.IsEditable;
+            }
+        }
+
+        public int PortalId
+        {
+            get
+            {
+                return this.ModuleContext.PortalId;
+            }
+        }
+
+        public int TabId
+        {
+            get
+            {
+                return this.ModuleContext.TabId;
+            }
+        }


If we expose ModuleContext, do we need to also expose those properties, could we make the API surface simper by limiting the redundant properties?

valadas · 2025-10-15T06:51:31Z

DNN Platform/DotNetNuke.Web.MvcPipeline/Controllers/ModuleViewControllerBase.cs

+        public UserInfo UserInfo
+        {
+            get
+            {
+                return this.PortalSettings.UserInfo;
+            }
+        }
+
+        public int UserId
+        {
+            get
+            {
+                return this.PortalSettings.UserId;
+            }
+        }
+
+        public PortalAliasInfo PortalAlias
+        {
+            get
+            {
+                return this.PortalSettings.PortalAlias;
+            }
+        }


Same for PortalSetings which is already exposed, do we need those properties also exposed?

valadas · 2025-10-15T06:58:03Z

DNN Platform/DotNetNuke.Web.MvcPipeline/Extensions/StartupExtensions.cs

+                            type is { IsClass: true, IsAbstract: false });
+            foreach (var controller in mvcControllerTypes)
+            {
+                services.TryAddTransient(controller);


Should this be scoped @bdukes ?

We're using scoped for web API controllers, but transient for MVC controllers:

Dnn.Platform/DNN Platform/DotNetNuke.Web/Api/Extensions/StartupExtensions.cs

Line 33 in 3ce2474

services.TryAddScoped(controller);

Dnn.Platform/DNN Platform/DotNetNuke.Web.Mvc/Extensions/StartupExtensions.cs

Line 33 in 3ce2474

services.TryAddTransient(controller);

Scoped it probably the right way

sachatrauwaen · 2025-10-15T15:16:15Z

MVC Pipeline and CSP are 2 big ticket items and bringing both in the same PR makes putthing this in a reviewable state hard IMO. The goal of this PR was to have a baseline so that we can review smaller additions in an easier way.

There an separate PR for CSP on the develop branch.

But removing it from this PR needs to remove a lot of code.
The idee is to replace the CSP project of this PR with the PR on develop when the PR is merged in develop.

Actually we heave a lot of sync issues between the differnt PR's.
I don'tknow the best way. So your advice is welcome.

valadas · 2025-10-15T20:16:05Z

MVC Pipeline and CSP are 2 big ticket items and bringing both in the same PR makes putthing this in a reviewable state hard IMO. The goal of this PR was to have a baseline so that we can review smaller additions in an easier way.

There an separate PR for CSP on the develop branch.

But removing it from this PR needs to remove a lot of code. The idee is to replace the CSP project of this PR with the PR on develop when the PR is merged in develop.

Actually we heave a lot of sync issues between the differnt PR's. I don'tknow the best way. So your advice is welcome.

So if that is the case, we need to review/agree on the CSP one first before spending too much time reviewing this one...

valadas · 2025-10-15T20:16:11Z

MVC Pipeline and CSP are 2 big ticket items and bringing both in the same PR makes putthing this in a reviewable state hard IMO. The goal of this PR was to have a baseline so that we can review smaller additions in an easier way.

There an separate PR for CSP on the develop branch.

But removing it from this PR needs to remove a lot of code. The idee is to replace the CSP project of this PR with the PR on develop when the PR is merged in develop.

Actually we heave a lot of sync issues between the differnt PR's. I don'tknow the best way. So your advice is welcome.

So if that is the case, we need to review/agree on the CSP one first before spending too much time reviewing this one...

donker and others added 30 commits March 20, 2025 11:36

Initial port of MVC-PAGE branch to DNN 10

96b3954

Fixes

806a567

With previous

18dda02

More moving stuff around

2b93b83

Moving more classes to Mvc Pipeline project

e66429e

Add build file

195b407

Move MVC skin detection to separate class

192a1fe

Added the controllers for the core website

ed9dfad

NewDDRMenu project which forks the original DDR menu project with cus…

0b39bb2

…tomizations for the MVC pipeline.

Rewire the way we detect MVC skins

23b6b13

Ability to replace the urlrewriter

825ae25

Changes that hard wired to ascx files for the skin

26a4165

Revert changes to core project

261a156

Merge branch 'release/10.0.0' into MVCPL

9ec0a3d

Rolling back mvc skin detection to what it was previously

44c90c7

Fixing routing

bcba2c6

Fix layout issue

6b1f317

Merge pull request #1 from donker/fix1

4700236

Fix layout issue

- avoid dublicate key error with standard DDRMENU

af884e5

- fix case

client dependecy for mvc (because it not exist in nuget)

78bcb42

Merge pull request #3 from sachatrauwaen/mvcppl1

419c044

client dependecy for mvc (because it not exist in nuget)

restrore use of clientdependency.core.mvc

5e3b2d4

add use of css variables

d17225a

add support for report-to

e903cda

update web.config for mvc

55308ab

port all what is needed to run from dnn9 version

e107bd4

first part of the html module : view control

c91f68a

Merge pull request #4 from sachatrauwaen/mvcpl2

09c4c04

Mvcpl2

use dnn10 default.css

4aae3e8

make edit in mvc work again

0fd3b47

donker added 11 commits June 7, 2025 14:43

Fix for postback in MVC modules

6f44d1f

Merge pull request #34 from donker/tmpmerge

26b700a

Merge latest DNN dev commits and fix for MVC modules

Merge branch 'develop' into MVCPL

a6d4027

Merge branch 'develop' into tmp

9ab6e50

Merge pull request #35 from donker/tmp

fdcde3a

Merge latest development branch from core project

Code fixes for outdated code

ec3b710

Rolling back earlier change

d94ca80

Solution to get AF token to work on MVC pipeline

c369e1e

Merge pull request #36 from donker/aftoken

df42015

Solution to get AF token to work on MVC pipeline

Merge branch 'develop' into MVCPL

4476348

Merge branch 'develop' into MVCPL

ae00a15

# Conflicts: # DNN Platform/DotNetNuke.Web.Client/packages.config

donker added this to Issue Triage Oct 3, 2025

mitchelsellers reviewed Oct 7, 2025

View reviewed changes

mitchelsellers added this to the Future: Major milestone Oct 7, 2025

mitchelsellers reviewed Oct 7, 2025

View reviewed changes

mitchelsellers reviewed Oct 8, 2025

View reviewed changes

sachatrauwaen added 2 commits October 14, 2025 15:19

Fix tests on DnnController.cs

7be534e

Fix blank lines in DnnController.cs

526412b

remove blank linesDnnController.cs

9ab7cae

valadas reviewed Oct 15, 2025

View reviewed changes

Uh oh!

First code submission for MVC Pipeline #6749

Are you sure you want to change the base?

First code submission for MVC Pipeline #6749

Uh oh!

Conversation

donker commented Oct 3, 2025

Uh oh!

mitchelsellers commented Oct 3, 2025

Uh oh!

valadas commented Oct 3, 2025

Uh oh!

mitchelsellers commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachatrauwaen Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachatrauwaen commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchelsellers commented Oct 13, 2025

Uh oh!

donker commented Oct 14, 2025

Uh oh!

valadas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachatrauwaen commented Oct 15, 2025

Uh oh!

valadas commented Oct 15, 2025

Uh oh!

valadas commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sachatrauwaen Oct 13, 2025 •

edited

Loading

sachatrauwaen commented Oct 13, 2025 •

edited

Loading