Default XmlPeek, XmlPoke, and XslTransformation to DtdProcessing.Prohibit

[vijaya-lakshmi-venkatraman]

Fixes #5817

Context

XMLReaderSettings has default value of Prohibit while XMLPeek, XMLPoke and XSLTransformation files have Ignore set. This has been changed to Prohibit

Changes Made

Changed the default value of XMLReaderSettings from Ignore to Prohibit

[dnfadmin]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

:x: vijaya-lakshmi-venkatraman sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[vijaya-lakshmi-venkatraman]

Thank you for the contribution! Do you think it would be useful for XmlPoke and XslTransformation to have the ProhibitDtd property, similar to XmlPeek, so developers can switch to the old behavior if needed?

👍
I can include that property in XMLPoke and XSLTransformation. Do I also have to make changes similar to this in these files so the value is picked up based on what's set on the property? DtdProcessing = prohibitDtd ? DtdProcessing.Prohibit : DtdProcessing.Ignore

[ladipro]

Do I also have to make changes similar to this in these files so the value is picked up based on what's set on the property? DtdProcessing = prohibitDtd ? DtdProcessing.Prohibit : DtdProcessing.Ignore

I'll defer to @BenVillalobos to confirm. In general, I think that we should strive for

1. Consistency. By default, all these tasks should behave the same with respect to DTD processing.
2. Backward compatibility. Unless we are 100% sure that the previous setting was useless, there should be a way for users to bring it back if they depend on it.

In spirit of 2., is it safe to unconditionally change the behavior when loading namespaces (in XmlPeek and XmlPoke) and XSLT parameters (in XsltTransform)?

[benvillalobos]

Backward compatibility. Unless we are 100% sure that the previous setting was useless, there should be a way for users to bring it back if they depend on it.

The classic msbuild 'gotcha'. It's public so theoretically tons of devs could be using it, but we have no way to tell and should assume worst case scenario and provide an opt-into original behavior. Giving XmlPoke and XslTransformation a public ProhibitDtd boolean (defaulted to true) is a good idea in that respect.

So to directly answer this:

Do I also have to make changes similar to this in these files so the value is picked up based on what's set on the property? DtdProcessing = prohibitDtd ? DtdProcessing.Prohibit : DtdProcessing.Ignore

Yep! Each class should have the ProhibitDtd boolean and have them set DtdProcessing to either Prohibit or Ignore based on that boolean.

[Forgind]

@vijaya-lakshmi-venkatraman, do you know when you'll have a chance to get back to this?

[vijaya-lakshmi-venkatraman]

@vijaya-lakshmi-venkatraman, do you know when you'll have a chance to get back to this?

@Forgind Apologies for not updating you. I have been trying to get approval from my organization for signing the CLA (which seems to be required for merging the changes). Please can I get a week's time?

[Forgind]

@vijaya-lakshmi-venkatraman Of course! I'm just making sure everything open is still in progress. Take all the time you need.

[Forgind]

Hi @vijaya-lakshmi-venkatraman, is this still on your radar? Has your organization said anything positive or negative on signing the CLA?

[vijaya-lakshmi-venkatraman]

Hi @vijaya-lakshmi-venkatraman, is this still on your radar? Has your organization said anything positive or negative on signing the CLA?

My deepest apologies to keep this open for so long. Yes! There has been some pointers on who can sign the CLA. Unfortunately the point of contact to authorize my CLA has been off sick. I will need a little more time to sort this admin part.

Reg the code changes, the changes required is

1. public bool ProhibitDtd { get; set; } = true; on all files
2. Remove default value of true in the methods Is there anything else needed?

[Forgind]

No worries! I hope your contact feels better (and I hope it isn't covid).

I think that's right, though I haven't looked at this in a while, so I don't remember for sure.

[rainersigwald]

While you're getting approval I'm going to convert this to draft so it's more obvious that it's not ready yet.

[vijaya-lakshmi-venkatraman]

Hi @rainersigwald I accidentally deleted my repository fork. However, I do have a local copy of the changes. I will merge them to my latest fork Please can you let me know where the latest Corporate CLA can be found (So I can get it signed)?

[Forgind]

If you deleted your fork, it's probably easier just to make a new PR with this content. Then the bot will pop up and let you sign from there.

[vijaya-lakshmi-venkatraman]

Thank you I am able to see the contents of the CLA Can I just request the authorized signatory of our organization sign the CLA on the link (eg: https://cla.dotnetfoundation.org/dotnet/msbuild?pullRequest=6418) or is there another process for Corporate CLA?

[rainersigwald]

@terrajobst do you know how to advise @vijaya-lakshmi-venkatraman on this question?

Can I just request the authorized signatory of our organization sign the CLA on the link (eg: https://cla.dotnetfoundation.org/dotnet/msbuild?pullRequest=6418) or is there another process for Corporate CLA?

[vijaya-lakshmi-venkatraman]

@terrajobst Please can you help with the Corporate CLA process?