Created IPortalAliasService for Dependency Injection

[SkyeHoefling]

related to #3985

Summary

• Adds a new interface IPortalAliasService which contains all the public APIs on the PortalAliasController.
• Marked all static APIs on the PortalAliasController as deprecated
• Updated IPortalInfo
• Updated IPortalAliasInfo
  • Removed some APIs that were accidently added to the IPortalAliasInfo, see my review comments as it is technically a breaking change.
• Added new PortalAliasCollection

Before Merging

Task	Status
Validate PR build	✅ Completed
Unit Tests Passing	✅ Completed

[SkyeHoefling]

Updated the PR to fix the failing unit tests to produce build artifacts. I tested the install build and portal aliases appear to update correctly. Looking forward to our code review discussion 👍

[bdukes]

If we're creating something new, do we want to use a custom collection class? Or would it be simpler to just use IDictionary<string, IPortalAliasInfo>?

[SkyeHoefling]

I agree with you on this, adding our own collection objects is something that I don't see as necessary anymore. I brought it over to keep consistency. I am going to take a 2nd look at the implementation and see if it is truly necessary. I'll report my findings here

[mitchelsellers]

I would second the desire to rid of the custom collection in favor of a standard collection

[mitchelsellers]

Also, creating something new, would it be good to have a "GetDefaultPortalAlias" or similar with Portal & Culture? To fix what people end up needing to do so often

[bdukes]

Does this look right @mitchelsellers?

/// <summary>Gets the primary alias for the site.</summary>
/// <param name="portalId">The portal ID.</param>
/// <exception cref="ArgumentException">No portal has the given <paramref name="portalId"/>.</exception>
/// <returns>An <see cref="IPortalAliasInfo" /> instance.</returns>
/// <remarks>If no alias is marked as primary, returns the first neutral culture alias.</remarks>
IPortalAliasInfo GetPrimaryPortalAlias(int portalId);

/// <summary>Gets the primary alias for the site.</summary>
/// <param name="portalId">The portal ID.</param>
/// <param name="cultureCode">The culture code.</param>
/// <exception cref="ArgumentException">No portal has the given <paramref name="portalId"/>, or no alias matches the <paramref name="cultureCode"/>.</exception>
/// <returns>An <see cref="IPortalAliasInfo" /> instance.</returns>
/// <remarks>If no alias is marked as primary, returns the first alias matching the culture.</remarks>
IPortalAliasInfo GetPrimaryPortalAlias(int portalId, string cultureCode);

[SkyeHoefling]

@bdukes @mitchelsellers is there any actionable item here that I need to take on the PR?

[sleupold]

shouldn't the method be named GetPortalAliasByPortalAliasId instead of GetPortalAliasByPortalAliasID?

[bdukes]

Thanks!

[SkyeHoefling]

Thank you everyone for the feedback and assistance on this PR. I have pushed my changes based on the feedback everyone has left. Right now I am waiting for a final decision on the last remaining open discussion #4021 (comment).

[bdukes]

Sorry to keep coming up with changes, but I'm hopeful we can build a solid foundation with this work. We really appreciate all you're putting into the Platform!

[mitchelsellers]

Just to pull this review full-circle for @ahoefling, as well as to call attention to a few more small items

Naming Conventions
I think for these new API's we should, as @bdukes recommended adopt a naming convention of Id rather than ID to match the current standards.

In addition to this, the HTTPAlias item then becomes a question, should this be HttpAlias? I believe yes.

Nullable Values
Not to possibly throw a truly crazy breaking change in here, but I think it should be discussed. DNN Platform, up to this point has been a huge fan of magic numbers. Things such as Null.NullInteger and Null.NullDate rather than supporting actually nullables. SHOULD we get away from this practice with these new APIs.

For example it would simplify things with TermsTabId for example as a check for ".HasValue" would be enough to know if that was configured. But this would be a HUGE break/shift. @bdukes @ahoefling thoughts.

Fields Needing Removed & Documents as Obsolete

Confirming the list of fields that we do NOT want to include here is as follows. Can we also, as part of this get the existing values in the proper class flagged with a comment of.

Functionality Removed prior to 9.7.x, API will be removed during the 10.00.00 release

This is my recommendation as these features have been discussed as removed for a long time and we already have breaking changes included in 10.x of this nature. I'm fine being overridden on this. @david-poindexter I'd like your opinion on this as well.

Fields:

• BackgroundFile
• BannerAdvertising
• PaymentProcessor
• ProcessorPassword
• ProcessorUserId
• HostFee
• Currency
• Users
• Version
• Pages

[david-poindexter]

@mitchelsellers I am in agreement here.

[david-poindexter]

Nullable Values
Not to possibly throw a truly crazy breaking change in here, but I think it should be discussed. DNN Platform, up to this point has been a huge fan of magic numbers. Things such as Null.NullInteger and Null.NullDate rather than supporting actually nullables. SHOULD we get away from this practice with these new APIs.

My vote would be yes, to get away from this poor practice.

[mitchelsellers]

@ahoefling Let me know if you want to chat about the Nullables.....hate to throw crazy your way, but if we can make this "right" we can avoid breaking changes

[sleupold]

AFAIR, DNN used its dedicated null values, as .Net 1 didn't include nullable types.
If we'll move to using nullable types, IMO we should make sure., it is properly used in the database as well.

[bdukes]

👍🏻 for using nullable values (though it may be tricky to track down which fields are or are not nullable)

