diff --git a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs index 01812f8469..c0e4cd7032 100644 --- a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs +++ b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs @@ -1,7 +1,11 @@ using System; using System.Collections.Generic; using System.Linq; +using Umbraco.Core.Collections; +using Umbraco.Core.Composing; using Umbraco.Core.Exceptions; +using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Services; namespace Umbraco.Core.Models { @@ -101,35 +105,6 @@ namespace Umbraco.Core.Models } } - /// - /// Validates the content item's properties pass variant rules - /// - /// If the content type is variant, then culture can be either '*' or an actual culture, but neither 'null' nor - /// 'empty'. If the content type is invariant, then culture can be either '*' or null or empty. - public static Property[] ValidateProperties(this IContentBase content, string culture = "*") - { - // select invalid properties - return content.Properties.Where(x => - { - // if culture is null, we validate invariant properties only - // if culture is '*' we validate both variant and invariant properties, automatically - // if culture is specific eg 'en-US' we both too, but explicitly - - var varies = x.PropertyType.VariesByCulture(); - - if (culture == null) - return !(varies || x.IsValid(null)); // validate invariant property, invariant culture - - if (culture == "*") - return !x.IsValid(culture); // validate property, all cultures - - return varies - ? !x.IsValid(culture) // validate variant property, explicit culture - : !x.IsValid(null); // validate invariant property, explicit culture - }) - .ToArray(); - } - public static void SetPublishInfo(this IContent content, string culture, string name, DateTime date) { if (string.IsNullOrWhiteSpace(name)) @@ -191,15 +166,15 @@ namespace Umbraco.Core.Models content.CultureInfos.AddOrUpdate(culture, name, date); } + /// + /// Sets the publishing values for names and properties. + /// + /// + /// + /// A value indicating whether it was possible to publish the names and values for the specified + /// culture(s). The method may fail if required names are not set, but it does NOT validate property data public static bool PublishCulture(this IContent content, string culture = "*") { - return PublishCulture(content, out _, culture); - } - - public static bool PublishCulture(this IContent content, out Property[] invalidProperties, string culture = "*") - { - invalidProperties = null; - culture = culture.NullOrWhiteSpaceAsNull(); // the variation should be supported by the content type properties @@ -208,11 +183,6 @@ namespace Umbraco.Core.Models if (!content.ContentType.SupportsPropertyVariation(culture, "*", true)) throw new NotSupportedException($"Culture \"{culture}\" is not supported by content type \"{content.ContentType.Alias}\" with variation \"{content.ContentType.Variations}\"."); - // the values we want to publish should be valid - invalidProperties = content.ValidateProperties(culture); - if (invalidProperties.Length > 0) - return false; - var alsoInvariant = false; if (culture == "*") // all cultures { diff --git a/src/Umbraco.Core/Models/ContentType.cs b/src/Umbraco.Core/Models/ContentType.cs index 63c758611a..453670253e 100644 --- a/src/Umbraco.Core/Models/ContentType.cs +++ b/src/Umbraco.Core/Models/ContentType.cs @@ -13,7 +13,7 @@ namespace Umbraco.Core.Models [DataContract(IsReference = true)] public class ContentType : ContentTypeCompositionBase, IContentType { - public const bool IsPublishingConst = true; + public const bool SupportsPublishingConst = true; private int _defaultTemplate; private IEnumerable _allowedTemplates; @@ -45,7 +45,7 @@ namespace Umbraco.Core.Models public override ISimpleContentType ToSimple() => new SimpleContentType(this); /// - public override bool IsPublishing => IsPublishingConst; + public override bool SupportsPublishing => SupportsPublishingConst; //Custom comparer for enumerable private static readonly DelegateEqualityComparer> TemplateComparer = new DelegateEqualityComparer>( diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 320f667e07..ed8a098299 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -40,7 +40,7 @@ namespace Umbraco.Core.Models // actually OK as IsPublishing is constant // ReSharper disable once VirtualMemberCallInConstructor - _noGroupPropertyTypes = new PropertyTypeCollection(IsPublishing); + _noGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing); _noGroupPropertyTypes.CollectionChanged += PropertyTypesChanged; _variations = ContentVariation.Nothing; @@ -61,7 +61,7 @@ namespace Umbraco.Core.Models // actually OK as IsPublishing is constant // ReSharper disable once VirtualMemberCallInConstructor - _noGroupPropertyTypes = new PropertyTypeCollection(IsPublishing); + _noGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing); _noGroupPropertyTypes.CollectionChanged += PropertyTypesChanged; _variations = ContentVariation.Nothing; @@ -70,7 +70,7 @@ namespace Umbraco.Core.Models public abstract ISimpleContentType ToSimple(); /// - /// Gets a value indicating whether the content type is publishing. + /// Gets a value indicating whether the content type supports publishing. /// /// /// A publishing content type supports draft and published values for properties. @@ -80,7 +80,7 @@ namespace Umbraco.Core.Models /// the draft or published value of a property returns the same thing, and publishing /// a value property has no effect. /// - public abstract bool IsPublishing { get; } + public abstract bool SupportsPublishing { get; } //Custom comparer for enumerable private static readonly DelegateEqualityComparer> ContentTypeSortComparer = @@ -257,7 +257,7 @@ namespace Umbraco.Core.Models { if (_noGroupPropertyTypes != null) _noGroupPropertyTypes.CollectionChanged -= PropertyTypesChanged; - _noGroupPropertyTypes = new PropertyTypeCollection(IsPublishing, value); + _noGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing, value); _noGroupPropertyTypes.CollectionChanged += PropertyTypesChanged; PropertyTypesChanged(_noGroupPropertyTypes, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); } diff --git a/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs b/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs index ec6f803107..7497c100bc 100644 --- a/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs @@ -226,7 +226,7 @@ namespace Umbraco.Core.Models return null; // create the new group - var group = new PropertyGroup(IsPublishing) { Name = name, SortOrder = 0 }; + var group = new PropertyGroup(SupportsPublishing) { Name = name, SortOrder = 0 }; // check if it is inherited - there might be more than 1 but we want the 1st, to // reuse its sort order - if there are more than 1 and they have different sort diff --git a/src/Umbraco.Core/Models/MediaType.cs b/src/Umbraco.Core/Models/MediaType.cs index f7598b4965..2f087979f7 100644 --- a/src/Umbraco.Core/Models/MediaType.cs +++ b/src/Umbraco.Core/Models/MediaType.cs @@ -10,7 +10,7 @@ namespace Umbraco.Core.Models [DataContract(IsReference = true)] public class MediaType : ContentTypeCompositionBase, IMediaType { - public const bool IsPublishingConst = false; + public const bool SupportsPublishingConst = false; /// /// Constuctor for creating a MediaType with the parent's id. @@ -45,7 +45,7 @@ namespace Umbraco.Core.Models public override ISimpleContentType ToSimple() => new SimpleContentType(this); /// - public override bool IsPublishing => IsPublishingConst; + public override bool SupportsPublishing => SupportsPublishingConst; /// IMediaType IMediaType.DeepCloneWithResetIdentities(string newAlias) => (IMediaType)DeepCloneWithResetIdentities(newAlias); diff --git a/src/Umbraco.Core/Models/MemberType.cs b/src/Umbraco.Core/Models/MemberType.cs index e85aa0e15e..4633775a43 100644 --- a/src/Umbraco.Core/Models/MemberType.cs +++ b/src/Umbraco.Core/Models/MemberType.cs @@ -11,7 +11,7 @@ namespace Umbraco.Core.Models [DataContract(IsReference = true)] public class MemberType : ContentTypeCompositionBase, IMemberType { - public const bool IsPublishingConst = false; + public const bool SupportsPublishingConst = false; //Dictionary is divided into string: PropertyTypeAlias, Tuple: MemberCanEdit, VisibleOnProfile, PropertyTypeId private string _alias; @@ -35,7 +35,7 @@ namespace Umbraco.Core.Models public override ISimpleContentType ToSimple() => new SimpleContentType(this); /// - public override bool IsPublishing => IsPublishingConst; + public override bool SupportsPublishing => SupportsPublishingConst; public override ContentVariation Variations { diff --git a/src/Umbraco.Core/Models/Property.cs b/src/Umbraco.Core/Models/Property.cs index 726ddc1cf5..76349823ac 100644 --- a/src/Umbraco.Core/Models/Property.cs +++ b/src/Umbraco.Core/Models/Property.cs @@ -184,7 +184,7 @@ namespace Umbraco.Core.Models { if (pvalue == null) return null; - return PropertyType.IsPublishing + return PropertyType.SupportsPublishing ? (published ? pvalue.PublishedValue : pvalue.EditedValue) : pvalue.EditedValue; } @@ -244,7 +244,7 @@ namespace Umbraco.Core.Models { if (pvalue == null) return; - if (!PropertyType.IsPublishing) + if (!PropertyType.SupportsPublishing) throw new NotSupportedException("Property type does not support publishing."); var origValue = pvalue.PublishedValue; pvalue.PublishedValue = PropertyType.ConvertAssignedValue(pvalue.EditedValue); @@ -255,7 +255,7 @@ namespace Umbraco.Core.Models { if (pvalue == null) return; - if (!PropertyType.IsPublishing) + if (!PropertyType.SupportsPublishing) throw new NotSupportedException("Property type does not support publishing."); var origValue = pvalue.PublishedValue; pvalue.PublishedValue = PropertyType.ConvertAssignedValue(null); @@ -288,7 +288,7 @@ namespace Umbraco.Core.Models { var (pvalue, _) = GetPValue(culture, segment, true); - if (published && PropertyType.IsPublishing) + if (published && PropertyType.SupportsPublishing) pvalue.PublishedValue = value; else pvalue.EditedValue = value; @@ -332,56 +332,6 @@ namespace Umbraco.Core.Models return (pvalue, change); } - /// - /// Gets a value indicating whether the property has valid values. - /// - internal bool IsValid(string culture = "*", string segment = "*") - { - // TODO: validating values shouldn't be done here, this calls in to IsValidValue - - culture = culture.NullOrWhiteSpaceAsNull(); - segment = segment.NullOrWhiteSpaceAsNull(); - - // if validating invariant/neutral, and it is supported, validate - // (including ensuring that the value exists, if mandatory) - if ((culture == null || culture == "*") && (segment == null || segment == "*") && PropertyType.SupportsVariation(null, null)) - if (!IsValidValue(_pvalue?.EditedValue)) - return false; - - // if validating only invariant/neutral, we are good - if (culture == null && segment == null) - return true; - - // if nothing else to validate, we are good - if ((culture == null || culture == "*") && (segment == null || segment == "*") && !PropertyType.VariesByCulture()) - return true; - - // for anything else, validate the existing values (including mandatory), - // but we cannot validate mandatory globally (we don't know the possible cultures and segments) - - if (_vvalues == null) return culture == "*" || IsValidValue(null); - - var pvalues = _vvalues.Where(x => - PropertyType.SupportsVariation(x.Value.Culture, x.Value.Segment, true) && // the value variation is ok - (culture == "*" || x.Value.Culture.InvariantEquals(culture)) && // the culture matches - (segment == "*" || x.Value.Segment.InvariantEquals(segment))) // the segment matches - .Select(x => x.Value) - .ToList(); - - return pvalues.Count == 0 || pvalues.All(x => IsValidValue(x.EditedValue)); - } - - /// - /// Boolean indicating whether the passed in value is valid - /// - /// - /// True is property value is valid, otherwise false - private bool IsValidValue(object value) - { - // TODO: this shouldn't exist here, the model itself shouldn't be responsible for it's own validation and this requires singleton access - return PropertyType.IsPropertyValueValid(value); - } - protected override void PerformDeepClone(object clone) { base.PerformDeepClone(clone); diff --git a/src/Umbraco.Core/Models/PropertyType.cs b/src/Umbraco.Core/Models/PropertyType.cs index 1d6a3b4fe9..1e950a876c 100644 --- a/src/Umbraco.Core/Models/PropertyType.cs +++ b/src/Umbraco.Core/Models/PropertyType.cs @@ -82,9 +82,22 @@ namespace Umbraco.Core.Models } /// - /// Gets a value indicating whether the content type, owning this property type, is publishing. + /// Gets a value indicating whether the content type owning this property type is publishing. /// - public bool IsPublishing { get; internal set; } + /// + /// A publishing content type supports draft and published values for properties. + /// It is possible to retrieve either the draft (default) or published value of a property. + /// Setting the value always sets the draft value, which then needs to be published. + /// A non-publishing content type only supports one value for properties. Getting + /// the draft or published value of a property returns the same thing, and publishing + /// a value property has no effect. + /// When true, getting the property value returns the edited value by default, but + /// it is possible to get the published value using the appropriate 'published' method + /// parameter. + /// When false, getting the property value always return the edited value, + /// regardless of the 'published' method parameter. + /// + public bool SupportsPublishing { get; internal set; } /// /// Gets of sets the name of the property type. @@ -355,18 +368,6 @@ namespace Umbraco.Core.Models } - // TODO: this and other value validation methods should be a service level (not a model) thing. Changing this to internal for now - /// - /// Determines whether a value is valid for this property type. - /// - internal bool IsPropertyValueValid(object value) - { - var editor = Current.PropertyEditors[_propertyEditorAlias]; // TODO: inject - var configuration = Current.Services.DataTypeService.GetDataType(_dataTypeId).Configuration; // TODO: inject - var valueEditor = editor.GetValueEditor(configuration); - return !valueEditor.Validate(value, Mandatory, ValidationRegExp).Any(); - } - /// /// Sanitizes a property type alias. /// diff --git a/src/Umbraco.Core/Models/PropertyTypeCollection.cs b/src/Umbraco.Core/Models/PropertyTypeCollection.cs index e79015d828..6181ee078b 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -23,18 +23,18 @@ namespace Umbraco.Core.Models [IgnoreDataMember] internal Action OnAdd; - internal PropertyTypeCollection(bool isPublishing) + internal PropertyTypeCollection(bool supportsPublishing) { - IsPublishing = isPublishing; + SupportsPublishing = supportsPublishing; } - public PropertyTypeCollection(bool isPublishing, IEnumerable properties) - : this(isPublishing) + public PropertyTypeCollection(bool supportsPublishing, IEnumerable properties) + : this(supportsPublishing) { Reset(properties); } - public bool IsPublishing { get; } + public bool SupportsPublishing { get; } /// /// Resets the collection to only contain the instances referenced in the parameter. @@ -51,7 +51,7 @@ namespace Umbraco.Core.Models protected override void SetItem(int index, PropertyType item) { - item.IsPublishing = IsPublishing; + item.SupportsPublishing = SupportsPublishing; base.SetItem(index, item); OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index)); } @@ -65,7 +65,7 @@ namespace Umbraco.Core.Models protected override void InsertItem(int index, PropertyType item) { - item.IsPublishing = IsPublishing; + item.SupportsPublishing = SupportsPublishing; base.InsertItem(index, item); OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item)); } @@ -79,7 +79,7 @@ namespace Umbraco.Core.Models // TODO: Instead of 'new' this should explicitly implement one of the collection interfaces members internal new void Add(PropertyType item) { - item.IsPublishing = IsPublishing; + item.SupportsPublishing = SupportsPublishing; // TODO: this is not pretty and should be refactored try @@ -155,7 +155,7 @@ namespace Umbraco.Core.Models public object DeepClone() { - var clone = new PropertyTypeCollection(IsPublishing); + var clone = new PropertyTypeCollection(SupportsPublishing); foreach (var propertyType in this) clone.Add((PropertyType) propertyType.DeepClone()); return clone; diff --git a/src/Umbraco.Core/Persistence/Factories/MemberTypeReadOnlyFactory.cs b/src/Umbraco.Core/Persistence/Factories/MemberTypeReadOnlyFactory.cs index 8e3c7db3b0..c7ce98a89c 100644 --- a/src/Umbraco.Core/Persistence/Factories/MemberTypeReadOnlyFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/MemberTypeReadOnlyFactory.cs @@ -78,7 +78,7 @@ namespace Umbraco.Core.Persistence.Factories var propertyGroups = new PropertyGroupCollection(); foreach (var groupDto in dto.PropertyTypeGroups.Where(x => x.Id.HasValue)) { - var group = new PropertyGroup(MemberType.IsPublishingConst); + var group = new PropertyGroup(MemberType.SupportsPublishingConst); // if the group is defined on the current member type, // assign its identifier, else it will be zero @@ -93,7 +93,7 @@ namespace Umbraco.Core.Persistence.Factories group.Key = groupDto.UniqueId; group.Name = groupDto.Text; group.SortOrder = groupDto.SortOrder; - group.PropertyTypes = new PropertyTypeCollection(MemberType.IsPublishingConst); + group.PropertyTypes = new PropertyTypeCollection(MemberType.SupportsPublishingConst); //Because we are likely to have a group with no PropertyTypes we need to ensure that these are excluded var localGroupDto = groupDto; diff --git a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs index d83650d798..f1473b5888 100644 --- a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs @@ -121,7 +121,7 @@ namespace Umbraco.Core.Persistence.Factories foreach (var property in properties) { - if (property.PropertyType.IsPublishing) + if (property.PropertyType.SupportsPublishing) { //create the resulting hashset if it's not created and the entity varies by culture if (entityVariesByCulture && editedCultures == null) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs index a9bb097346..095c77eb42 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs @@ -25,7 +25,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement _templateRepository = templateRepository; } - protected override bool IsPublishing => ContentType.IsPublishingConst; + protected override bool SupportsPublishing => ContentType.SupportsPublishingConst; protected override IRepositoryCachePolicy CreateCachePolicy() { @@ -60,11 +60,11 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (ids.Any()) { //NOTE: This logic should never be executed according to our cache policy - return ContentTypeQueryMapper.GetContentTypes(Database, SqlSyntax, IsPublishing, this, _templateRepository) + return ContentTypeQueryMapper.GetContentTypes(Database, SqlSyntax, SupportsPublishing, this, _templateRepository) .Where(x => ids.Contains(x.Id)); } - return ContentTypeQueryMapper.GetContentTypes(Database, SqlSyntax, IsPublishing, this, _templateRepository); + return ContentTypeQueryMapper.GetContentTypes(Database, SqlSyntax, SupportsPublishing, this, _templateRepository); } protected override IEnumerable PerformGetAll(params Guid[] ids) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index 7f41d2a456..e9b952f11d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -32,7 +32,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - protected abstract bool IsPublishing { get; } + protected abstract bool SupportsPublishing { get; } public IEnumerable> Move(TEntity moving, EntityContainer container) { @@ -1021,7 +1021,7 @@ AND umbracoNode.id <> @id", var dtos = Database .Fetch(sql); - var propertyGroups = PropertyGroupFactory.BuildEntity(dtos, IsPublishing, id, createDate, updateDate,CreatePropertyType); + var propertyGroups = PropertyGroupFactory.BuildEntity(dtos, SupportsPublishing, id, createDate, updateDate,CreatePropertyType); return new PropertyGroupCollection(propertyGroups); } @@ -1057,7 +1057,7 @@ AND umbracoNode.id <> @id", //Reset dirty properties Parallel.ForEach(list, currentFile => currentFile.ResetDirtyProperties(false)); - return new PropertyTypeCollection(IsPublishing, list); + return new PropertyTypeCollection(SupportsPublishing, list); } protected void ValidateAlias(PropertyType pt) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaTypeRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaTypeRepository.cs index 5287af353a..a6f39bdb71 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaTypeRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaTypeRepository.cs @@ -20,7 +20,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - protected override bool IsPublishing => MediaType.IsPublishingConst; + protected override bool SupportsPublishing => MediaType.SupportsPublishingConst; protected override IRepositoryCachePolicy CreateCachePolicy() { @@ -55,11 +55,11 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (ids.Any()) { //NOTE: This logic should never be executed according to our cache policy - return ContentTypeQueryMapper.GetMediaTypes(Database, SqlSyntax, IsPublishing, this) + return ContentTypeQueryMapper.GetMediaTypes(Database, SqlSyntax, SupportsPublishing, this) .Where(x => ids.Contains(x.Id)); } - return ContentTypeQueryMapper.GetMediaTypes(Database, SqlSyntax, IsPublishing, this); + return ContentTypeQueryMapper.GetMediaTypes(Database, SqlSyntax, SupportsPublishing, this); } protected override IEnumerable PerformGetAll(params Guid[] ids) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberTypeRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberTypeRepository.cs index 22a5059be7..afb6ac8b43 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberTypeRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberTypeRepository.cs @@ -21,7 +21,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - protected override bool IsPublishing => MemberType.IsPublishingConst; + protected override bool SupportsPublishing => MemberType.SupportsPublishingConst; protected override IRepositoryCachePolicy CreateCachePolicy() { diff --git a/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs b/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs index 096b80de1f..c4380f032c 100644 --- a/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs +++ b/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs @@ -290,7 +290,7 @@ namespace Umbraco.Core.PropertyEditors /// public IEnumerable ConvertDbToXml(Property property, IDataTypeService dataTypeService, ILocalizationService localizationService, bool published) { - published &= property.PropertyType.IsPublishing; + published &= property.PropertyType.SupportsPublishing; var nodeName = property.PropertyType.Alias.ToSafeAlias(); diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 922eb4e2db..5bdc0959da 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -10,6 +10,7 @@ using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; using Umbraco.Core.Persistence.Repositories; +using Umbraco.Core.PropertyEditors; using Umbraco.Core.Scoping; using Umbraco.Core.Services.Changes; @@ -27,6 +28,8 @@ namespace Umbraco.Core.Services.Implement private readonly IDocumentBlueprintRepository _documentBlueprintRepository; private readonly ILanguageRepository _languageRepository; private IQuery _queryNotTrashed; + //TODO: The non-lazy object should be injected + private readonly Lazy _propertyValidationService = new Lazy(() => new PropertyValidationService()); #region Constructors @@ -193,7 +196,7 @@ namespace Umbraco.Core.Services.Implement var content = new Content(name, parentId, contentType); using (var scope = ScopeProvider.CreateScope()) { - CreateContent(scope, content, parent, userId, false); + CreateContent(scope, content, userId, false); scope.Complete(); } @@ -227,7 +230,7 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content type with that alias.", nameof(contentTypeAlias)); // causes rollback var content = new Content(name, parent, contentType); - CreateContent(scope, content, parent, userId, false); + CreateContent(scope, content, userId, false); scope.Complete(); return content; @@ -261,7 +264,7 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content with that id.", nameof(parentId)); // causes rollback var content = parentId > 0 ? new Content(name, parent, contentType) : new Content(name, parentId, contentType); - CreateContent(scope, content, parent, userId, true); + CreateContent(scope, content, userId, true); scope.Complete(); return content; @@ -293,14 +296,14 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content type with that alias.", nameof(contentTypeAlias)); // causes rollback var content = new Content(name, parent, contentType); - CreateContent(scope, content, parent, userId, true); + CreateContent(scope, content, userId, true); scope.Complete(); return content; } } - private void CreateContent(IScope scope, Content content, IContent parent, int userId, bool withIdentity) + private void CreateContent(IScope scope, IContent content, int userId, bool withIdentity) { content.CreatorId = userId; content.WriterId = userId; @@ -311,12 +314,12 @@ namespace Umbraco.Core.Services.Implement // if saving is cancelled, content remains without an identity var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) return; _documentRepository.Save(content); - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); scope.Events.Dispatch(TreeChanged, this, new TreeChange(content, TreeChangeTypes.RefreshNode).ToEventArgs()); } @@ -758,7 +761,7 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope()) { var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) { scope.Complete(); return OperationResult.Cancel(evtMsgs); @@ -783,7 +786,7 @@ namespace Umbraco.Core.Services.Implement if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } var changeType = TreeChangeTypes.RefreshNode; scope.Events.Dispatch(TreeChanged, this, new TreeChange(content, changeType).ToEventArgs()); @@ -813,7 +816,7 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope()) { var saveEventArgs = new ContentSavingEventArgs(contentsA, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) { scope.Complete(); return OperationResult.Cancel(evtMsgs); @@ -833,7 +836,7 @@ namespace Umbraco.Core.Services.Implement if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } scope.Events.Dispatch(TreeChanged, this, treeChanges.ToEventArgs()); Audit(AuditType.Save, userId == -1 ? 0 : userId, Constants.System.Root, "Saved multiple content"); @@ -866,35 +869,51 @@ namespace Umbraco.Core.Services.Implement throw new NotSupportedException($"Culture \"{culture}\" is not supported by invariant content types."); } - // if culture is specific, first publish the invariant values, then publish the culture itself. - // if culture is '*', then publish them all (including variants) - - Property[] invalidProperties; - - // explicitly SaveAndPublish a specific culture also publishes invariant values - if (!culture.IsNullOrWhiteSpace() && culture != "*") + using (var scope = ScopeProvider.CreateScope()) { - // publish the invariant values - var publishInvariant = content.PublishCulture(out invalidProperties, null); - if (!publishInvariant) + scope.WriteLock(Constants.Locks.ContentTree); + + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + Property[] invalidProperties; + + // if culture is specific, first publish the invariant values, then publish the culture itself. + // if culture is '*', then publish them all (including variants) + + // explicitly SaveAndPublish a specific culture also publishes invariant values + if (!culture.IsNullOrWhiteSpace() && culture != "*") + { + // publish the invariant values + var publishInvariant = content.PublishCulture(null); + if (!publishInvariant) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) + { + InvalidProperties = invalidProperties + }; + } + + // publish the culture(s) + var publishCulture = content.PublishCulture(culture); + if (!publishCulture) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) { - InvalidProperties = invalidProperties ?? Enumerable.Empty() + InvalidProperties = invalidProperties }; + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); + scope.Complete(); + return result; } - - // publish the culture(s) - var publishCulture = content.PublishCulture(out invalidProperties, culture); - if (!publishCulture) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) - { - InvalidProperties = invalidProperties ?? Enumerable.Empty() - }; - - // finally, "save publishing" - // what happens next depends on whether the content can be published or not - return CommitDocumentChanges(content, userId, raiseEvents); } /// @@ -903,23 +922,40 @@ namespace Umbraco.Core.Services.Implement if (content == null) throw new ArgumentNullException(nameof(content)); if (cultures == null) throw new ArgumentNullException(nameof(cultures)); - var evtMsgs = EventMessagesFactory.Get(); - - var varies = content.ContentType.VariesByCulture(); - - if (cultures.Length == 0) + using (var scope = ScopeProvider.CreateScope()) { - //no cultures specified and doesn't vary, so publish it, else nothing to publish - return !varies - ? SaveAndPublish(content, userId: userId, raiseEvents: raiseEvents) - : new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); + scope.WriteLock(Constants.Locks.ContentTree); + + var evtMsgs = EventMessagesFactory.Get(); + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + var varies = content.ContentType.VariesByCulture(); + + if (cultures.Length == 0) + { + //no cultures specified and doesn't vary, so publish it, else nothing to publish + return !varies + ? SaveAndPublish(content, userId: userId, raiseEvents: raiseEvents) + : new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); + } + + + if (cultures.Select(content.PublishCulture).Any(isValid => !isValid)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out var invalidProperties)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) + { + InvalidProperties = invalidProperties + }; + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); + scope.Complete(); + return result; } - - // TODO: currently, no way to know which one failed - if (cultures.Select(content.PublishCulture).Any(isValid => !isValid)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); - - return CommitDocumentChanges(content, userId, raiseEvents); } /// @@ -952,56 +988,87 @@ namespace Umbraco.Core.Services.Implement if (!content.Published) return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); - // all cultures = unpublish whole - if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) + using (var scope = ScopeProvider.CreateScope()) { - content.PublishedState = PublishedState.Unpublishing; - } - else - { - // If the culture we want to unpublish was already unpublished, nothing to do. - // To check for that we need to lookup the persisted content item - var persisted = content.HasIdentity ? GetById(content.Id) : null; + scope.WriteLock(Constants.Locks.ContentTree); - if (persisted != null && !persisted.IsCulturePublished(culture)) - return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); - // unpublish the culture - content.UnpublishCulture(culture); + // all cultures = unpublish whole + if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) + { + content.PublishedState = PublishedState.Unpublishing; + } + else + { + // If the culture we want to unpublish was already unpublished, nothing to do. + // To check for that we need to lookup the persisted content item + var persisted = content.HasIdentity ? GetById(content.Id) : null; + + if (persisted != null && !persisted.IsCulturePublished(culture)) + return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); + + // unpublish the culture + content.UnpublishCulture(culture); + } + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId); + scope.Complete(); + return result; } - // finally, "save publishing" - return CommitDocumentChanges(content, userId); } /// /// Saves a document and publishes/unpublishes any pending publishing changes made to the document. /// /// + /// + /// This MUST NOT be called from within this service, this used to be a public API and must only be used outside of this service. + /// Internally in this service, calls must be made to CommitDocumentChangesInternal + /// + /// /// This is the underlying logic for both publishing and unpublishing any document - /// Pending publishing/unpublishing changes on a document are made with calls to and - /// . + /// Pending publishing/unpublishing changes on a document are made with calls to and + /// . /// When publishing or unpublishing a single culture, or all cultures, use /// and . But if the flexibility to both publish and unpublish in a single operation is required - /// then this method needs to be used in combination with and + /// then this method needs to be used in combination with and /// on the content itself - this prepares the content, but does not commit anything - and then, invoke /// to actually commit the changes to the database. /// The document is *always* saved, even when publishing fails. /// - internal PublishResult CommitDocumentChanges(IContent content, int userId = Constants.Security.SuperUserId, bool raiseEvents = true) + internal PublishResult CommitDocumentChanges(IContent content, + int userId = Constants.Security.SuperUserId, bool raiseEvents = true) { using (var scope = ScopeProvider.CreateScope()) { + var evtMsgs = EventMessagesFactory.Get(); + scope.WriteLock(Constants.Locks.ContentTree); - var result = CommitDocumentChangesInternal(scope, content, userId, raiseEvents); + + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); scope.Complete(); return result; } } - private PublishResult CommitDocumentChangesInternal(IScope scope, IContent content, int userId = Constants.Security.SuperUserId, bool raiseEvents = true, bool branchOne = false, bool branchRoot = false) + private PublishResult CommitDocumentChangesInternal(IScope scope, IContent content, + ContentSavingEventArgs saveEventArgs, + int userId = Constants.Security.SuperUserId, bool raiseEvents = true, bool branchOne = false, bool branchRoot = false) { - var evtMsgs = EventMessagesFactory.Get(); + if (scope == null) throw new ArgumentNullException(nameof(scope)); + if (content == null) throw new ArgumentNullException(nameof(content)); + if (saveEventArgs == null) throw new ArgumentNullException(nameof(saveEventArgs)); + + var evtMsgs = saveEventArgs.Messages; + PublishResult publishResult = null; PublishResult unpublishResult = null; @@ -1027,11 +1094,6 @@ namespace Umbraco.Core.Services.Implement var changeType = isNew ? TreeChangeTypes.RefreshNode : TreeChangeTypes.RefreshBranch; var previouslyPublished = content.HasIdentity && content.Published; - // always save - var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) - return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); - if (publishing) { culturesUnpublishing = content.GetCulturesUnpublishing(); @@ -1067,7 +1129,7 @@ namespace Umbraco.Core.Services.Implement // but: (a) it means we don't reproduce the PublishState logic here and (b) setting the // PublishState to anything other than Publishing or Unpublishing - which is precisely // what we want to do here - throws - content.Published = content.Published; + content.Published = content.Published; } } @@ -1116,7 +1178,7 @@ namespace Umbraco.Core.Services.Implement // raise the Saved event, always if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } if (unpublishing) // we have tried to unpublish - won't happen in a branch @@ -1251,7 +1313,13 @@ namespace Umbraco.Core.Services.Implement .Distinct() .ToList(); - Property[] invalidProperties = null; + if (pendingCultures.Count == 0) + break; //shouldn't happen but no point in continuing if there's nothing there + + var saveEventArgs = new ContentSavingEventArgs(d, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + yield return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, d); + var publishing = true; foreach (var culture in pendingCultures) { @@ -1260,19 +1328,24 @@ namespace Umbraco.Core.Services.Implement if (d.Trashed) continue; // won't publish - publishing &= d.PublishCulture(out invalidProperties, culture); //set the culture to be published + //publish the culture values and validate the property values, if validation fails, log the invalid properties so the develeper has an idea of what has failed + Property[] invalidProperties = null; + var tryPublish = d.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(d, out invalidProperties); + if (invalidProperties != null && invalidProperties.Length > 0) + Logger.Warn("Scheduled publishing will fail for document {DocumentId} and culture {Culture} because of invalid properties {InvalidProperties}", + d.Id, culture, string.Join(",", invalidProperties.Select(x => x.Alias))); + + publishing &= tryPublish; //set the culture to be published if (!publishing) break; // no point continuing } if (d.Trashed) result = new PublishResult(PublishResultType.FailedPublishIsTrashed, evtMsgs, d); else if (!publishing) - result = new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, d) - { - InvalidProperties = invalidProperties ?? Enumerable.Empty() - }; + result = new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, d); else - result = CommitDocumentChanges(d, d.WriterId); + result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId); + if (result.Success == false) Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); @@ -1306,6 +1379,13 @@ namespace Umbraco.Core.Services.Implement .Distinct() .ToList(); + if (pendingCultures.Count == 0) + break; //shouldn't happen but no point in continuing if there's nothing there + + var saveEventArgs = new ContentSavingEventArgs(d, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + yield return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, d); + foreach (var c in pendingCultures) { //Clear this schedule for this culture @@ -1314,13 +1394,11 @@ namespace Umbraco.Core.Services.Implement d.UnpublishCulture(c); } - if (pendingCultures.Count > 0) - { - result = CommitDocumentChanges(d, d.WriterId); - if (result.Success == false) - Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); - yield return result; - } + result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId); + if (result.Success == false) + Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); + yield return result; + } else { @@ -1341,16 +1419,20 @@ namespace Umbraco.Core.Services.Implement } } - private bool SaveAndPublishBranch_PublishCultures(IContent c, HashSet culturesToPublish) + // utility 'PublishCultures' func used by SaveAndPublishBranch + private bool SaveAndPublishBranch_PublishCultures(IContent content, HashSet culturesToPublish) { + //TODO: This does not support being able to return invalid property details to bubble up to the UI + // variant content type - publish specified cultures // invariant content type - publish only the invariant culture - return c.ContentType.VariesByCulture() - ? culturesToPublish.All(c.PublishCulture) - : c.PublishCulture(); + return content.ContentType.VariesByCulture() + ? culturesToPublish.All(culture => content.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(content, out _)) + : content.PublishCulture() && _propertyValidationService.Value.IsPropertyDataValid(content, out _); } - private HashSet SaveAndPublishBranch_ShouldPublish3(ref HashSet cultures, string c, bool published, bool edited, bool isRoot, bool force) + // utility 'ShouldPublish' func used by SaveAndPublishBranch + private HashSet SaveAndPublishBranch_ShouldPublish(ref HashSet cultures, string c, bool published, bool edited, bool isRoot, bool force) { // if published, republish if (published) @@ -1368,7 +1450,6 @@ namespace Umbraco.Core.Services.Implement return cultures; } - /// public IEnumerable SaveAndPublishBranch(IContent content, bool force, string culture = "*", int userId = Constants.Security.SuperUserId) { @@ -1387,10 +1468,10 @@ namespace Umbraco.Core.Services.Implement HashSet culturesToPublish = null; if (!c.ContentType.VariesByCulture()) // invariant content type - return SaveAndPublishBranch_ShouldPublish3(ref culturesToPublish, "*", c.Published, c.Edited, isRoot, force); + return SaveAndPublishBranch_ShouldPublish(ref culturesToPublish, "*", c.Published, c.Edited, isRoot, force); if (culture != "*") // variant content type, specific culture - return SaveAndPublishBranch_ShouldPublish3(ref culturesToPublish, culture, c.IsCulturePublished(culture), c.IsCultureEdited(culture), isRoot, force); + return SaveAndPublishBranch_ShouldPublish(ref culturesToPublish, culture, c.IsCulturePublished(culture), c.IsCultureEdited(culture), isRoot, force); // variant content type, all cultures if (c.Published) @@ -1398,7 +1479,7 @@ namespace Umbraco.Core.Services.Implement // then some (and maybe all) cultures will be 'already published' (unless forcing), // others will have to 'republish this culture' foreach (var x in c.AvailableCultures) - SaveAndPublishBranch_ShouldPublish3(ref culturesToPublish, x, c.IsCulturePublished(x), c.IsCultureEdited(x), isRoot, force); + SaveAndPublishBranch_ShouldPublish(ref culturesToPublish, x, c.IsCulturePublished(x), c.IsCultureEdited(x), isRoot, force); return culturesToPublish; } @@ -1427,7 +1508,7 @@ namespace Umbraco.Core.Services.Implement HashSet culturesToPublish = null; if (!c.ContentType.VariesByCulture()) // invariant content type - return SaveAndPublishBranch_ShouldPublish3(ref culturesToPublish, "*", c.Published, c.Edited, isRoot, force); + return SaveAndPublishBranch_ShouldPublish(ref culturesToPublish, "*", c.Published, c.Edited, isRoot, force); // variant content type, specific cultures if (c.Published) @@ -1435,7 +1516,7 @@ namespace Umbraco.Core.Services.Implement // then some (and maybe all) cultures will be 'already published' (unless forcing), // others will have to 'republish this culture' foreach (var x in cultures) - SaveAndPublishBranch_ShouldPublish3(ref culturesToPublish, x, c.IsCulturePublished(x), c.IsCultureEdited(x), isRoot, force); + SaveAndPublishBranch_ShouldPublish(ref culturesToPublish, x, c.IsCulturePublished(x), c.IsCultureEdited(x), isRoot, force); return culturesToPublish; } @@ -1448,7 +1529,6 @@ namespace Umbraco.Core.Services.Implement return SaveAndPublishBranch(content, force, ShouldPublish, SaveAndPublishBranch_PublishCultures, userId); } - /// internal IEnumerable SaveAndPublishBranch(IContent document, bool force, Func> shouldPublish, Func, bool> publishCultures, @@ -1473,7 +1553,7 @@ namespace Umbraco.Core.Services.Implement throw new InvalidOperationException("Cannot mix PublishCulture and SaveAndPublishBranch."); // deal with the branch root - if it fails, abort - var result = SaveAndPublishBranchOne(scope, document, shouldPublish, publishCultures, true, publishedDocuments, evtMsgs, userId); + var result = SaveAndPublishBranchItem(scope, document, shouldPublish, publishCultures, true, publishedDocuments, evtMsgs, userId); if (result != null) { results.Add(result); @@ -1504,7 +1584,7 @@ namespace Umbraco.Core.Services.Implement } // no need to check path here, parent has to be published here - result = SaveAndPublishBranchOne(scope, d, shouldPublish, publishCultures, false, publishedDocuments, evtMsgs, userId); + result = SaveAndPublishBranchItem(scope, d, shouldPublish, publishCultures, false, publishedDocuments, evtMsgs, userId); if (result != null) { results.Add(result); @@ -1534,7 +1614,7 @@ namespace Umbraco.Core.Services.Implement // shouldPublish: a function determining whether the document has changes that need to be published // note - 'force' is handled by 'editing' // publishValues: a function publishing values (using the appropriate PublishCulture calls) - private PublishResult SaveAndPublishBranchOne(IScope scope, IContent document, + private PublishResult SaveAndPublishBranchItem(IScope scope, IContent document, Func> shouldPublish, Func, bool> publishCultures, bool isRoot, @@ -1547,15 +1627,18 @@ namespace Umbraco.Core.Services.Implement if (culturesToPublish.Count == 0) // empty = already published return new PublishResult(PublishResultType.SuccessPublishAlready, evtMsgs, document); + var saveEventArgs = new ContentSavingEventArgs(document, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, document); + // publish & check if values are valid if (!publishCultures(document, culturesToPublish)) { //TODO: Based on this callback behavior there is no way to know which properties may have been invalid if this failed, see other results of FailedPublishContentInvalid return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, document); } - - var result = CommitDocumentChangesInternal(scope, document, userId, branchOne: true, branchRoot: isRoot); + var result = CommitDocumentChangesInternal(scope, document, saveEventArgs, userId, branchOne: true, branchRoot: isRoot); if (result.Success) publishedDocuments.Add(document); return result; @@ -2530,7 +2613,6 @@ namespace Umbraco.Core.Services.Implement return new PublishResult(PublishResultType.SuccessPublishCulture, evtMsgs, content); } - Logger.Info("Document {ContentName} (id={ContentId}) has been published.", content.Name, content.Id); return new PublishResult(evtMsgs, content); } diff --git a/src/Umbraco.Core/Services/Implement/MemberService.cs b/src/Umbraco.Core/Services/Implement/MemberService.cs index 45a6dc7dc1..8c69664712 100644 --- a/src/Umbraco.Core/Services/Implement/MemberService.cs +++ b/src/Umbraco.Core/Services/Implement/MemberService.cs @@ -1175,7 +1175,7 @@ namespace Umbraco.Core.Services.Implement var identity = int.MaxValue; var memType = new MemberType(-1); - var propGroup = new PropertyGroup(MemberType.IsPublishingConst) + var propGroup = new PropertyGroup(MemberType.SupportsPublishingConst) { Name = "Membership", Id = --identity diff --git a/src/Umbraco.Core/Services/PropertyValidationService.cs b/src/Umbraco.Core/Services/PropertyValidationService.cs new file mode 100644 index 0000000000..146173dd5c --- /dev/null +++ b/src/Umbraco.Core/Services/PropertyValidationService.cs @@ -0,0 +1,135 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Umbraco.Core.Collections; +using Umbraco.Core.Composing; +using Umbraco.Core.Models; +using Umbraco.Core.PropertyEditors; + +namespace Umbraco.Core.Services +{ + //TODO: We should make this an interface and inject it into the ContentService + internal class PropertyValidationService + { + private readonly PropertyEditorCollection _propertyEditors; + private readonly IDataTypeService _dataTypeService; + + public PropertyValidationService(PropertyEditorCollection propertyEditors, IDataTypeService dataTypeService) + { + _propertyEditors = propertyEditors; + _dataTypeService = dataTypeService; + } + + //TODO: Remove this method in favor of the overload specifying all dependencies + public PropertyValidationService() + : this(Current.PropertyEditors, Current.Services.DataTypeService) + { + } + + /// + /// Validates the content item's properties pass validation rules + /// + /// If the content type is variant, then culture can be either '*' or an actual culture, but neither 'null' nor + /// 'empty'. If the content type is invariant, then culture can be either '*' or null or empty. + public bool IsPropertyDataValid(IContentBase content, out Property[] invalidProperties, string culture = "*") + { + // select invalid properties + invalidProperties = content.Properties.Where(x => + { + // if culture is null, we validate invariant properties only + // if culture is '*' we validate both variant and invariant properties, automatically + // if culture is specific eg 'en-US' we both too, but explicitly + + var varies = x.PropertyType.VariesByCulture(); + + if (culture == null) + return !(varies || IsPropertyValid(x, null)); // validate invariant property, invariant culture + + if (culture == "*") + return !IsPropertyValid(x, culture); // validate property, all cultures + + return varies + ? !IsPropertyValid(x, culture) // validate variant property, explicit culture + : !IsPropertyValid(x, null); // validate invariant property, explicit culture + }) + .ToArray(); + + return invalidProperties.Length == 0; + } + + /// + /// Gets a value indicating whether the property has valid values. + /// + public bool IsPropertyValid(Property property, string culture = "*", string segment = "*") + { + //NOTE - the pvalue and vvalues logic in here is borrowed directly from the Property.Values setter so if you are wondering what that's all about, look there. + // The underlying Property._pvalue and Property._vvalues are not exposed but we can re-create these values ourselves which is what it's doing. + + culture = culture.NullOrWhiteSpaceAsNull(); + segment = segment.NullOrWhiteSpaceAsNull(); + + Property.PropertyValue pvalue = null; + + // if validating invariant/neutral, and it is supported, validate + // (including ensuring that the value exists, if mandatory) + if ((culture == null || culture == "*") && (segment == null || segment == "*") && property.PropertyType.SupportsVariation(null, null)) + { + // validate pvalue (which is the invariant value) + pvalue = property.Values.FirstOrDefault(x => x.Culture == null && x.Segment == null); + if (!IsValidPropertyValue(property, pvalue?.EditedValue)) + return false; + } + + // if validating only invariant/neutral, we are good + if (culture == null && segment == null) + return true; + + // if nothing else to validate, we are good + if ((culture == null || culture == "*") && (segment == null || segment == "*") && !property.PropertyType.VariesByCulture()) + return true; + + // for anything else, validate the existing values (including mandatory), + // but we cannot validate mandatory globally (we don't know the possible cultures and segments) + + // validate vvalues (which are the variant values) + + // if we don't have vvalues (property.Values is empty or only contains pvalue), validate null + if (property.Values.Count == (pvalue == null ? 0 : 1)) + return culture == "*" || IsValidPropertyValue(property, null); + + // else validate vvalues (but don't revalidate pvalue) + var pvalues = property.Values.Where(x => + x != pvalue && // don't revalidate pvalue + property.PropertyType.SupportsVariation(x.Culture, x.Segment, true) && // the value variation is ok + (culture == "*" || x.Culture.InvariantEquals(culture)) && // the culture matches + (segment == "*" || x.Segment.InvariantEquals(segment))) // the segment matches + .ToList(); + + return pvalues.Count == 0 || pvalues.All(x => IsValidPropertyValue(property, x.EditedValue)); + } + + /// + /// Boolean indicating whether the passed in value is valid + /// + /// + /// + /// True is property value is valid, otherwise false + private bool IsValidPropertyValue(Property property, object value) + { + return IsPropertyValueValid(property.PropertyType, value); + } + + /// + /// Determines whether a value is valid for this property type. + /// + private bool IsPropertyValueValid(PropertyType propertyType, object value) + { + var editor = _propertyEditors[propertyType.PropertyEditorAlias]; + var configuration = _dataTypeService.GetDataType(propertyType.DataTypeId).Configuration; + var valueEditor = editor.GetValueEditor(configuration); + return !valueEditor.Validate(value, propertyType.Mandatory, propertyType.ValidationRegExp).Any(); + } + } +} diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 65b346b895..89fc80f7c6 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -211,6 +211,7 @@ + diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 36fb399fa7..5569ecfa60 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -135,7 +135,7 @@ namespace Umbraco.Tests.Models Assert.Throws(() => prop.PublishValues()); // change - propertyType.IsPublishing = true; + propertyType.SupportsPublishing = true; // can get value // and now published value is null @@ -364,6 +364,7 @@ namespace Umbraco.Tests.Models [Test] public void ContentPublishValuesWithMixedPropertyTypeVariations() { + var propertyValidationService = new PropertyValidationService(Current.Factory.GetInstance(), Current.Factory.GetInstance().DataTypeService); const string langFr = "fr-FR"; // content type varies by Culture @@ -383,11 +384,15 @@ namespace Umbraco.Tests.Models content.SetCultureName("hello", langFr); - Assert.IsFalse(content.PublishCulture(langFr)); // fails because prop1 is mandatory + Assert.IsTrue(content.PublishCulture(langFr)); // succeeds because names are ok (not validating properties here) + Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// fails because prop1 is mandatory + content.SetValue("prop1", "a", langFr); - Assert.IsFalse(content.PublishCulture(langFr)); // fails because prop2 is mandatory and invariant + Assert.IsTrue(content.PublishCulture(langFr)); // succeeds because names are ok (not validating properties here) + Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// fails because prop2 is mandatory and invariant content.SetValue("prop2", "x"); - Assert.IsTrue(content.PublishCulture(langFr)); // now it's ok + Assert.IsTrue(content.PublishCulture(langFr)); // still ok... + Assert.IsTrue(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// now it's ok Assert.AreEqual("a", content.GetValue("prop1", langFr, published: true)); Assert.AreEqual("x", content.GetValue("prop2", published: true)); @@ -480,20 +485,21 @@ namespace Umbraco.Tests.Models [Test] public void ValidationTests() { - var propertyType = new PropertyType("editor", ValueStorageType.Nvarchar) { Alias = "prop", IsPublishing = true }; + var propertyType = new PropertyType("editor", ValueStorageType.Nvarchar) { Alias = "prop", SupportsPublishing = true }; var prop = new Property(propertyType); prop.SetValue("a"); Assert.AreEqual("a", prop.GetValue()); Assert.IsNull(prop.GetValue(published: true)); + var propertyValidationService = new PropertyValidationService(Current.Factory.GetInstance(), Current.Factory.GetInstance().DataTypeService); - Assert.IsTrue(prop.IsValid()); + Assert.IsTrue(propertyValidationService.IsPropertyValid(prop)); propertyType.Mandatory = true; - Assert.IsTrue(prop.IsValid()); + Assert.IsTrue(propertyValidationService.IsPropertyValid(prop)); prop.SetValue(null); - Assert.IsFalse(prop.IsValid()); + Assert.IsFalse(propertyValidationService.IsPropertyValid(prop)); // can publish, even though invalid prop.PublishValues(); diff --git a/src/Umbraco.Tests/Services/ContentServiceEventTests.cs b/src/Umbraco.Tests/Services/ContentServiceEventTests.cs index 1547ec232a..0cb301fcad 100644 --- a/src/Umbraco.Tests/Services/ContentServiceEventTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceEventTests.cs @@ -1,5 +1,6 @@ using System.Linq; using NUnit.Framework; +using Umbraco.Core; using Umbraco.Core.Events; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Repositories.Implement; @@ -30,7 +31,7 @@ namespace Umbraco.Tests.Services } [Test] - public void SavingTest() + public void Saving_Culture() { var languageService = ServiceContext.LocalizationService; @@ -80,13 +81,68 @@ namespace Umbraco.Tests.Services ContentService.Saving += OnSaving; ContentService.Saved += OnSaved; - contentService.Save(document); - ContentService.Saving -= OnSaving; - ContentService.Saved -= OnSaved; + try + { + contentService.Save(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } } [Test] - public void PublishingTest() + public void Saving_Set_Value() + { + var contentTypeService = ServiceContext.ContentTypeService; + + var contentType = MockedContentTypes.CreateTextPageContentType(); + ServiceContext.FileService.SaveTemplate(contentType.DefaultTemplate); + contentTypeService.Save(contentType); + + var contentService = ServiceContext.ContentService; + + IContent document = new Content("content", -1, contentType); + + void OnSaving(IContentService sender, ContentSavingEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.IsTrue(document.GetValue("title").IsNullOrWhiteSpace()); + + saved.SetValue("title", "title"); + } + + void OnSaved(IContentService sender, ContentSavedEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.AreSame("title", document.GetValue("title")); + + //we're only dealing with invariant here + var propValue = saved.Properties["title"].Values.First(x => x.Culture == null && x.Segment == null); + + Assert.AreEqual("title", propValue.EditedValue); + Assert.IsNull(propValue.PublishedValue); + } + + ContentService.Saving += OnSaving; + ContentService.Saved += OnSaved; + try + { + contentService.Save(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } + + } + + [Test] + public void Publishing_Culture() { var languageService = ServiceContext.LocalizationService; @@ -136,9 +192,15 @@ namespace Umbraco.Tests.Services ContentService.Publishing += OnPublishing; ContentService.Published += OnPublished; - contentService.SaveAndPublish(document, "fr-FR"); - ContentService.Publishing -= OnPublishing; - ContentService.Published -= OnPublished; + try + { + contentService.SaveAndPublish(document, "fr-FR"); + } + finally + { + ContentService.Publishing -= OnPublishing; + ContentService.Published -= OnPublished; + } document = contentService.GetById(document.Id); @@ -148,7 +210,105 @@ namespace Umbraco.Tests.Services } [Test] - public void UnpublishingTest() + public void Publishing_Set_Value() + { + var contentTypeService = ServiceContext.ContentTypeService; + + var contentType = MockedContentTypes.CreateTextPageContentType(); + ServiceContext.FileService.SaveTemplate(contentType.DefaultTemplate); + contentTypeService.Save(contentType); + + var contentService = ServiceContext.ContentService; + + IContent document = new Content("content", -1, contentType); + + void OnSaving(IContentService sender, ContentSavingEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.IsTrue(document.GetValue("title").IsNullOrWhiteSpace()); + + saved.SetValue("title", "title"); + } + + void OnSaved(IContentService sender, ContentSavedEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.AreSame("title", document.GetValue("title")); + + //we're only dealing with invariant here + var propValue = saved.Properties["title"].Values.First(x => x.Culture == null && x.Segment == null); + + Assert.AreEqual("title", propValue.EditedValue); + Assert.AreEqual("title", propValue.PublishedValue); + } + + //We are binding to Saving (not Publishing), because the Publishing event is really just used for cancelling, it should not be + //used for setting values and it won't actually work! This is because the Publishing event is raised AFTER the values on the model + //are published, but Saving is raised BEFORE. + ContentService.Saving += OnSaving; + ContentService.Saved += OnSaved; + try + { + contentService.SaveAndPublish(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } + } + + [Test] + public void Publishing_Set_Mandatory_Value() + { + var contentTypeService = ServiceContext.ContentTypeService; + + var contentType = MockedContentTypes.CreateTextPageContentType(); + var titleProperty = contentType.PropertyTypes.First(x => x.Alias == "title"); + titleProperty.Mandatory = true; // make this required! + ServiceContext.FileService.SaveTemplate(contentType.DefaultTemplate); + contentTypeService.Save(contentType); + + var contentService = ServiceContext.ContentService; + + IContent document = new Content("content", -1, contentType); + + var result = contentService.SaveAndPublish(document); + Assert.IsFalse(result.Success); + Assert.AreEqual("title", result.InvalidProperties.First().Alias); + + // when a service operation fails, the object is dirty and should not be re-used, + // re-create it + document = new Content("content", -1, contentType); + + void OnSaving(IContentService sender, ContentSavingEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.IsTrue(document.GetValue("title").IsNullOrWhiteSpace()); + + saved.SetValue("title", "title"); + } + + //We are binding to Saving (not Publishing), because the Publishing event is really just used for cancelling, it should not be + //used for setting values and it won't actually work! This is because the Publishing event is raised AFTER the values on the model + //are published, but Saving is raised BEFORE. + ContentService.Saving += OnSaving; + try + { + result = contentService.SaveAndPublish(document); + Assert.IsTrue(result.Success); //will succeed now because we were able to specify the required value in the Saving event + } + finally + { + ContentService.Saving -= OnSaving; + } + } + + [Test] + public void Unpublishing_Culture() { var languageService = ServiceContext.LocalizationService; @@ -206,9 +366,15 @@ namespace Umbraco.Tests.Services ContentService.Publishing += OnPublishing; ContentService.Published += OnPublished; - contentService.CommitDocumentChanges(document); - ContentService.Publishing -= OnPublishing; - ContentService.Published -= OnPublished; + try + { + contentService.CommitDocumentChanges(document); + } + finally + { + ContentService.Publishing -= OnPublishing; + ContentService.Published -= OnPublished; + } document = contentService.GetById(document.Id); diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index bcbf01d3f0..04cdc2aab7 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -18,6 +18,7 @@ using Umbraco.Core.Services.Implement; using Umbraco.Tests.Testing; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Cache; +using Umbraco.Core.PropertyEditors; using Umbraco.Tests.LegacyXmlPublishedCache; namespace Umbraco.Tests.Services @@ -986,7 +987,10 @@ namespace Umbraco.Tests.Services Assert.IsTrue(parent.Published); // content cannot publish values because they are invalid - Assert.IsNotEmpty(content.ValidateProperties()); + var propertyValidationService = new PropertyValidationService(Factory.GetInstance(), ServiceContext.DataTypeService); + var isValid = propertyValidationService.IsPropertyDataValid(content, out var invalidProperties); + Assert.IsFalse(isValid); + Assert.IsNotEmpty(invalidProperties); // and therefore cannot be published, // because it did not have a published version at all diff --git a/src/Umbraco.Web/Models/Mapping/ContentTypeMapperProfile.cs b/src/Umbraco.Web/Models/Mapping/ContentTypeMapperProfile.cs index 6653877d24..cc2c7b0cb9 100644 --- a/src/Umbraco.Web/Models/Mapping/ContentTypeMapperProfile.cs +++ b/src/Umbraco.Web/Models/Mapping/ContentTypeMapperProfile.cs @@ -163,7 +163,7 @@ namespace Umbraco.Web.Models.Mapping .IgnoreEntityCommonProperties() - .ForMember(dest => dest.IsPublishing, opt => opt.Ignore()) + .ForMember(dest => dest.SupportsPublishing, opt => opt.Ignore()) // see note above - have to do this here? .ForMember(dest => dest.PropertyEditorAlias, opt => opt.Ignore())