WIXFEAT:4763 Change "string" variable type to literal and add "formatted".

6 files changed, 122 insertions, 40 deletions

diff --git a/src/WixToolset.Core.Burn/Bind/SetVariableSearchFacade.cs b/src/WixToolset.Core.Burn/Bind/SetVariableSearchFacade.cs
index fb6f72dd..e88f26ef 100644
--- a/src/WixToolset.Core.Burn/Bind/SetVariableSearchFacade.cs
+++ b/src/WixToolset.Core.Burn/Bind/SetVariableSearchFacade.cs
@@ -21,10 +21,25 @@ namespace WixToolset.Core.Burn
 
             base.WriteXml(writer);
 
-            if (this.SetVariableSymbol.Type != null)
+            if (this.SetVariableSymbol.Type != WixBundleVariableType.Unknown)
             {
                 writer.WriteAttributeString("Value", this.SetVariableSymbol.Value);
-                writer.WriteAttributeString("Type", this.SetVariableSymbol.Type);
+
+                switch (this.SetVariableSymbol.Type)
+                {
+                    case WixBundleVariableType.Formatted:
+                        writer.WriteAttributeString("Type", "formatted");
+                        break;
+                    case WixBundleVariableType.Numeric:
+                        writer.WriteAttributeString("Type", "numeric");
+                        break;
+                    case WixBundleVariableType.String:
+                        writer.WriteAttributeString("Type", "string");
+                        break;
+                    case WixBundleVariableType.Version:
+                        writer.WriteAttributeString("Type", "version");
+                        break;
+                }
             }
 
             writer.WriteEndElement();
diff --git a/src/WixToolset.Core.Burn/Bundles/CreateBurnManifestCommand.cs b/src/WixToolset.Core.Burn/Bundles/CreateBurnManifestCommand.cs
index 05b15ab6..6eafcdd9 100644
--- a/src/WixToolset.Core.Burn/Bundles/CreateBurnManifestCommand.cs
+++ b/src/WixToolset.Core.Burn/Bundles/CreateBurnManifestCommand.cs
@@ -131,10 +131,25 @@ namespace WixToolset.Core.Burn.Bundles
                 {
                     writer.WriteStartElement("Variable");
                     writer.WriteAttributeString("Id", variable.Id.Id);
-                    if (null != variable.Type)
+                    if (variable.Type != WixBundleVariableType.Unknown)
                     {
                         writer.WriteAttributeString("Value", variable.Value);
-                        writer.WriteAttributeString("Type", variable.Type);
+
+                        switch (variable.Type)
+                        {
+                            case WixBundleVariableType.Formatted:
+                                writer.WriteAttributeString("Type", "formatted");
+                                break;
+                            case WixBundleVariableType.Numeric:
+                                writer.WriteAttributeString("Type", "numeric");
+                                break;
+                            case WixBundleVariableType.String:
+                                writer.WriteAttributeString("Type", "string");
+                                break;
+                            case WixBundleVariableType.Version:
+                                writer.WriteAttributeString("Type", "version");
+                                break;
+                        }
                     }
                     writer.WriteAttributeString("Hidden", variable.Hidden ? "yes" : "no");
                     writer.WriteAttributeString("Persisted", variable.Persisted ? "yes" : "no");
diff --git a/src/WixToolset.Core/Compiler_Bundle.cs b/src/WixToolset.Core/Compiler_Bundle.cs
index d73db84d..33467dda 100644
--- a/src/WixToolset.Core/Compiler_Bundle.cs
+++ b/src/WixToolset.Core/Compiler_Bundle.cs
@@ -2141,7 +2141,7 @@ namespace WixToolset.Core
                         case "":
                             break;
                         default:
-                            this.Core.Write(ErrorMessages.IllegalAttributeValue(sourceLineNumbers, node.Name.LocalName, attrib.Name.LocalName, value, "button", "yes", "no"));
+                            this.Core.Write(ErrorMessages.IllegalAttributeValue(sourceLineNumbers, node.Name.LocalName, attrib.Name.LocalName, value, "always", "yes", "no"));
                             break;
                         }
                         break;
@@ -3043,7 +3043,7 @@ namespace WixToolset.Core
             string condition = null;
             string after = null;
             string value = null;
-            string type = null;
+            string typeValue = null;
 
             foreach (var attrib in node.Attributes())
             {
@@ -3067,7 +3067,7 @@ namespace WixToolset.Core
                             value = this.Core.GetAttributeValue(sourceLineNumbers, attrib);
                             break;
                         case "Type":
-                            type = this.Core.GetAttributeValue(sourceLineNumbers, attrib);
+                            typeValue = this.Core.GetAttributeValue(sourceLineNumbers, attrib);
                             break;
 
                         default:
@@ -3081,13 +3081,13 @@ namespace WixToolset.Core
                 }
             }
 
-            type = this.ValidateVariableTypeWithValue(sourceLineNumbers, type, value);
+            var type = this.ValidateVariableTypeWithValue(sourceLineNumbers, node, typeValue, value);
 
             this.Core.ParseForExtensionElements(node);
 
             if (id == null)
             {
-                id = this.Core.CreateIdentifier("sbv", variable, condition, after, value, type);
+                id = this.Core.CreateIdentifier("sbv", variable, condition, after, value, type.ToString());
             }
 
             this.Core.CreateWixSearchSymbol(sourceLineNumbers, node.Name.LocalName, id, variable, condition, after);
@@ -3113,7 +3113,7 @@ namespace WixToolset.Core
             string name = null;
             var persisted = false;
             string value = null;
-            string type = null;
+            string typeValue = null;
 
             foreach (var attrib in node.Attributes())
             {
@@ -3140,7 +3140,7 @@ namespace WixToolset.Core
                         value = this.Core.GetAttributeValue(sourceLineNumbers, attrib, EmptyRule.CanBeEmpty);
                         break;
                     case "Type":
-                        type = this.Core.GetAttributeValue(sourceLineNumbers, attrib);
+                        typeValue = this.Core.GetAttributeValue(sourceLineNumbers, attrib);
                         break;
                     default:
                         this.Core.UnexpectedAttribute(node, attrib);
@@ -3162,7 +3162,7 @@ namespace WixToolset.Core
                 this.Core.Write(ErrorMessages.ReservedNamespaceViolation(sourceLineNumbers, node.Name.LocalName, "Name", "Wix"));
             }
 
-            type = this.ValidateVariableTypeWithValue(sourceLineNumbers, type, value);
+            var type = this.ValidateVariableTypeWithValue(sourceLineNumbers, node, typeValue, value);
 
             this.Core.ParseForExtensionElements(node);
 
@@ -3178,46 +3178,67 @@ namespace WixToolset.Core
             }
         }
 
-        private string ValidateVariableTypeWithValue(SourceLineNumber sourceLineNumbers, string type, string value)
+        private WixBundleVariableType ValidateVariableTypeWithValue(SourceLineNumber sourceLineNumbers, XElement node, string typeValue, string value)
         {
-            var newType = type;
-            if (newType == null && value != null)
+            WixBundleVariableType type;
+            switch (typeValue)
+            {
+                case "formatted":
+                    type = WixBundleVariableType.Formatted;
+                    break;
+                case "numeric":
+                    type = WixBundleVariableType.Numeric;
+                    break;
+                case "string":
+                    type = WixBundleVariableType.String;
+                    break;
+                case "version":
+                    type = WixBundleVariableType.Version;
+                    break;
+                case null:
+                    type = WixBundleVariableType.Unknown;
+                    break;
+                default:
+                    this.Core.Write(ErrorMessages.IllegalAttributeValue(sourceLineNumbers, node.Name.LocalName, "Type", typeValue, "formatted", "numeric", "string", "version"));
+                    return WixBundleVariableType.Unknown;
+            }
+
+            if (type != WixBundleVariableType.Unknown)
             {
-                // Infer the type from the current value...
-                if (value.StartsWith("v", StringComparison.OrdinalIgnoreCase))
+                if (value == null)
                 {
-                    // Version constructor does not support simple "v#" syntax so check to see if the value is
-                    // non-negative real quick.
-                    if (Int32.TryParse(value.Substring(1), NumberStyles.None, CultureInfo.InvariantCulture.NumberFormat, out var _))
-                    {
-                        newType = "version";
-                    }
-                    else if (Version.TryParse(value.Substring(1), out var _))
-                    {
-                        newType = "version";
-                    }
+                    this.Core.Write(ErrorMessages.ExpectedAttribute(sourceLineNumbers, "Variable", "Value", "Type"));
                 }
 
-                // Not a version, check for numeric.
-                if (newType == null)
+                return type;
+            }
+            else if (value == null)
+            {
+                return type;
+            }
+
+            // Infer the type from the current value...
+            if (value.StartsWith("v", StringComparison.OrdinalIgnoreCase))
+            {
+                // Version constructor does not support simple "v#" syntax so check to see if the value is
+                // non-negative real quick.
+                if (Int32.TryParse(value.Substring(1), NumberStyles.None, CultureInfo.InvariantCulture.NumberFormat, out var _))
                 {
-                    if (Int64.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture.NumberFormat, out var _))
-                    {
-                        newType = "numeric";
-                    }
-                    else
-                    {
-                        newType = "string";
-                    }
+                    return WixBundleVariableType.Version;
+                }
+                else if (Version.TryParse(value.Substring(1), out var _))
+                {
+                    return WixBundleVariableType.Version;
                 }
             }
 
