Allows optionally removing rel from privacy and terms #5054

Andy9999 · 2022-03-20T15:00:32Z

Fix for issue #5047.

This will allow to optionally remove the rel attribute from privacy and terms links. Default is still "rel=nofollow" but it could now either be changed or removed entirely from the ascx file.

Unfortunately the asp.net control doesn't have a rel attribute, so we have to add it ourselves.

Added Rel attribute to Privacy and Terms class and added rel="nofollow" to privacy.ascx and terms.ascx skin file.

Summary

Fix for issue dnnsoftware#5047. This will allow to optionally remove the rel attribute from privacy and terms links. Default is still "rel=nofollow" but it could now either be changed or removed entirely from the ascx file. Unfortunately the asp.net control doesn't have a rel attribute, so we have to add it ourselves. Added Rel attribute to Privacy and Terms class and added rel="nofollow" to privacy.ascx and terms.ascx skin file.

valadas

Unless I am misunderstanding something, this will not make it configurable, it will always be rel="nofollow" this way...

valadas · 2022-03-20T20:20:18Z

Also, we use the PR titles mainly to write the release notes, so I changed the title to explain what the PR does.

Andy9999 · 2022-03-20T21:50:19Z

Unless I am misunderstanding something, this will not make it configurable, it will always be rel="nofollow" this way...

Yes, correct. This is the current behavior. The difference is that it could now be removed from the ascx file if desired, without recompiling.

Andy9999 · 2022-03-20T22:03:02Z

Also, we use the PR titles mainly to write the release notes, so I changed the title to explain what the PR does.

OK. Thank you, will do it properly the next time.

valadas · 2022-03-21T00:33:05Z

Yes, correct. This is the current behavior. The difference is that it could now be removed from the ascx file if desired, without recompiling.

But it's not a best practice to edit core files anyway, I was thinking it could be made an option in a parameter like you did but it looks to me like the parameter is not being used this way.

Andy9999 · 2022-03-21T08:42:19Z

Yes, correct. This is the current behavior. The difference is that it could now be removed from the ascx file if desired, without recompiling.

But it's not a best practice to edit core files anyway, I was thinking it could be made an option in a parameter like you did but it looks to me like the parameter is not being used this way.

The reason why rel ended up in the cs file in the first place is because Microsoft's asp hyperlink doesn't have a rel attribute, so it cannot be set from the markup usually. Attempting to set it from the ascx produces an error because the attribute cannot be mapped to a member of the class. I found this workaround on stackexchange for example.

The (in my opinion) better alternative was to add the missing attribute to the class(es). It works. I tested it yesterday. With the change in place, it will still render the rel as nofollow. When removing it from the ascx it will disappear entirely, same if setting it to some other value than nofollow. It looks a bit strange because the classes member Rel is not used anywhere, but it will still get rendered (magically). One could use Rel though in the class implementation but there is no need.

bdukes

As @valadas said, our goal would be to allow theme authors to override this without making any changes to core files. I think this approach with my edits should work. Thanks!

DNN Platform/Website/admin/Skins/Privacy.ascx.cs

bdukes · 2022-03-21T13:34:05Z

DNN Platform/Website/admin/Skins/Privacy.ascx.cs

@@ -58,7 +60,6 @@ protected override void OnLoad(EventArgs e)
                }

                this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);


Suggested change

this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);

this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);

this.hypPrivacy.Attribute["rel"] = this.Rel;

DNN Platform/Website/admin/Skins/Terms.ascx.cs

bdukes · 2022-03-21T13:34:46Z

DNN Platform/Website/admin/Skins/Terms.ascx.cs

@@ -58,8 +60,6 @@ protected override void OnLoad(EventArgs e)
                }

                this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);


Suggested change

this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);

this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);

this.hypTerms.Attributes["rel"] = this.Rel;

DNN Platform/Website/admin/Skins/privacy.ascx

DNN Platform/Website/admin/Skins/terms.ascx

valadas · 2022-03-21T16:02:41Z

With the suggested changes, than this does make sense to me.
@Andy9999 if you can accept those, this looks good to me.

Andy9999 · 2022-03-21T16:43:09Z

Thanks. That change would not really have been needed. That's why I changed the ascx file. Rel is now initialized twice, once from the rel attribute in the HTML markup and once from the code behind file. But the markup will win.

But anyways. Main thing is it works. :)

PS: Just saw you also removed it from the markup again. No complains. The way it is implemented now is maybe more compatible in case someone already made (some other) changes to the markup file, but I think the ascx would be overwritten anyways when updating. And then the original modified version could be simply copied over the one overwritten by the installer. I always have to create a backup and then do a file compare to not miss out any of these type changes after updating, because there is no list of what got overwritten.

Andy9999 · 2022-03-21T17:25:50Z

I am wondering why the markup for login.ascx or user.ascx works without the class in the code behind having added a Rel member? Form where do these inherit that member? (https://github.com/dnnsoftware/Dnn.Platform/blob/develop/DNN%20Platform/Website/admin/Skins/login.ascx)

valadas · 2022-03-21T17:29:59Z

The way I see it, the login one has it hardcoded in the view and it cannot be customized (without editing the core files, which we do not recommend).

Andy9999 · 2022-03-21T17:33:20Z

OK. Interesting to see the devs perspective. Am more in the user perspective here. :)

I made a couple of such changes and merge them during each update. From my perspective this is totally acceptable. And of course I prefer changing some (core) markup files over having to recompile DNN.

valadas · 2022-03-21T17:33:29Z

Rel is now initialized twice, once from the rel attribute in the HTML markup and once from the code behind file. But the markup will win

If you accept the suggestions @bdukes made above by clicking the "Commit suggestion" on each, then it will have a default and theme developers will have a setting on the skin object to override that default. The class will change it at runtime accordingly thus no longer needing to modify the core ascx file, it will be simply a theme implementation that does not get overridden upon upgrades.

bdukes · 2022-03-21T17:34:29Z

@Andy9999 the intention of the Platform is that you should not have any changes which get overwritten by an upgrade. Any changes that you want which would be overwritten by an upgrade should use an extension point which DNN does not overwrite (or a new extension point if one doesn't exist, e.g. adding a Rel property to the Privacy theme object control)

ASP.NET web controls can have their Attributes collection set via the ascx markup if an attribute in the markup doesn't match an existing property on the control's class. So the asp:HyperLink control doesn't need any special support to set rel="nofollow", it just renders it directly as an attribute on the rendered <a> tag. We want the Privacy control to allow a theme developer to set <dnn:Privacy runat="server" rel=""/> (which sets the Rel property defined on the class), which then sets the "rel" attribute on the asp:HyperLink, resulting in rel="" being rendered).

Andy9999 · 2022-03-21T17:36:22Z

Rel is now initialized twice, once from the rel attribute in the HTML markup and once from the code behind file. But the markup will win

If you accept the suggestions @bdukes made above by clicking the "Commit suggestion" on each, then it will have a default and theme developers will have a setting on the skin object to override that default. The class will change it at runtime accordingly thus no longer needing to modify the core ascx file, it will be simply a theme implementation that does not get overridden upon upgrades.

Oh....wasn't aware of that.

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Andy9999 · 2022-03-21T17:40:34Z

The way I see it, the login one has it hardcoded in the view and it cannot be customized (without editing the core files, which we do not recommend).

Yeah, not having any changed core files would be my long term goal as well. :)

Andy9999 · 2022-03-21T17:43:38Z

@Andy9999 the intention of the Platform is that you should not have any changes which get overwritten by an upgrade. Any changes that you want which would be overwritten by an upgrade should use an extension point which DNN does not overwrite (or a new extension point if one doesn't exist, e.g. adding a Rel property to the Privacy theme object control)

ASP.NET web controls can have their Attributes collection set via the ascx markup if an attribute in the markup doesn't match an existing property on the control's class. So the asp:HyperLink control doesn't need any special support to set rel="nofollow", it just renders it directly as an attribute on the rendered <a> tag. We want the Privacy control to allow a theme developer to set <dnn:Privacy runat="server" rel=""/> (which sets the Rel property defined on the class), which then sets the "rel" attribute on the asp:HyperLink, resulting in rel="" being rendered).

Thanks....maybe the exception I saw, when adding rel to the markup without that change, was because the code behind attempted to add an attribute that already exists in the markup/skin. I didn't investigate the crash any further and simply removed it from the code behind and set the default value in the markup.

Andy9999 · 2022-03-21T17:46:17Z

Seems I also missed out that Login.Ascx or User.Ascx use hyperlink directly whereas Privacy and Terms have their own classes.

Andy9999 · 2022-03-21T17:52:25Z

To sum it up and if I got your feedback right, you would prefer to have a setting somewhere, e.g. in PortalSettings to turn it on and off?

Then....no core files would have to be touched. Right? Sorry if I am slow. I could make another update next weekend. :)

DNN Platform/Website/admin/Skins/Privacy.ascx.cs

DNN Platform/Website/admin/Skins/Terms.ascx.cs

bdukes · 2022-03-21T18:59:53Z

Sorry, my typo. Fix building now.

Andy9999 · 2022-03-21T19:01:04Z

Very agile!

Andy9999 · 2022-03-21T19:02:08Z

Would probably be easier if we were in the same room or in a chat somehow.

Andy9999 · 2022-03-21T20:01:52Z

