From 64da0eef1ff0e2f3c068fead9b1ceea39962a9bf Mon Sep 17 00:00:00 2001 From: "Ronald M. Martin" Date: Thu, 5 Oct 2023 16:46:23 -0400 Subject: [PATCH 1/2] Respond to review comments.Add tests to simulate issue 7739. Generalize the issue to apply to most situations in which inner text is used in Wix 3 to represent condidtions. These all suffer from the mishandling of whitespace. Review other uses of inner text amd pay attention to the treatment of CTEXT nodes. --- src/wix/WixToolset.Converters/WixConverter.cs | 197 +++++++++--- .../ConditionFixture.cs | 282 +++++++++++++++++- 2 files changed, 440 insertions(+), 39 deletions(-) diff --git a/src/wix/WixToolset.Converters/WixConverter.cs b/src/wix/WixToolset.Converters/WixConverter.cs index f3675ecc6..3217a3253 100644 --- a/src/wix/WixToolset.Converters/WixConverter.cs +++ b/src/wix/WixToolset.Converters/WixConverter.cs @@ -1040,7 +1040,7 @@ private void ConvertControlElement(XElement element) { var action = UppercaseFirstChar(xCondition.Attribute("Action")?.Value); if (!String.IsNullOrEmpty(action) && - TryGetInnerText(xCondition, out var text, out comments, comments) && + TryGetInnerText(xCondition, out var text, out comments, true, false, comments) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}Condition' attribute instead.", xCondition.Name.LocalName, action)) { conditions.Add(new KeyValuePair(action, text)); @@ -1237,9 +1237,14 @@ private void ConvertLaunchConditionElement(XElement element) var message = element.Attribute("Message")?.Value; if (!String.IsNullOrEmpty(message) && - TryGetInnerText(element, out var text, out var comments) && + TryGetInnerText(element, out var text, out var comments, true, true) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Launch' element instead.", element.Name.LocalName)) { + if (String.IsNullOrWhiteSpace(text)) + { + text = String.Empty; + } + using (var lab = new ConversionLab(element)) { lab.ReplaceTargetElement(new XElement(LaunchElementName, @@ -1983,8 +1988,14 @@ private void ConvertCustomActionElement(XElement xCustomAction) var xScript = xCustomAction.Attribute("Script"); - if (xScript != null && TryGetInnerText(xCustomAction, out var scriptText, out var comments)) + if (xScript != null && TryGetInnerText(xCustomAction, out var scriptText, out var comments, false, false, new List())) { + if (null != scriptText) + { + char[] whitespaceChars = { ' ', '\t', '\r', '\n' }; + scriptText = scriptText.Trim(whitespaceChars); + } + if (this.OnInformation(ConverterTestType.InnerTextDeprecated, xCustomAction, "Using {0} element text is deprecated. Extract the text to a file and use the 'ScriptSourceFile' attribute to reference it.", xCustomAction.Name.LocalName)) { var scriptFolder = Path.GetDirectoryName(this.SourceFile) ?? String.Empty; @@ -2150,10 +2161,7 @@ private void ConvertInnerTextToAttribute(XElement element, string attributeName) var attribute = element.Attribute(attributeName); if (attribute != null) { - if (!String.IsNullOrWhiteSpace(text)) - { - this.OnError(ConverterTestType.InnerTextDeprecated, attribute, "Using {0} element text is deprecated. Remove the element's text and use only the '{1}' attribute. See the conversion FAQ for more information: https://wixtoolset.org/docs/fourthree/faqs/#converting-packages", element.Name.LocalName, attributeName); - } + this.OnError(ConverterTestType.InnerTextDeprecated, attribute, "Using {0} element text is deprecated. Remove the element's text and use only the '{1}' attribute. See the conversion FAQ for more information: https://wixtoolset.org/docs/fourthree/faqs/#converting-packages", element.Name.LocalName, attributeName); } else if (this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}' attribute instead.", element.Name.LocalName, attributeName)) { @@ -2854,20 +2862,45 @@ private static void RenameElementToStandardDirectory(XElement element) } } - private static bool TryGetInnerText(XElement element, out string value, out List comments) - { - return TryGetInnerText(element, out value, out comments, new List()); - } - - private static bool TryGetInnerText(XElement element, out string value, out List comments, List initialComments) + // This function is used to simultaneously extract the inner text of, and any comments embedded in, an XElement. + // The comments are returned in a list, so there can be several of them. In addition, the code is designed so that + // it can be called in a loop, processing one element at a time, extracting the comments cumulatively, and returning + // the inner text separately with each call. An initial list is passed into the "initialComments" parameter. This + // parameter defaults to null, which is replaced by an empty list. The augmented list is returned through the "comments" + // out parameter. Thiks list is then passed into the "initialComments" paameter the next time around the loop. + // + // The inner text is returned through the "value" out parameter. The function returns true if inner text is found. + // + // There are two other parameters that control the behavior of the function: The "protectQuotes" parameter, with a default + // value of "true", causes single and double quotes to be processed into an appropriate set of escape sequences (escaped with + // a backslash) ap sppropriate for a "Condition" attribute. Contiguous whitespace is coalesced into a single space character + // and leading and trailing whitespace is trimmed. When this paarameter is false, the inner text is returned with all whitespace + // intact. + // + // The other optional parameter, "reportWhitespace", has a default value of "false". When it is true, the value returned by the + // function is true, not only when non-whitespace text is returned in the "value" out parameter, but also if only whitespace was + // found. In this case, the text returned is a single space. If this parameter is false, the text found is trimmed of leading and + // trailing whitespace before being returned and the function only returns true if the returned text is not empty. + // + // There is one other important case in which the function returns true: If a CDATA node is processed, even if all the whitespace removed + // causes the remaining text to be empty, the function returns true. If in fact, the remaining text is empty, the text returned is a single + // space. + private static bool TryGetInnerText(XElement element, + out string value, + out List comments, + bool protectQuotes = true, + bool reportWhitespace = false, + List initialComments = null) { value = null; - comments = null; - var found = false; - + char[] whitespaceChars = { ' ', '\t', '\r', '\n' }; var nodes = element.Nodes().ToList(); - comments = initialComments; - var nonCommentNodes = new List(); + comments = initialComments ?? new List(); ; + char? currentQuote = null; + var inWhitespace = false; + var cDataFound = false; + var whitespaceFound = false; + var sb = new StringBuilder(); foreach (var node in nodes) { @@ -2875,18 +2908,122 @@ private static bool TryGetInnerText(XElement element, out string value, out List { comments.Add(node); } - else + else if (XmlNodeType.CDATA == node.NodeType || XmlNodeType.Text == node.NodeType) { - nonCommentNodes.Add(node); + var isCData = XmlNodeType.CDATA == node.NodeType; + + if (isCData) + { + cDataFound = true; + } + + var text = (node as XText)?.Value; + + if (protectQuotes) + { + var nodeSB = new StringBuilder(); + + foreach (var c in text) + { + char? emit = c; + + // Replace contiguous whitespace with a single space. + if (' ' == c || '\r' == c || '\n' == c || '\t' == c) + { + if (!inWhitespace) + { + inWhitespace = true; + whitespaceFound = true; + emit = ' '; + } + else + { + emit = null; + } + } + else + { + inWhitespace = false; + + if ('\\' == c) + { + // Escape backslash. + nodeSB.Append('\\'); + } + + else if (!isCData) + { + // Escape nested quote. + if ('\'' == c || '"' == c) + { + if (currentQuote.HasValue) + { + if (currentQuote == c) + { + currentQuote = null; + } + else + { + nodeSB.Append('\\'); + } + } + else + { + currentQuote = c; + } + } + } + } + + if (emit.HasValue) + { + nodeSB.Append(emit); + } + } + + text = nodeSB.ToString(); + } + + if (0 < text.Length) + { + text = text.Trim(whitespaceChars); + + if (0 == text.Length && reportWhitespace) + { + text = " "; + } + } + + sb.Append(text); } } - if (nonCommentNodes.Any() && nonCommentNodes.All(e => e.NodeType == XmlNodeType.Text || e.NodeType == XmlNodeType.CDATA)) + var found = false; + + value = sb.ToString(); + + if (0 < value.Length) { - value = String.Join(String.Empty, nonCommentNodes.Cast().Select(TrimTextValue)); found = true; } + value = value.Trim(whitespaceChars); + + if (reportWhitespace && whitespaceFound) + { + found = true; + } + + if (cDataFound) + { + found = true; + + if (0 == value.Length) + { + value = " "; + } + } + return found; } @@ -2910,22 +3047,6 @@ private static void TrimLeadingText(XDocument document) } } - private static string TrimTextValue(XText text) - { - var value = text.Value; - - if (String.IsNullOrEmpty(value)) - { - return String.Empty; - } - else if (text.NodeType == XmlNodeType.CDATA && String.IsNullOrWhiteSpace(value)) - { - return " "; - } - - return value.Trim(); - } - private static void RemoveChildren(XElement element) { var nodes = element.Nodes().ToList(); diff --git a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs index e3563c60c..b985f226e 100644 --- a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs +++ b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs @@ -142,6 +142,49 @@ public void FixControlConditionWithComment() WixAssert.CompareLineByLine(expected, actualLines); } + [Fact] + public void FixControlConditionWithStringConstants() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " ", + " \"x\"==y", + " \"x's\"==y", + " ", + " ", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(4, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + [Fact] public void FixPublishCondition() { @@ -305,6 +348,78 @@ public void FixComponentConditionWithComment() WixAssert.CompareLineByLine(expected, actualLines); } + [Fact] + public void FixMultilineComponentCondition() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " 1<2", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(3, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + + [Fact] + public void FixMultilineComponentConditionWithComment() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " ", + " 1<2 OR ", + " 4<3", + " ", + " ", + " ", + ""); + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(3, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + [Fact] public void FixEmptyCondition() { @@ -480,6 +595,90 @@ public void FixFeatureConditionWithComment() WixAssert.CompareLineByLine(expected, actualLines); } + [Fact] + public void FixMultilineFeatureCondition() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " (NOT Installed AND NOT PERUSER) OR", + " (Installed AND ZOS_SHELL_EXTENSION_INSTALLLED)", + " ", + " ", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(3, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + + [Fact] + public void FixMultilineFeatureConditionWithComment() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " ", + " (NOT Installed AND NOT PERUSER) OR", + " (Installed AND ZOS_SHELL_EXTENSION_INSTALLLED)", + " ", + " ", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(3, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + [Fact] public void FixLaunchConditionInFragment() { @@ -519,7 +718,7 @@ public void FixLaunchConditionInFragment() } [Fact] - public void FixLaunchConditionWithComment() + public void FixLaunchConditionInFragmentWithComment() { var parse = String.Join(Environment.NewLine, "", @@ -558,6 +757,87 @@ public void FixLaunchConditionWithComment() WixAssert.CompareLineByLine(expected, actualLines); } + [Fact] + public void FixMultilineLaunchConditionInFragment() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " 1<2 OR ", + " 4<3", + " ", + " ", + " 1=2", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(4, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + + [Fact] + public void FixMultilineLaunchConditionInFragmentWithComment() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " 1<2 OR", + " 4<3", + " ", + " ", + " 1=2", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + Assert.Equal(4, errors); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + [Fact] public void FixLaunchConditionInProduct() { From 9c1ca4ffdac75a13808f38a19606591c1f321da6 Mon Sep 17 00:00:00 2001 From: "Ronald M. Martin" Date: Thu, 2 Nov 2023 21:18:23 -0400 Subject: [PATCH 2/2] Respond to second code review: Stopped escaping single and double quotes, as well as backslashes when converting inner text to attributes. Eliminated all out parameters of TryGetInnerText in favor of using a context parameter. --- src/wix/WixToolset.Converters/WixConverter.cs | 211 ++++++++++-------- .../ConditionFixture.cs | 65 +++++- 2 files changed, 169 insertions(+), 107 deletions(-) diff --git a/src/wix/WixToolset.Converters/WixConverter.cs b/src/wix/WixToolset.Converters/WixConverter.cs index 3217a3253..f719edf35 100644 --- a/src/wix/WixToolset.Converters/WixConverter.cs +++ b/src/wix/WixToolset.Converters/WixConverter.cs @@ -1033,17 +1033,18 @@ private void ConvertControlElement(XElement element) using (var lab = new ConversionLab(element)) { var xConditions = element.Elements(ConditionElementName).ToList(); - var comments = new List(); + var context = new InnerTextContext(); var conditions = new List>(); foreach (var xCondition in xConditions) { var action = UppercaseFirstChar(xCondition.Attribute("Action")?.Value); + if (!String.IsNullOrEmpty(action) && - TryGetInnerText(xCondition, out var text, out comments, true, false, comments) && + TryGetInnerText(xCondition, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}Condition' attribute instead.", xCondition.Name.LocalName, action)) { - conditions.Add(new KeyValuePair(action, text)); + conditions.Add(new KeyValuePair(action, context.Value)); } } @@ -1062,7 +1063,7 @@ private void ConvertControlElement(XElement element) } lab.RemoveOrphanTextNodes(); - lab.AddCommentsAsSiblings(comments); + lab.AddCommentsAsSiblings(context.Comments); } } @@ -1080,15 +1081,17 @@ private void ConvertComponentElement(XElement element) var xCondition = element.Element(ConditionElementName); if (xCondition != null) { - if (TryGetInnerText(xCondition, out var text, out var comments) && + var context = new InnerTextContext(); + + if (TryGetInnerText(xCondition, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", xCondition.Name.LocalName)) { using (var lab = new ConversionLab(element)) { xCondition.Remove(); - element.Add(new XAttribute("Condition", text)); + element.Add(new XAttribute("Condition", context.Value)); lab.RemoveOrphanTextNodes(); - lab.AddCommentsAsSiblings(comments); + lab.AddCommentsAsSiblings(context.Comments); } } } @@ -1191,16 +1194,18 @@ private void ConvertFeatureElement(XElement element) if (xCondition != null) { var level = xCondition.Attribute("Level")?.Value; + var context = new InnerTextContext(); + if (!String.IsNullOrEmpty(level) && - TryGetInnerText(xCondition, out var text, out var comments) && + TryGetInnerText(xCondition, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Level' element instead.", xCondition.Name.LocalName)) { using (var lab = new ConversionLab(xCondition)) { lab.ReplaceTargetElement(new XElement(LevelElementName, new XAttribute("Value", level), - new XAttribute("Condition", text))); - lab.AddCommentsAsSiblings(comments); + new XAttribute("Condition", context.Value))); + lab.AddCommentsAsSiblings(context.Comments); } } } @@ -1235,22 +1240,23 @@ private void ConvertFileElement(XElement element) private void ConvertLaunchConditionElement(XElement element) { var message = element.Attribute("Message")?.Value; + var context = new InnerTextContext() { ReportWhitespace = true }; if (!String.IsNullOrEmpty(message) && - TryGetInnerText(element, out var text, out var comments, true, true) && + TryGetInnerText(element, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Launch' element instead.", element.Name.LocalName)) { - if (String.IsNullOrWhiteSpace(text)) + if (String.IsNullOrWhiteSpace(context.Value)) { - text = String.Empty; + context.Value = String.Empty; } using (var lab = new ConversionLab(element)) { lab.ReplaceTargetElement(new XElement(LaunchElementName, - new XAttribute("Condition", text), + new XAttribute("Condition", context.Value), new XAttribute("Message", message))); - lab.AddCommentsAsSiblings(comments); + lab.AddCommentsAsSiblings(context.Comments); } } } @@ -1295,7 +1301,8 @@ private void ConvertPermissionExElement(XElement element) var xCondition = element.Element(ConditionElementName); if (xCondition != null) { - if (TryGetInnerText(xCondition, out var text, out var comments) && + var context = new InnerTextContext(); + if (TryGetInnerText(xCondition, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", xCondition.Name.LocalName)) { using (var lab = new ConversionLab(xCondition)) @@ -1304,9 +1311,9 @@ private void ConvertPermissionExElement(XElement element) } using (var lab = new ConversionLab(element)) { - element.Add(new XAttribute("Condition", text)); + element.Add(new XAttribute("Condition", context.Value)); lab.RemoveOrphanTextNodes(); - lab.AddCommentsAsSiblings(comments); + lab.AddCommentsAsSiblings(context.Comments); } } } @@ -1674,22 +1681,24 @@ private void ConvertCustomActionRefElement(XElement element) private void ConvertPublishElement(XElement element) { - if (TryGetInnerText(element, out var text, out var comments) && + var context = new InnerTextContext(); + + if (TryGetInnerText(element, context) && this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", element.Name.LocalName)) { using (var lab = new ConversionLab(element)) { - if ("1" == text) + if ("1" == context.Value) { this.OnInformation(ConverterTestType.PublishConditionOneUnnecessary, element, "Adding Condition='1' on {0} elements is no longer necessary. Remove the Condition attribute.", element.Name.LocalName); } else { - element.Add(new XAttribute("Condition", text)); + element.Add(new XAttribute("Condition", context.Value)); } lab.RemoveOrphanTextNodes(); - lab.AddCommentsAsSiblings(comments); + lab.AddCommentsAsSiblings(context.Comments); } } @@ -1987,13 +1996,14 @@ private void ConvertCustomActionElement(XElement xCustomAction) } var xScript = xCustomAction.Attribute("Script"); + var context = new InnerTextContext() { CoalesceWhitespace = false }; - if (xScript != null && TryGetInnerText(xCustomAction, out var scriptText, out var comments, false, false, new List())) + if (xScript != null && TryGetInnerText(xCustomAction, context)) { - if (null != scriptText) + if (null != context.Value) { char[] whitespaceChars = { ' ', '\t', '\r', '\n' }; - scriptText = scriptText.Trim(whitespaceChars); + context.Value = context.Value.Trim(whitespaceChars); } if (this.OnInformation(ConverterTestType.InnerTextDeprecated, xCustomAction, "Using {0} element text is deprecated. Extract the text to a file and use the 'ScriptSourceFile' attribute to reference it.", xCustomAction.Name.LocalName)) @@ -2003,12 +2013,12 @@ private void ConvertCustomActionElement(XElement xCustomAction) var ext = (xScript.Value == "jscript") ? ".js" : (xScript.Value == "vbscript") ? ".vbs" : ".txt"; var scriptFile = Path.Combine(scriptFolder, id + ext); - File.WriteAllText(scriptFile, scriptText); + File.WriteAllText(scriptFile, context.Value); RemoveChildren(xCustomAction); xCustomAction.Add(new XAttribute("ScriptSourceFile", scriptFile)); - if (comments.Any()) + if (context.Comments.Any()) { var remainingNodes = xCustomAction.NodesAfterSelf().ToList(); var replacementNodes = remainingNodes.Where(e => XmlNodeType.Text != e.NodeType); @@ -2016,7 +2026,7 @@ private void ConvertCustomActionElement(XElement xCustomAction) { node.Remove(); } - foreach (var comment in comments) + foreach (var comment in context.Comments) { xCustomAction.Add(comment); xCustomAction.Add("\n"); @@ -2155,7 +2165,9 @@ private void ConvertWixLocalizationUIElement(XElement element) private void ConvertInnerTextToAttribute(XElement element, string attributeName) { - if (TryGetInnerText(element, out var text, out var comments)) + var context = new InnerTextContext(); + + if (TryGetInnerText(element, context)) { // If the target attribute already exists, error if we have anything more than whitespace. var attribute = element.Attribute(attributeName); @@ -2168,8 +2180,8 @@ private void ConvertInnerTextToAttribute(XElement element, string attributeName) using (var lab = new ConversionLab(element)) { lab.RemoveOrphanTextNodes(); - element.Add(new XAttribute(attributeName, text)); - lab.AddCommentsAsSiblings(comments); + element.Add(new XAttribute(attributeName, context.Value)); + lab.AddCommentsAsSiblings(context.Comments); } } } @@ -2865,38 +2877,38 @@ private static void RenameElementToStandardDirectory(XElement element) // This function is used to simultaneously extract the inner text of, and any comments embedded in, an XElement. // The comments are returned in a list, so there can be several of them. In addition, the code is designed so that // it can be called in a loop, processing one element at a time, extracting the comments cumulatively, and returning - // the inner text separately with each call. An initial list is passed into the "initialComments" parameter. This - // parameter defaults to null, which is replaced by an empty list. The augmented list is returned through the "comments" - // out parameter. Thiks list is then passed into the "initialComments" paameter the next time around the loop. + // the inner text separately with each call. + // + // This function's first parameter refers to an XElement containinng inner text and comments to be extracted. // - // The inner text is returned through the "value" out parameter. The function returns true if inner text is found. + // Other information is passed to and from TryGetInnerText() through the properties of an InnerTextContext object, + // which is passed to this function as its second parameter. // - // There are two other parameters that control the behavior of the function: The "protectQuotes" parameter, with a default - // value of "true", causes single and double quotes to be processed into an appropriate set of escape sequences (escaped with - // a backslash) ap sppropriate for a "Condition" attribute. Contiguous whitespace is coalesced into a single space character - // and leading and trailing whitespace is trimmed. When this paarameter is false, the inner text is returned with all whitespace - // intact. + // On entry, the Comments property of the InnerTextContext object contains a List erpresenting the comments + // collected during any previous calls to this function using this InnerTextCntext object. TryGetInnerText() adds + // any comments it finds to this List. // - // The other optional parameter, "reportWhitespace", has a default value of "false". When it is true, the value returned by the - // function is true, not only when non-whitespace text is returned in the "value" out parameter, but also if only whitespace was - // found. In this case, the text returned is a single space. If this parameter is false, the text found is trimmed of leading and - // trailing whitespace before being returned and the function only returns true if the returned text is not empty. + // The inner text is returned through the Value property of the InnerTextContext object. // - // There is one other important case in which the function returns true: If a CDATA node is processed, even if all the whitespace removed - // causes the remaining text to be empty, the function returns true. If in fact, the remaining text is empty, the text returned is a single - // space. - private static bool TryGetInnerText(XElement element, - out string value, - out List comments, - bool protectQuotes = true, - bool reportWhitespace = false, - List initialComments = null) - { - value = null; + // The function, itself, returns true if inner text is found. + // + // There are two other properties of the InnerTextContext object that control the behavior of the function: The CoalesceWhitespace + // property, with a default value of true, causes contiguous whitespace to be coalesced into a single space character and leading and + // trailing whitespace to be trimmed. When this paarameter is false, the inner text is returned with all whitespace intact. + // + // The other property of the InnerTextContext object, ReportWhitespace, has a default value of "false". When this property is + // false, the text found is trimmed of leading and trailing whitespace before being returned and the function only returns true + // if the returned text is not empty. If it is true, the value returned by the function is true, not only when non-whitespace + // text is returned in the Value property, but also when only whitespace is found. In this case, the text returned is a single space. + // + // There is one other important case in which the function returns true: If a CDATA node is processed, even if all the whitespace + // removed causes the remaining text to be empty, the function returns true. If in fact, the remaining text is empty, the text + // returned is a single space. + private static bool TryGetInnerText(XElement element, InnerTextContext context) + { + context.Value = null; char[] whitespaceChars = { ' ', '\t', '\r', '\n' }; var nodes = element.Nodes().ToList(); - comments = initialComments ?? new List(); ; - char? currentQuote = null; var inWhitespace = false; var cDataFound = false; var whitespaceFound = false; @@ -2906,7 +2918,7 @@ private static bool TryGetInnerText(XElement element, { if (XmlNodeType.Comment == node.NodeType) { - comments.Add(node); + context.Comments.Add(node); } else if (XmlNodeType.CDATA == node.NodeType || XmlNodeType.Text == node.NodeType) { @@ -2919,7 +2931,7 @@ private static bool TryGetInnerText(XElement element, var text = (node as XText)?.Value; - if (protectQuotes) + if (context.CoalesceWhitespace) { var nodeSB = new StringBuilder(); @@ -2944,35 +2956,6 @@ private static bool TryGetInnerText(XElement element, else { inWhitespace = false; - - if ('\\' == c) - { - // Escape backslash. - nodeSB.Append('\\'); - } - - else if (!isCData) - { - // Escape nested quote. - if ('\'' == c || '"' == c) - { - if (currentQuote.HasValue) - { - if (currentQuote == c) - { - currentQuote = null; - } - else - { - nodeSB.Append('\\'); - } - } - else - { - currentQuote = c; - } - } - } } if (emit.HasValue) @@ -2988,7 +2971,7 @@ private static bool TryGetInnerText(XElement element, { text = text.Trim(whitespaceChars); - if (0 == text.Length && reportWhitespace) + if (0 == text.Length && context.ReportWhitespace) { text = " "; } @@ -3000,16 +2983,16 @@ private static bool TryGetInnerText(XElement element, var found = false; - value = sb.ToString(); + context.Value = sb.ToString(); - if (0 < value.Length) + if (0 < context.Value.Length) { found = true; } - value = value.Trim(whitespaceChars); + context.Value = context.Value.Trim(whitespaceChars); - if (reportWhitespace && whitespaceFound) + if (context.ReportWhitespace && whitespaceFound) { found = true; } @@ -3018,9 +3001,9 @@ private static bool TryGetInnerText(XElement element, { found = true; - if (0 == value.Length) + if (0 == context.Value.Length) { - value = " "; + context.Value = " "; } } @@ -3096,6 +3079,44 @@ private static bool WasImplicitlyStringTyped(string value) return true; } + // This class represents the context in which TryGetInnerText() is called. + // + // Upon return from TryGetInnerText(), the Value property represents a string comprisiing the non-comment portion of an XML element's inner text. + // + // The Comments property always returns a (possibly empty) List representing any comments that have been accumulated from previous calls + // to TryGetInnerText(), using the same InnerTextContext object. Even if the intended call to TryGetInnerText is short-circuited, the Comments + // property still returns the intended List. + // + // Upon entry to TryGetInnerText(), the CoalesceWhitespace and reportWhitespace properties contain values that control the behavior of TryGetInnerContext(), + // as explained in the introductory comments of that function. + private class InnerTextContext + { + internal string Value { get; set; } = null; + internal List comments = null; + + internal List Comments + { + get + { + if (null == this.comments) + { + this.comments = new List(); + } + + return this.comments; + } + + set => this.comments = value; + } + + internal bool CoalesceWhitespace { get; set; } = true; + internal bool ReportWhitespace { get; set; } = false; + + internal InnerTextContext() + { + } + } + /// /// Converter test types. These are used to condition error messages down to warnings. /// diff --git a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs index b985f226e..f9b606732 100644 --- a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs +++ b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs @@ -21,8 +21,8 @@ public void FixControlCondition() " ", " ", " ", - " x=y", - " a<>b", + " $(var.x)=$(var.y)", + " $(var.a)<>$(var.b)", " ", " ", " ", @@ -35,7 +35,7 @@ public void FixControlCondition() " ", " ", " ", - " ", + " ", " ", " ", " ", @@ -64,8 +64,8 @@ public void FixDoubleControlCondition() " ", " ", " ", - " x=y", - " a<>b", + " $(var.x)=$(var.y)", + " $(var.a)<>$(var.b)", " ", " ", " ", @@ -78,7 +78,7 @@ public void FixDoubleControlCondition() " ", " ", " ", - " ", + " ", " ", " ", " ", @@ -107,8 +107,8 @@ public void FixControlConditionWithComment() " ", " ", " ", - " x=y", - " a<>b", + " $(var.x)=$(var.y)", + " $(var.a)<>$(var.b)", " ", " ", " ", @@ -123,7 +123,7 @@ public void FixControlConditionWithComment() " ", " ", " ", - " ", + " ", " ", " ", " ", @@ -142,6 +142,47 @@ public void FixControlConditionWithComment() WixAssert.CompareLineByLine(expected, actualLines); } + [Fact] + public void DemonstrateTheHandlingOfQuotedBackslashesInInnerText() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + " ", + " ", + " ", + " \"\\\"=$(var.separator)", + " ", + " ", + " ", + " ", + ""); + + var expected = new[] + { + "", + " ", + " ", + " ", + " ", + " ", + " ", + " ", + "" + }; + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertDocument(document); + + var actualLines = UnformattedDocumentLines(document); + WixAssert.CompareLineByLine(expected, actualLines); + } + [Fact] public void FixControlConditionWithStringConstants() { @@ -152,8 +193,8 @@ public void FixControlConditionWithStringConstants() " ", " ", " ", - " \"x\"==y", - " \"x's\"==y", + " \"x\"=$(var.y)", + " \"x's\"=$(var.y)", " ", " ", " ", @@ -166,7 +207,7 @@ public void FixControlConditionWithStringConstants() " ", " ", " ", - " ", + " ", " ", " ", " ",