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

Deprecate Razor Modules for DNN 11.0 #2618

Merged
merged 3 commits into from
Apr 4, 2019

Conversation

SkyeHoefling
Copy link
Contributor

Closes: #2613

Summary

Marks all public and protected APIs in

  • DotNetNuke.Web.Razor
  • DotNetNuke.Modules.RazorHost
[Obsolete("Deprecated in 9.3.1, will be removed in 11.0.0, use Razor Pages instead")]

As we build out Razor Pages modules we will update the messages where applicable to point to what they should be using instead.

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.

Looking at this, do you believe there is any possible reason to retain the "RazorHost" module concept, which allows users to upload arbitrary Razor to run on the page? However, have it use the proper API's on the backend?

If so, it might be prudent to not Obsolete the items inside of /DNN Platform/Modules/RazorHost

@dnnsoftware/tag-group Any thoughts on the importance or usage of this module?

@SkyeHoefling
Copy link
Contributor Author

@mitchelsellers thanks for bringing that up, as I wasn't 100% sure what we wanted to do with that. In my opinion we should Deprecate "Razor Host" Modules and remove it from the platform, but create a new one in it's place that utilizes Razor Pages.

I think this will be valuable because:

  • It will create an example module for Razor Pages
  • It will give current Razor Host implementations a migration path
  • It will be ready to transition to .NET Core

@valadas
Copy link
Contributor

valadas commented Mar 10, 2019

I would prefer that we do have the razor pages nailed down not as an experimental feature before announcing this deprecated. So maybe we could have a link to documentation on how the new pipeline works or an example module before telling people it is deprecated, people need to know how to move forward when they see that deprecation notice.

@mitchelsellers
Copy link
Contributor

@valadas I would traditionally agree, however, our policy is putting things into a bit of a bind.

We have a published policy that we are not removing items until 2 major release after we announce that we remove it. If we do not do this in the 9.x release cycle, we would then be unable to complete this until at least version 12, which would be a long way down the future.

We can hold the PR until 9.4.0 when the Experimental Feature is enabled, however, the goal is to get notice out sooner than later on this. The actual usage/adoption of this feature is unknown.

@valadas
Copy link
Contributor

valadas commented Mar 12, 2019

I understand.

@SkyeHoefling
Copy link
Contributor Author

@mitchelsellers @valadas
Is there anything I need to do for this PR

@ohine
Copy link
Contributor

ohine commented Mar 21, 2019

@ahoefling I don't think so, we should be able to merge it in shortly for either v9.3.1 or v9.4.0. I'm ok with it going in v9.3.1.

@SkyeHoefling
Copy link
Contributor Author

Perfect! Thanks for the clarification

@mitchelsellers
Copy link
Contributor

@ahoefling I'll add my review on this as well. I don't see any issue with merging to 9.3.1.

The key note here is that the current "Razor Host" module is going to go away, but we might have a replacement "Razor Pages Host" or similar functionality that would then trigger an update to the Depreciation messages.

mitchelsellers
mitchelsellers previously approved these changes Mar 21, 2019
valadas
valadas previously approved these changes Mar 23, 2019
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@ohine
Copy link
Contributor

ohine commented Apr 4, 2019

@ahoefling Sorry we missed merging this into v9.3.1. Can you update the message? If you're targeting development, set the obsolete message to Deprecated in 9.4.0. If you rebase from release/9.3.2, then update the message to Deprecated in 9.3.2

@SkyeHoefling
Copy link
Contributor Author

@ohine thanks for the update. I'll rebase this against 9.3.2 and update the PR

@SkyeHoefling SkyeHoefling changed the base branch from development to release/9.3.2 April 4, 2019 02:33
@ohine ohine added this to the 9.3.2 milestone Apr 4, 2019
@SkyeHoefling SkyeHoefling dismissed stale reviews from valadas and mitchelsellers via 41eb2da April 4, 2019 02:34
@SkyeHoefling
Copy link
Contributor Author

@ohine I have updated the PR and rebased it to Release/9.3.2

@mitchelsellers @valadas
I needed to do a force-push since my branch had commits that aren't in the 9.3.2 branch. As part of doing the force push your reviews became stale. Please take a look at the changes and let me know if there is anything we need to do.

@ohine
Copy link
Contributor

ohine commented Apr 4, 2019

Thanks @ahoefling! We will get this reviewed and merged very shortly!

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.

Lgtm

@ohine ohine merged commit a9d8644 into dnnsoftware:release/9.3.2 Apr 4, 2019
@SkyeHoefling SkyeHoefling deleted the deprecate_razor branch April 4, 2019 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants