Apply changes from #11805 and #11806 to v9 (#11904)

* Apply changes from #11805 and #11806 to v9

* Update documentation and cleanup code styling

Author: ronaldbarendse

src/Umbraco.Core/Models/PropertyTagsExtensions.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.Extensions.Logging;
@@ -75,8 +75,7 @@ private static void AssignTags(this IProperty property, IEnumerable<string> tags
                         var updatedTags = currentTags.Union(trimmedTags).ToArray();
                         var updatedValue = updatedTags.Length == 0 ? null : serializer.Serialize(updatedTags);
                         property.SetValue(updatedValue, culture); // json array
-                         break;
-                        property.SetValue(serializer.Serialize(currentTags.Union(trimmedTags).ToArray()), culture); // json array
+                        break;
                 }
             }
             else
@@ -88,7 +87,8 @@ private static void AssignTags(this IProperty property, IEnumerable<string> tags
                         break;
 
                     case TagsStorageType.Json:
-                        property.SetValue(serializer.Serialize(trimmedTags), culture); // json array
+                        var updatedValue = trimmedTags.Length == 0 ? null : serializer.Serialize(trimmedTags);
+                        property.SetValue(updatedValue, culture); // json array
                         break;
                 }
             }

src/Umbraco.Core/PropertyEditors/DataValueEditor.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations;
 using System.Globalization;
@@ -149,84 +149,90 @@ public IEnumerable<ValidationResult> Validate(object value, bool required, strin
         public virtual bool IsReadOnly => false;
 
         /// <summary>
-        /// Used to try to convert the string value to the correct CLR type based on the DatabaseDataType specified for this value editor
+        /// Used to try to convert the string value to the correct CLR type based on the <see cref="ValueType" /> specified for this value editor.
         /// </summary>
-        /// <param name="value"></param>
-        /// <returns></returns>
+        /// <param name="value">The value.</param>
+        /// <returns>
+        /// The result of the conversion attempt.
+        /// </returns>
+        /// <exception cref="System.ArgumentOutOfRangeException">ValueType was out of range.</exception>
         internal Attempt<object> TryConvertValueToCrlType(object value)
         {
-            // if (value is JValue)
-            //     value = value.ToString();
-
-            //this is a custom check to avoid any errors, if it's a string and it's empty just make it null
+            // Ensure empty string values are converted to null
             if (value is string s && string.IsNullOrWhiteSpace(s))
+            {
                 value = null;
+            }
 
+            // Ensure JSON is serialized properly (without indentation or converted to null when empty)
+            if (value is not null && ValueType.InvariantEquals(ValueTypes.Json))
+            {
+                var jsonValue = _jsonSerializer.Serialize(value);
+
+                if (jsonValue.DetectIsEmptyJson())
+                {
+                    value = null;
+                }
+                else
+                {
+                    value = jsonValue;
+                }
+            }
+
+            // Convert the string to a known type
             Type valueType;
-            //convert the string to a known type
             switch (ValueTypes.ToStorageType(ValueType))
             {
                 case ValueStorageType.Ntext:
                 case ValueStorageType.Nvarchar:
                     valueType = typeof(string);
                     break;
-                case ValueStorageType.Integer:
-                    //ensure these are nullable so we can return a null if required
-                    //NOTE: This is allowing type of 'long' because I think json.net will deserialize a numerical value as long
-                    // instead of int. Even though our db will not support this (will get truncated), we'll at least parse to this.
 
+                case ValueStorageType.Integer:
+                    // Ensure these are nullable so we can return a null if required
+                    // NOTE: This is allowing type of 'long' because I think JSON.NEt will deserialize a numerical value as long instead of int
+                    // Even though our DB will not support this (will get truncated), we'll at least parse to this
                     valueType = typeof(long?);
 
-                    //if parsing is successful, we need to return as an Int, we're only dealing with long's here because of json.net, we actually
-                    //don't support long values and if we return a long value it will get set as a 'long' on the Property.Value (object) and then
-                    //when we compare the values for dirty tracking we'll be comparing an int -> long and they will not match.
+                    // If parsing is successful, we need to return as an int, we're only dealing with long's here because of JSON.NET,
+                    // we actually don't support long values and if we return a long value, it will get set as a 'long' on the Property.Value (object) and then
+                    // when we compare the values for dirty tracking we'll be comparing an int -> long and they will not match.
                     var result = value.TryConvertTo(valueType);
+
                     return result.Success && result.Result != null
                         ? Attempt<object>.Succeed((int)(long)result.Result)
                         : result;
 
                 case ValueStorageType.Decimal:
-                    //ensure these are nullable so we can return a null if required
+                    // Ensure these are nullable so we can return a null if required
                     valueType = typeof(decimal?);
                     break;
 
                 case ValueStorageType.Date:
-                    //ensure these are nullable so we can return a null if required
+                    // Ensure these are nullable so we can return a null if required
                     valueType = typeof(DateTime?);
                     break;
+
                 default:
-                    throw new ArgumentOutOfRangeException();
+                    throw new ArgumentOutOfRangeException("ValueType was out of range.");
             }
 
             return value.TryConvertTo(valueType);
         }
 
-        ///  <summary>
-        ///  A method to deserialize the string value that has been saved in the content editor
-        ///  to an object to be stored in the database.
-        ///  </summary>
-        ///  <param name="editorValue"></param>
-        ///  <param name="currentValue">
-        ///  The current value that has been persisted to the database for this editor. This value may be useful for
-        ///  how the value then get's deserialized again to be re-persisted. In most cases it will probably not be used.
-        ///  </param>
-        /// <param name="languageId"></param>
-        /// <param name="segment"></param>
-        /// <returns></returns>
-        ///  <remarks>
-        ///  By default this will attempt to automatically convert the string value to the value type supplied by ValueType.
-        ///
-        ///  If overridden then the object returned must match the type supplied in the ValueType, otherwise persisting the
-        ///  value to the DB will fail when it tries to validate the value type.
-        ///  </remarks>
+        /// <summary>
+        /// A method to deserialize the string value that has been saved in the content editor to an object to be stored in the database.
+        /// </summary>
+        /// <param name="editorValue">The value returned by the editor.</param>
+        /// <param name="currentValue">The current value that has been persisted to the database for this editor. This value may be useful for how the value then get's deserialized again to be re-persisted. In most cases it will probably not be used.</param>
+        /// <returns>The value that gets persisted to the database.</returns>
+        /// <remarks>
+        /// By default this will attempt to automatically convert the string value to the value type supplied by ValueType.
+        /// If overridden then the object returned must match the type supplied in the ValueType, otherwise persisting the
+        /// value to the DB will fail when it tries to validate the value type.
+        /// </remarks>
         public virtual object FromEditor(ContentPropertyData editorValue, object currentValue)
         {
-            //if it's json but it's empty json, then return null
-            if (ValueType.InvariantEquals(ValueTypes.Json) && editorValue.Value != null && editorValue.Value.ToString().DetectIsEmptyJson())
-            {
-                return null;
-            }
-
             var result = TryConvertValueToCrlType(editorValue.Value);
             if (result.Success == false)
             {
@@ -238,64 +244,71 @@ public virtual object FromEditor(ContentPropertyData editorValue, object current
         }
 
         /// <summary>
-        /// A method used to format the database value to a value that can be used by the editor
+        /// A method used to format the database value to a value that can be used by the editor.
         /// </summary>
-        /// <param name="property"></param>
-        /// <param name="dataTypeService"></param>
-        /// <param name="culture"></param>
-        /// <param name="segment"></param>
+        /// <param name="property">The property.</param>
+        /// <param name="culture">The culture.</param>
+        /// <param name="segment">The segment.</param>
         /// <returns></returns>
+        /// <exception cref="System.ArgumentOutOfRangeException">ValueType was out of range.</exception>
         /// <remarks>
-        /// The object returned will automatically be serialized into json notation. For most property editors
-        /// the value returned is probably just a string but in some cases a json structure will be returned.
+        /// The object returned will automatically be serialized into JSON notation. For most property editors
+        /// the value returned is probably just a string, but in some cases a JSON structure will be returned.
         /// </remarks>
         public virtual object ToEditor(IProperty property, string culture = null, string segment = null)
         {
-            var val = property.GetValue(culture, segment);
-            if (val == null) return string.Empty;
+            var value = property.GetValue(culture, segment);
+            if (value == null)
+            {
+                return string.Empty;
+            }
 
             switch (ValueTypes.ToStorageType(ValueType))
             {
                 case ValueStorageType.Ntext:
                 case ValueStorageType.Nvarchar:
-                    //if it is a string type, we will attempt to see if it is json stored data, if it is we'll try to convert
-                    //to a real json object so we can pass the true json object directly to angular!
-                    var asString = val.ToString();
-                    if (asString.DetectIsJson())
+                    // If it is a string type, we will attempt to see if it is JSON stored data, if it is we'll try to convert
+                    // to a real JSON object so we can pass the true JSON object directly to Angular!
+                    var stringValue = value as string ?? value.ToString();
+                    if (stringValue.DetectIsJson())
                     {
                         try
                         {
-                            var json = _jsonSerializer.Deserialize<dynamic>(asString);
-                            return json;
+                            return _jsonSerializer.Deserialize<dynamic>(stringValue);
                         }
                         catch
                         {
-                            //swallow this exception, we thought it was json but it really isn't so continue returning a string
+                            // Swallow this exception, we thought it was JSON but it really isn't so continue returning a string
                         }
                     }
-                    return asString;
+
+                    return stringValue;
+
                 case ValueStorageType.Integer:
                 case ValueStorageType.Decimal:
-                    //Decimals need to be formatted with invariant culture (dots, not commas)
-                    //Anything else falls back to ToString()
-                    var decim = val.TryConvertTo<decimal>();
-                    return decim.Success
-                        ? decim.Result.ToString(NumberFormatInfo.InvariantInfo)
-                        : val.ToString();
+                    // Decimals need to be formatted with invariant culture (dots, not commas)
+                    // Anything else falls back to ToString()
+                    var decimalValue = value.TryConvertTo<decimal>();
+
+                    return decimalValue.Success
+                        ? decimalValue.Result.ToString(NumberFormatInfo.InvariantInfo)
+                        : value.ToString();
+
                 case ValueStorageType.Date:
-                    var date = val.TryConvertTo<DateTime?>();
-                    if (date.Success == false || date.Result == null)
+                    var dateValue = value.TryConvertTo<DateTime?>();
+                    if (dateValue.Success == false || dateValue.Result == null)
                     {
                         return string.Empty;
                     }
-                    //Dates will be formatted as yyyy-MM-dd HH:mm:ss
-                    return date.Result.Value.ToIsoString();
+
+                    // Dates will be formatted as yyyy-MM-dd HH:mm:ss
+                    return dateValue.Result.Value.ToIsoString();
+
                 default:
-                    throw new ArgumentOutOfRangeException();
+                    throw new ArgumentOutOfRangeException("ValueType was out of range.");
             }
         }
 
-
         // TODO: the methods below should be replaced by proper property value convert ToXPath usage!
 
         /// <summary>

src/Umbraco.Infrastructure/PropertyEditors/GridPropertyEditor.cs

@@ -1,12 +1,10 @@
-// Copyright (c) Umbraco.
+// Copyright (c) Umbraco.
 // See LICENSE for more details.
 
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using Microsoft.Extensions.Logging;
 using Newtonsoft.Json;
-using Umbraco.Cms.Core.Hosting;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Media;
 using Umbraco.Cms.Core.Models;
@@ -153,7 +151,8 @@ public override object FromEditor(ContentPropertyData editorValue, object curren
             public override object ToEditor(IProperty property, string culture = null, string segment = null)
             {
                 var val = property.GetValue(culture, segment)?.ToString();
-                if (val.IsNullOrWhiteSpace()) return string.Empty;
+                if (val.IsNullOrWhiteSpace())
+                    return string.Empty;
 
                 var grid = DeserializeGridValue(val, out var rtes, out _);
 
@@ -199,7 +198,7 @@ public IEnumerable<UmbracoEntityReference> GetReferences(object value)
                     _richTextPropertyValueEditor.GetReferences(x.Value)))
                     yield return umbracoEntityReference;
 
-                foreach (var umbracoEntityReference in mediaValues.Where(x=>x.Value.HasValues)
+                foreach (var umbracoEntityReference in mediaValues.Where(x => x.Value.HasValues)
                     .SelectMany(x => _mediaPickerPropertyValueEditor.GetReferences(x.Value["udi"])))
                     yield return umbracoEntityReference;
             }

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyEditor.cs

@@ -208,6 +208,7 @@ public void Handle(ContentCopiedNotification notification)
                     {
                         continue;
                     }
+
                     var sourcePath = _mediaFileManager.FileSystem.GetRelativePath(src);
                     var copyPath = _mediaFileManager.CopyFile(notification.Copy, property.PropertyType, sourcePath);
                     jo["src"] = _mediaFileManager.FileSystem.GetUrl(copyPath);
@@ -273,10 +274,8 @@ private void AutoFillProperties(IContentBase model)
                             // the property value will be the file source eg '/media/23454/hello.jpg' and we
                             // are fixing that anomaly here - does not make any sense at all but... bah...
                             src = svalue;
-                            property.SetValue(JsonConvert.SerializeObject(new
-                            {
-                                src = svalue
-                            }, Formatting.None), pvalue.Culture, pvalue.Segment);
+
+                            property.SetValue(JsonConvert.SerializeObject(new { src = svalue }, Formatting.None), pvalue.Culture, pvalue.Segment);
                         }
                         else
                         {