Honestly the result now is not really what I had on mind and what the issue title suggests (removal of rel) as a rel="" would remain on each page.

How could that be solved? Any suggestions? I mean we could add some sort of setting if you would prefer that.

I will still stick to my initial code submission and deploy that in our production and I guess I will still have to recompile DNN for the foreseeing future.

Maybe it's my fault somehow in not better describing the issue or something. Don't know.

bdukes · 2022-03-21T20:14:45Z

If you really don't want rel="" you can set the value to null:

<dnn:Terms runat="server" ID="TheTerms" />

<script runat="server">
protected override void OnLoad(EventArgs e)
{
  TheTerms.Rel = null;
}
</script>

Andy9999 · 2022-03-21T20:15:31Z

I think the whole change could now be undone, because that was possible before. :)

It is exactly 100% the same behavior as before, nothing has changed except for moving the "nofollow" a lil bit towards the top of the file to initialize the Rel member/attribute of the class.

Does this change make any sense at all in its current state??

bdukes · 2022-03-21T20:19:39Z

We can add a if (!string.IsNullOrWhiteSpace(this.Rel) { this.hypTerms["rel"] = this.Rel; } wrapper around the defining of the attribute if you want to make a follow-up contribution.

It is now possible to change or remove the rel="nofollow" attribute from the link without making any changes to the core files, but only adjusting your theme. This is a valuable contribution, otherwise we wouldn't have accepted it. I'm sorry if there was miscommunication on what our goals were.

Andy9999 · 2022-03-21T20:20:45Z

We can add a if (!string.IsNullOrWhiteSpace(this.Rel) { this.hypTerms["rel"] = this.Rel; } wrapper around the defining of the attribute if you want to make a follow-up contribution.

It is now possible to change or remove the rel="nofollow" attribute from the link without making any changes to the core files, but only adjusting your theme. This is a valuable contribution, otherwise we wouldn't have accepted it. I'm sorry if there was miscommunication on what our goals were.

I guess this won't help due to the order of execution.

Rel is initialized to nofollow in the codebehind
OnLoad is executed and adds the attribute to the hyperlink (nofollow)
Then Rel is set to whatever is in the markup code without any effect

I just tried to play around with it on my test system and I couldn't change rel at all.

bdukes · 2022-03-21T20:22:56Z

Are you trying to set the Rel property on the <dnn:Terms> control in your theme file?

Andy9999 · 2022-03-21T20:23:32Z

No, maybe that's the culprit! I did it in the Terms.Ascx file, the file you told me to not change. :)

Andy9999 · 2022-03-21T20:24:07Z

I slowly start to understand what we are doing here.

Andy9999 · 2022-03-21T20:24:53Z

I will try it now!

Andy9999 · 2022-03-21T20:34:47Z

Wow! Works perfectly if including your last suggestion. Now it matches the issue title again. Can I still submit it or is it too late now?

Andy9999 · 2022-03-21T20:41:27Z

Just submitted the change and will change the skin files. The way it should be done! Thanks Daniel and Brian.

Andy9999 · 2022-03-21T20:57:30Z

New issue??

bdukes · 2022-03-21T21:02:50Z

No, you can associate it with the same issue. Thanks!

bdukes · 2022-03-21T21:03:47Z

You will have to open a new PR, but it'll be associated to the same issue #5047

Andy9999 · 2022-03-21T21:10:57Z

Like this? #5057

bdukes · 2022-03-21T21:15:10Z

That works 👍🏻

The one thing to do differently is instead of include #5047 in the PR title, in the description include "Fixes #5047" (or "For #5047" if it's not a complete fix). This will let GitHub automatically create a link between the PR and the issue.

Andy9999 · 2022-03-21T21:33:05Z

Thanks.

Andy9999 · 2022-03-21T23:39:06Z

Got it deployed on the production server now along with the other issue (search result category localization). Looks great now! I find nothing to complain about.

Somehow (with my limited understanding) it did not seem to be a big difference if rel would be changed in the Terms/Privacy markup files or the skin. I have to watch out when updating the skin as well and merge changes with the ones we did. We use the awesome EasyDNN themes. But I clearly see the advantage now. :)

When I'm done with the other open issues I can later contribute at least the German translations for it. Along with a "Search.ascx.de-DE.resx". There are some more missing translations for the module settings as well (low priority problem). But one step after the other.

CU you next weekend. :)

Andy9999 · 2022-03-21T23:46:46Z

One final question.....would it make a big difference to initialize the new Rel attribute from the markup instead of having it in the code behind file? And...would this raise more awareness for the skin developers that there is now such an attribute that could be customized? Or would they consult the code behind anyways or get that info from the release notes?

Just a question for future issue of similar nature.

bdukes · 2022-03-22T13:20:10Z