[SkyeHoefling]

Thanks everyone for the comments and concerns on this PR. There is a lot to unpack with all the additional comments

Naming Convention

Id vs ID for properties.

@mitchelsellers

I think for these new API's we should, as @bdukes recommended adopt a naming convention of Id rather than ID to match the current standards.

@bdukes

what do you think about enforcing a consistent naming convention for Id over ID in these new interfaces?

It is considered best practices to use Id instead of ID which is a more strict CamalCasing.

✅ I agree

Strict CamalCasing

@mitchelsellers

In addition to this, the HTTPAlias item then becomes a question, should this be HttpAlias? I believe yes.

As just mentioned it is considered a best practice.

✅ I agree

Removing Unused Payment Properties

@mitchelsellers @bdukes and I have been discussing the removal of several payment gateway fields that are no longer used. We shouldn't bring these over to the new interface. @mitchelsellers put together this final list which will be removed from the new IPortalInfo interface.

• BackgroundFile
• BannerAdvertising
• PaymentProcessor
• ProcessorPassword
• ProcessorUserId
• HostFee
• Currency
• Users
• Version
• Pages

✅ I agree

Nullable Fields

@mitchelsellers

DNN Platform, up to this point has been a huge fan of magic numbers. Things such as Null.NullInteger and Null.NullDate rather than supporting actually nullables. SHOULD we get away from this practice with these new APIs.

The Null.NullInteger is an anti-pattern in how it leverages in magic numbers, this ultimately should be removed from DNN. I do not think this is the time and place to do it. We should create a separate work item to put on the backlog that captures this. As @sleupold and @bdukes it will be difficult to get this right. Especially ensuring that the database is updated correctly.

❌ I disagree, this should be created as a new work item. It goes way out of scope of this. (Still a great discussion to have)

API Deprecation

@mitchelsellers

Functionality Removed prior to 9.7.x, API will be removed during the 10.00.00 release

I have no problem with removing deprecated fields at the 10.0 release instead of the 11.0 release. It actually makes things much easier for us to innovate and move the platform forward. In my opinion the 2 major release deprecation notice is kind of excessive, many OSS projects deprecate and remove APIs at their next major version release.

❓ I am not sure what you would like me to do with this. Should I be updating all active deprecation notices that are < 9.7.x (which is everything at this moment since this PR is targeting 9.7.2) to list they will be removed at v10.0?.

[Obsolete("Deprecated in 9.7.2. Scheduled for removal in v10.0.0, use IDictionary<string, IPortalAliasInfo> instead")]
public class PortalAliasCollection

Next Steps

I have 1 major question about API deprecation to be clarified. Other than that, anything that I marked with a ✅ I will be implementing in the PR. Anything I marked with a ❌ I believe is out of scope.

[mitchelsellers]

Regarding deprecating methods. Let’s keep the scope here simple and just flag the fields removed as Obsolete for removal later.

I agree on all others. I would love to discuss the Nullable stuff at a near future time

[bdukes]

Let's just remove those properties from IPortalInfo, we can talk about deprecating them on PortalInfo separately.

Agreed to handle nullable types separately.

[SkyeHoefling]

I have updated the PR with the requested changes that we have been discussing. Since there are a lot of conversations going on in separate threads I am asking everyone that has reviewed part of this PR to re-review the entire PR. If you feel like something you commented on got resolved and shouldn't be, please just start a new review and we can discuss it.

GitHub wouldn't let me request a re-review of everyone but I would love to get feedback and code reviews from everyone on this thread.

• @mitchelsellers
• @bdukes
• @valadas
• @david-poindexter
• @sleupold

[bdukes]

Looks great @ahoefling!

[mitchelsellers]

Just one thing, I noted a single instance, but there are a few Obsolete comments that need updating, otherwise looks good.

[SkyeHoefling]

When we are extracting interfaces should we be allowing gets/sets on all of these properties? Are we missing properties that should be readonly?

[sleupold]

@ahoefling
for calculated properties, of course.
for all other properties, which shouldn't be modified this would mean, you'd include these properties in the constructor?

[bdukes]

I could see the primary key being read-only, but if it's settable on the underlying object, I'm not inclined to worry too much about it.

[SkyeHoefling]

I have made the requested changes by @mitchelsellers and went through all of the deprecation notices to certify they are correct to the best of my knowledge. Also, I have rebased this branch on the latest changes to develop and believe it is ready to be merged.

If there is any other outstanding work that needs to be done, let me know and I'm happy to make the changes 👍

[bdukes]

🎉 Thanks @ahoefling!

@mitchelsellers any concerns with merging this for 9.7.2? It's ready to merge as far as I'm concerned.

[SkyeHoefling]

If we push this to 9.8.0 we will need to update all the deprecation notices, not a big deal but don't want to lose sight of it.

[mitchelsellers]

No concerns here merging

[Timo-Breumelhof]

In addition to this, the HTTPAlias item then becomes a question, should this be HttpAlias? I believe yes.

Well, I just noticed that any VB code that uses HTTPAlias now crashes and there's no real way around this.

[bdukes]

@Timo-Breumelhof what version of DNN are you compiling against?

[Timo-Breumelhof]

@bdukes well, I once decided it would be better to not compile so one could include the SKO inside a skin package. So it's like many Skin Objects just an ascx and an ascx.vb file. This will change when I finally convert this to c# :-)