-            if (value == null && newType != null)
+            // Not a version, check for numeric.
+            if (Int64.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture.NumberFormat, out var _))
             {
-                this.Core.Write(ErrorMessages.ExpectedAttribute(sourceLineNumbers, "Variable", "Value", "Type"));
+                return WixBundleVariableType.Numeric;
             }
 
-            return newType;
+            return WixBundleVariableType.String;
         }
 
         private class RemotePayload
diff --git a/src/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs b/src/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
index d6c7b091..7a630a36 100644
--- a/src/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
+++ b/src/test/WixToolsetTest.CoreIntegration/BadInputFixture.cs
@@ -32,5 +32,28 @@ namespace WixToolsetTest.CoreIntegration
                 Assert.InRange(result.ExitCode, 2, Int32.MaxValue);
             }
         }
+
+        [Fact]
+        public void BundleVariableWithBadTypeIsRejected()
+        {
+            var folder = TestData.Get(@"TestData\BadInput");
+
+            using (var fs = new DisposableFileSystem())
+            {
+                var baseFolder = fs.GetFolder();
+                var intermediateFolder = Path.Combine(baseFolder, "obj");
+                var wixlibPath = Path.Combine(intermediateFolder, @"test.wixlib");
+
+                var result = WixRunner.Execute(new[]
+                {
+                    "build",
+                    Path.Combine(folder, "BundleVariable.wxs"),
+                    "-intermediateFolder", intermediateFolder,
+                    "-o", wixlibPath,
+                });
+
+                Assert.Equal(21, result.ExitCode);
+            }
+        }
     }
 }
diff --git a/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/BundleVariable.wxs b/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/BundleVariable.wxs
new file mode 100644
index 00000000..293b379f
--- /dev/null
+++ b/src/test/WixToolsetTest.CoreIntegration/TestData/BadInput/BundleVariable.wxs
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<Wix xmlns="http://wixtoolset.org/schemas/v4/wxs"
+     xmlns:ex="http://www.example.com/scheams/v1/wxs">
+    <Fragment>
+        <Variable Name="BadType" Type="doesnotexist" Value="dne" />
+    </Fragment>
+</Wix>
diff --git a/src/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj b/src/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
index 601a66e7..f4aab391 100644
--- a/src/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
+++ b/src/test/WixToolsetTest.CoreIntegration/WixToolsetTest.CoreIntegration.csproj
@@ -22,6 +22,7 @@
     <Content Include="TestData\AppSearch\NestedDirSearchUnderRegSearch.msi" CopyToOutputDirectory="PreserveNewest" />
     <Content Include="TestData\AppSearch\RegistrySearch.wxs" CopyToOutputDirectory="PreserveNewest" />
     <Content Include="TestData\BadEnsureTable\BadEnsureTable.wxs" CopyToOutputDirectory="PreserveNewest" />
+    <Content Include="TestData\BadInput\BundleVariable.wxs" CopyToOutputDirectory="PreserveNewest" />
     <Content Include="TestData\BadInput\RegistryKey.wxs" CopyToOutputDirectory="PreserveNewest" />
     <Content Include="TestData\BindVariables\DefaultedVariable.wxs" CopyToOutputDirectory="PreserveNewest" />
     <Content Include="TestData\BindVariables\data\test.txt" CopyToOutputDirectory="PreserveNewest" />