diff --git a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs
index 01812f8469..8f57842f34 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,19 @@ namespace Umbraco.Core.Models
content.CultureInfos.AddOrUpdate(culture, name, date);
}
- public static bool PublishCulture(this IContent content, string culture = "*")
+ ///
+ /// This will set the publishing values for names and properties for the content/culture
+ ///
+ ///
+ ///
+ ///
+ /// A boolean if it's possible to publish the values for the provided culture. This may fail required names are not set.
+ ///
+ ///
+ /// This does not validation 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 +187,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/Property.cs b/src/Umbraco.Core/Models/Property.cs
index 726ddc1cf5..803b8eb5a5 100644
--- a/src/Umbraco.Core/Models/Property.cs
+++ b/src/Umbraco.Core/Models/Property.cs
@@ -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 d3a47c281c..e1da631868 100644
--- a/src/Umbraco.Core/Models/PropertyType.cs
+++ b/src/Umbraco.Core/Models/PropertyType.cs
@@ -363,18 +363,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/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs
index 922eb4e2db..412f2a1160 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,10 +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)
{
@@ -1116,7 +1179,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 +1314,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 +1329,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 +1380,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 +1395,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,13 +1420,15 @@ namespace Umbraco.Core.Services.Implement
}
}
- private bool SaveAndPublishBranch_PublishCultures(IContent c, HashSet culturesToPublish)
+ 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)
@@ -1547,15 +1628,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;
diff --git a/src/Umbraco.Core/Services/PropertyValidationService.cs b/src/Umbraco.Core/Services/PropertyValidationService.cs
new file mode 100644
index 0000000000..538ec323e9
--- /dev/null
+++ b/src/Umbraco.Core/Services/PropertyValidationService.cs
@@ -0,0 +1,133 @@
+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))
+ {
+ 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)
+
+ var vvalues = property.Values.Count > (pvalue == null ? 0 : 1)
+ ? property.Values.Where(x => x != pvalue).ToDictionary(x => new CompositeNStringNStringKey(x.Culture, x.Segment), x => x)
+ : null;
+
+ if (vvalues == null) return culture == "*" || IsValidPropertyValue(property,null);
+
+ var pvalues = vvalues.Where(x =>
+ property.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 => 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 c7e7db18a7..7ed4c398dc 100755
--- a/src/Umbraco.Core/Umbraco.Core.csproj
+++ b/src/Umbraco.Core/Umbraco.Core.csproj
@@ -210,6 +210,7 @@
+
diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs
index 36fb399fa7..66173fc5ba 100644
--- a/src/Umbraco.Tests/Models/VariationTests.cs
+++ b/src/Umbraco.Tests/Models/VariationTests.cs
@@ -486,14 +486,15 @@ namespace Umbraco.Tests.Models
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 2c0cde0a94..7342665379 100644
--- a/src/Umbraco.Tests/Services/ContentServiceEventTests.cs
+++ b/src/Umbraco.Tests/Services/ContentServiceEventTests.cs
@@ -81,9 +81,15 @@ 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]
@@ -123,9 +129,16 @@ 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]
@@ -179,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);
@@ -225,13 +244,69 @@ namespace Umbraco.Tests.Services
Assert.AreEqual("title", propValue.PublishedValue);
}
- //We are binding to Saving (not Publishing), because it shouldn't make a difference, when setting a property value
- //during Saving, this should become published during a SaveAndPublish operation
+ //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;
- contentService.SaveAndPublish(document);
- 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);
+
+ //TODO: The ContentService doesn't reset the document's PublishedState so since the above fails, if we then try to do
+ // a SaveAndPublish again, we will get an exception: "Cannot save-and-publish (un)publishing content, use the dedicated CommitDocumentChanges method."
+ // but this exception is misleading and is caused because the document's PublishedState wasn't reset.
+ // So instead, we'll just 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]
@@ -293,9 +368,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