<dnn:Terms runat="server" ID="TheTerms" Rel="" /> is the way you would initialize the property via markup. There isn't a place to do that initialization in the Terms control itself, it has to be in the codebehind.

We probably do need to document this new property by creating a page on https://docs.dnncommunity.org/content/tutorials/themes/theme-objects/index.html#breadcrumb-theme-object-example (e.g. like the breadcrumb docs)

Andy9999 · 2022-03-22T14:16:00Z

Thanks for the patience with me I will memorize that now! Don't know how I ended up at that assumption. I completely forgot about the skin objects and what they are there for, only looking at the rendered HTML.

I did not define a <summary> in the code behind file either. Is that still best practice?

Maybe I can do both next weekend before beginning with the other two issues I wanted to work on.

bdukes · 2022-03-22T15:05:24Z

Doc comments are always appreciated 🙂

valadas · 2022-03-22T15:43:09Z

@Andy9999 documentation about theme objects is also welcome if you have the time https://docs.dnncommunity.org/content/tutorials/themes/theme-objects/index.html

valadas changed the title ~~Fix for issue #5047.~~ Allows optionally removing rel from privacy and terms Mar 20, 2022

valadas requested changes Mar 20, 2022

View reviewed changes

bdukes reviewed Mar 21, 2022

View reviewed changes

valadas added the Type: Enhancement label Mar 21, 2022

valadas added this to the 9.10.3 milestone Mar 21, 2022

Andy9999 and others added 4 commits March 21, 2022 18:37

Update DNN Platform/Website/admin/Skins/Privacy.ascx.cs

4fa439c

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Update DNN Platform/Website/admin/Skins/Terms.ascx.cs

4783633

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Update DNN Platform/Website/admin/Skins/privacy.ascx

22cb52a

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Update DNN Platform/Website/admin/Skins/terms.ascx

da56b4a

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

bdukes reviewed Mar 21, 2022

View reviewed changes

DNN Platform/Website/admin/Skins/Privacy.ascx.cs Show resolved Hide resolved

bdukes reviewed Mar 21, 2022

View reviewed changes

DNN Platform/Website/admin/Skins/Terms.ascx.cs Show resolved Hide resolved

Apply suggestions from code review

6d9b111

valadas merged commit fee726b into dnnsoftware:develop Mar 21, 2022

bdukes linked an issue Mar 21, 2022 that may be closed by this pull request

DNN Platform\Website\admin\Skins\Privacy.ascx.cs & Terms.ascx.cs #5047

Closed

valadas modified the milestones: 9.10.3, 9.11.0 Sep 28, 2022

		@@ -58,7 +60,6 @@ protected override void OnLoad(EventArgs e)
		}

		this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);

	this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);
	this.hypPrivacy.NavigateUrl = this.PortalSettings.PrivacyTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Privacy") : this._navigationManager.NavigateURL(this.PortalSettings.PrivacyTabId);
	this.hypPrivacy.Attribute["rel"] = this.Rel;

		@@ -58,8 +60,6 @@ protected override void OnLoad(EventArgs e)
		}

		this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);

	this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);
	this.hypTerms.NavigateUrl = this.PortalSettings.TermsTabId == Null.NullInteger ? this._navigationManager.NavigateURL(this.PortalSettings.ActiveTab.TabID, "Terms") : this._navigationManager.NavigateURL(this.PortalSettings.TermsTabId);
	this.hypTerms.Attributes["rel"] = this.Rel;

Allows optionally removing rel from privacy and terms #5054

Allows optionally removing rel from privacy and terms #5054

Conversation

Andy9999 commented Mar 20, 2022

Summary

valadas left a comment

Choose a reason for hiding this comment

valadas commented Mar 20, 2022

Andy9999 commented Mar 20, 2022

Andy9999 commented Mar 20, 2022

valadas commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

bdukes left a comment

Choose a reason for hiding this comment

bdukes Mar 21, 2022

Choose a reason for hiding this comment

bdukes Mar 21, 2022

Choose a reason for hiding this comment

valadas commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

Andy9999 commented Mar 21, 2022

valadas commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

valadas commented Mar 21, 2022

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

Andy9999 commented Mar 21, 2022 • edited Loading

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

bdukes commented Mar 21, 2022

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

bdukes commented Mar 21, 2022

Andy9999 commented Mar 21, 2022

Andy9999 commented Mar 21, 2022 • edited Loading

Andy9999 commented Mar 21, 2022

bdukes commented Mar 22, 2022

Andy9999 commented Mar 22, 2022 • edited Loading

bdukes commented Mar 22, 2022 • edited Loading

valadas commented Mar 22, 2022

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 21, 2022 •

edited

Loading

Andy9999 commented Mar 22, 2022 •

edited

Loading

bdukes commented Mar 22, 2022 •

edited

Loading