Merge pull request #5992 from umbraco/v8/bugfix/5554-unpublish-state

V8/bugfix/5554 unpublish state

src/Umbraco.Core/Models/ContentRepositoryExtensions.cs

@@ -222,7 +222,13 @@ namespace Umbraco.Core.Models
             return true;
         }
 
-        public static void UnpublishCulture(this IContent content, string culture = "*")
+        /// <summary>
+        /// Returns false if the culture is already unpublished
+        /// </summary>
+        /// <param name="content"></param>
+        /// <param name="culture"></param>
+        /// <returns></returns>
+        public static bool UnpublishCulture(this IContent content, string culture = "*")
         {
             culture = culture.NullOrWhiteSpaceAsNull();
 
@@ -230,16 +236,31 @@ 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}\".");
 
-            if (culture == "*") // all cultures
+
+            var keepProcessing = true;
+
+            if (culture == "*")
+            {
+                // all cultures
                 content.ClearPublishInfos();
-            else // one single culture
-                content.ClearPublishInfo(culture);
+            }
+            else
+            {
+                // one single culture
+                keepProcessing = content.ClearPublishInfo(culture);
+            }
+            
 
-            // property.PublishValues only publishes what is valid, variation-wise
-            foreach (var property in content.Properties)
-                property.UnpublishValues(culture);
+            if (keepProcessing)
+            {
+                // property.PublishValues only publishes what is valid, variation-wise
+                foreach (var property in content.Properties)
+                    property.UnpublishValues(culture);
 
-            content.PublishedState = PublishedState.Publishing;
+                content.PublishedState = PublishedState.Publishing;
+            }
+
+            return keepProcessing;
         }
 
         public static void ClearPublishInfos(this IContent content)
@@ -247,15 +268,24 @@ namespace Umbraco.Core.Models
             content.PublishCultureInfos = null;
         }
 
-        public static void ClearPublishInfo(this IContent content, string culture)
+        /// <summary>
+        /// Returns false if the culture is already unpublished
+        /// </summary>
+        /// <param name="content"></param>
+        /// <param name="culture"></param>
+        /// <returns></returns>
+        public static bool ClearPublishInfo(this IContent content, string culture)
         {
             if (culture.IsNullOrWhiteSpace())
                 throw new ArgumentNullOrEmptyException(nameof(culture));
 
-            content.PublishCultureInfos.Remove(culture);
-
-            // set the culture to be dirty - it's been modified
-            content.TouchCulture(culture);
+            var removed = content.PublishCultureInfos.Remove(culture);
+            if (removed)
+            {
+                // set the culture to be dirty - it's been modified
+                content.TouchCulture(culture);
+            }
+            return removed;
         }
 
         /// <summary>

src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs

@@ -512,31 +512,16 @@ namespace Umbraco.Core.Persistence.Repositories.Implement
             foreach (var a in allPropertyDataDtos)
                 a.PropertyTypeDto = indexedPropertyTypeDtos[a.PropertyTypeId];
 
-            // prefetch configuration for tag properties
-            var tagEditors = new Dictionary<string, TagConfiguration>();
-            foreach (var propertyTypeDto in indexedPropertyTypeDtos.Values)
-            {
-                var editorAlias = propertyTypeDto.DataTypeDto.EditorAlias;
-                var editorAttribute = PropertyEditors[editorAlias].GetTagAttribute();
-                if (editorAttribute == null) continue;
-                var tagConfigurationSource = propertyTypeDto.DataTypeDto.Configuration;
-                var tagConfiguration = string.IsNullOrWhiteSpace(tagConfigurationSource)
-                    ? new TagConfiguration()
-                    : JsonConvert.DeserializeObject<TagConfiguration>(tagConfigurationSource);
-                if (tagConfiguration.Delimiter == default) tagConfiguration.Delimiter = editorAttribute.Delimiter;
-                tagEditors[editorAlias] = tagConfiguration;
-            }
-
             // now we have
             // - the definitions
             // - all property data dtos
-            // - tag editors
+            // - tag editors (Actually ... no we don't since i removed that code, but we don't need them anyways it seems)
             // and we need to build the proper property collections
 
-            return GetPropertyCollections(temps, allPropertyDataDtos, tagEditors);
+            return GetPropertyCollections(temps, allPropertyDataDtos);
         }
 
-        private IDictionary<int, PropertyCollection> GetPropertyCollections<T>(List<TempContent<T>> temps, IEnumerable<PropertyDataDto> allPropertyDataDtos, Dictionary<string, TagConfiguration> tagConfigurations)
+        private IDictionary<int, PropertyCollection> GetPropertyCollections<T>(List<TempContent<T>> temps, IEnumerable<PropertyDataDto> allPropertyDataDtos)
             where T : class, IContentBase
         {
             var result = new Dictionary<int, PropertyCollection>();

src/Umbraco.Core/PropertyEditors/TagsPropertyEditorAttribute.cs

@@ -55,6 +55,7 @@ namespace Umbraco.Core.PropertyEditors
         /// <summary>
         /// Gets the type of the dynamic configuration provider.
         /// </summary>
+        //TODO: This is not used and should be implemented in a nicer way, see https://github.com/umbraco/Umbraco-CMS/issues/6017#issuecomment-516253562
         public Type TagsConfigurationProviderType { get; }
     }
 }

src/Umbraco.Core/Services/Implement/ContentService.cs

@@ -886,6 +886,8 @@ namespace Umbraco.Core.Services.Implement
             {
                 scope.WriteLock(Constants.Locks.ContentTree);
 
+                var allLangs = _languageRepository.GetMany().ToList();
+
                 var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs);
                 if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving)))
                     return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content);
@@ -894,13 +896,13 @@ namespace Umbraco.Core.Services.Implement
                 // if culture is '*', then publish them all (including variants)
 
                 //this will create the correct culture impact even if culture is * or null
-                var impact = CultureImpact.Create(culture, _languageRepository.IsDefault(culture), content);
+                var impact = CultureImpact.Create(culture, IsDefaultCulture(allLangs, culture), content);
 
                 // publish the culture(s)
                 // we don't care about the response here, this response will be rechecked below but we need to set the culture info values now.
                 content.PublishCulture(impact);
 
-                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents);
+                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, allLangs, userId, raiseEvents);
                 scope.Complete();
                 return result;
             }
@@ -921,6 +923,8 @@ namespace Umbraco.Core.Services.Implement
             {
                 scope.WriteLock(Constants.Locks.ContentTree);
 
+                var allLangs = _languageRepository.GetMany().ToList();
+
                 var evtMsgs = EventMessagesFactory.Get();
                 var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs);
                 if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving)))
@@ -928,25 +932,23 @@ namespace Umbraco.Core.Services.Implement
 
                 var varies = content.ContentType.VariesByCulture();
 
-                if (cultures.Length == 0)
+                if (cultures.Length == 0 && !varies)
                 {
                     //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);
+                    return SaveAndPublish(content, userId: userId, raiseEvents: raiseEvents);
                 }
 
                 if (cultures.Any(x => x == null || x == "*"))
                     throw new InvalidOperationException("Only valid cultures are allowed to be used in this method, wildcards or nulls are not allowed");
 
-                var impacts = cultures.Select(x => CultureImpact.Explicit(x, _languageRepository.IsDefault(x)));
+                var impacts = cultures.Select(x => CultureImpact.Explicit(x, IsDefaultCulture(allLangs, x)));
 
                 // publish the culture(s)
                 // we don't care about the response here, this response will be rechecked below but we need to set the culture info values now.
                 foreach (var impact in impacts)
                     content.PublishCulture(impact);
 
-                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents);
+                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, allLangs, userId, raiseEvents);
                 scope.Complete();
                 return result;
             }
@@ -986,6 +988,8 @@ namespace Umbraco.Core.Services.Implement
             {
                 scope.WriteLock(Constants.Locks.ContentTree);
 
+                var allLangs = _languageRepository.GetMany().ToList();
+
                 var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs);
                 if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving)))
                     return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content);
@@ -993,26 +997,39 @@ namespace Umbraco.Core.Services.Implement
                 // all cultures = unpublish whole
                 if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null))
                 {
+                    // It's important to understand that when the document varies by culture but the "*" is used,
+                    // we are just unpublishing the whole document but leaving all of the culture's as-is. This is expected
+                    // because we don't want to actually unpublish every culture and then the document, we just want everything
+                    // to be non-routable so that when it's re-published all variants were as they were.
+
                     content.PublishedState = PublishedState.Unpublishing;
+                    var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, allLangs, userId);
+                    scope.Complete();
+                    return result;
                 }
                 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;
+                    // Unpublish the culture, this will change the document state to Publishing! ... which is expected because this will
+                    // essentially be re-publishing the document with the requested culture removed.
+                    // The call to CommitDocumentChangesInternal will perform all the checks like if this is a mandatory culture or the last culture being unpublished
+                    // and will then unpublish the document accordingly.
+                    // If the result of this is false it means there was no culture to unpublish (i.e. it was already unpublished or it did not exist)
+                    var removed = content.UnpublishCulture(culture);
 
-                    if (persisted != null && !persisted.IsCulturePublished(culture))
+                    //save and publish any changes
+                    var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, allLangs, userId);
+
+                    scope.Complete();
+
+                    // In one case the result will be PublishStatusType.FailedPublishNothingToPublish which means that no cultures
+                    // were specified to be published which will be the case when removed is false. In that case
+                    // we want to swap the result type to PublishResultType.SuccessUnpublishAlready (that was the expectation before).
+                    if (result.Result == PublishResultType.FailedPublishNothingToPublish && !removed)
                         return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content);
 
-                    // unpublish the culture
-                    content.UnpublishCulture(culture);
+                    return result;
                 }
-
-                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId);
-                scope.Complete();
-                return result;
             }
-
         }
 
         /// <summary>
@@ -1047,15 +1064,35 @@ namespace Umbraco.Core.Services.Implement
                 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);
+                var allLangs = _languageRepository.GetMany().ToList();
+
+                var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, allLangs, userId, raiseEvents);
                 scope.Complete();
                 return result;
             }
         }
 
+        /// <summary>
+        /// Handles a lot of business logic cases for how the document should be persisted
+        /// </summary>
+        /// <param name="scope"></param>
+        /// <param name="content"></param>
+        /// <param name="saveEventArgs"></param>
+        /// <param name="userId"></param>
+        /// <param name="raiseEvents"></param>
+        /// <param name="branchOne"></param>
+        /// <param name="branchRoot"></param>
+        /// <returns></returns>
+        /// <remarks>
+        /// <para>
+        /// Business logic cases such: as unpublishing a mandatory culture, or unpublishing the last culture, checking for pending scheduled publishing, etc... is dealt with in this method.
+        /// There is quite a lot of cases to take into account along with logic that needs to deal with scheduled saving/publishing, branch saving/publishing, etc...       
+        /// </para>
+        /// </remarks>
         private PublishResult CommitDocumentChangesInternal(IScope scope, IContent content,
-            ContentSavingEventArgs saveEventArgs,
-            int userId = Constants.Security.SuperUserId, bool raiseEvents = true, bool branchOne = false, bool branchRoot = false)
+            ContentSavingEventArgs saveEventArgs, IReadOnlyCollection<ILanguage> allLangs,
+            int userId = Constants.Security.SuperUserId,
+            bool raiseEvents = true, bool branchOne = false, bool branchRoot = false)
         {
             if (scope == null) throw new ArgumentNullException(nameof(scope));
             if (content == null) throw new ArgumentNullException(nameof(content));
@@ -1070,8 +1107,8 @@ namespace Umbraco.Core.Services.Implement
             if (content.PublishedState != PublishedState.Publishing && content.PublishedState != PublishedState.Unpublishing)
                 content.PublishedState = PublishedState.Publishing;
 
-            // state here is either Publishing or Unpublishing
-            // (even though, Publishing to unpublish a culture may end up unpublishing everything)
+            // State here is either Publishing or Unpublishing
+            // Publishing to unpublish a culture may end up unpublishing everything so these flags can be flipped later
             var publishing = content.PublishedState == PublishedState.Publishing;
             var unpublishing = content.PublishedState == PublishedState.Unpublishing;
 
@@ -1088,6 +1125,18 @@ namespace Umbraco.Core.Services.Implement
             var changeType = isNew ? TreeChangeTypes.RefreshNode : TreeChangeTypes.RefreshBranch;
             var previouslyPublished = content.HasIdentity && content.Published;
 
+            //inline method to persist the document with the documentRepository since this logic could be called a couple times below
+            void SaveDocument(IContent c)
+            {
+                // save, always
+                if (c.HasIdentity == false)
+                    c.CreatorId = userId;
+                c.WriterId = userId;
+
+                // saving does NOT change the published version, unless PublishedState is Publishing or Unpublishing
+                _documentRepository.Save(c);
+            }
+
             if (publishing)
             {
                 //determine cultures publishing/unpublishing which will be based on previous calls to content.PublishCulture and ClearPublishInfo
@@ -1097,11 +1146,25 @@ namespace Umbraco.Core.Services.Implement
                         : null;
 
                 // ensure that the document can be published, and publish handling events, business rules, etc
-                publishResult = StrategyCanPublish(scope, content, /*checkPath:*/ (!branchOne || branchRoot), culturesPublishing, culturesUnpublishing, evtMsgs, saveEventArgs);
+                publishResult = StrategyCanPublish(scope, content, /*checkPath:*/ (!branchOne || branchRoot), culturesPublishing, culturesUnpublishing, evtMsgs, saveEventArgs, allLangs);
                 if (publishResult.Success)
                 {
                     // note: StrategyPublish flips the PublishedState to Publishing!
                     publishResult = StrategyPublish(content, culturesPublishing, culturesUnpublishing, evtMsgs);
+
+                    //check if a culture has been unpublished and if there are no cultures left, and then unpublish document as a whole
+                    if (publishResult.Result == PublishResultType.SuccessUnpublishCulture && content.PublishCultureInfos.Count == 0)
+                    {
+                        // This is a special case! We are unpublishing the last culture and to persist that we need to re-publish without any cultures
+                        // so the state needs to remain Publishing to do that. However, we then also need to unpublish the document and to do that
+                        // the state needs to be Unpublishing and it cannot be both. This state is used within the documentRepository to know how to
+                        // persist certain things. So before proceeding below, we need to save the Publishing state to publish no cultures, then we can
+                        // mark the document for Unpublishing.
+                        SaveDocument(content);
+
+                        //set the flag to unpublish and continue
+                        unpublishing = content.Published; // if not published yet, nothing to do
+                    }
                 }
                 else
                 {
@@ -1162,13 +1225,8 @@ namespace Umbraco.Core.Services.Implement
                 }
             }
 
-            // save, always
-            if (content.HasIdentity == false)
-                content.CreatorId = userId;
-            content.WriterId = userId;
-
-            // saving does NOT change the published version, unless PublishedState is Publishing or Unpublishing
-            _documentRepository.Save(content);
+            //Persist the document
+            SaveDocument(content);
 
             // raise the Saved event, always
             if (raiseEvents)
@@ -1186,17 +1244,34 @@ namespace Umbraco.Core.Services.Implement
 
                     if (culturesUnpublishing != null)
                     {
-                        //If we are here, it means we tried unpublishing a culture but it was mandatory so now everything is unpublished
-                        var langs = string.Join(", ", _languageRepository.GetMany()
+                        // This will mean that that we unpublished a mandatory culture or we unpublished the last culture.
+                        
+                        var langs = string.Join(", ", allLangs
                                     .Where(x => culturesUnpublishing.InvariantContains(x.IsoCode))
                                     .Select(x => x.CultureName));
                         Audit(AuditType.UnpublishVariant, userId, content.Id, $"Unpublished languages: {langs}", langs);
-                        //log that the whole content item has been unpublished due to mandatory culture unpublished
-                        Audit(AuditType.Unpublish, userId, content.Id, "Unpublished (mandatory language unpublished)");
-                    }
-                    else
-                        Audit(AuditType.Unpublish, userId, content.Id);
 
+                        if (publishResult == null)
+                            throw new PanicException("publishResult == null - should not happen");
+
+                        switch(publishResult.Result)
+                        {
+                            case PublishResultType.FailedPublishMandatoryCultureMissing:
+                                //occurs when a mandatory culture was unpublished (which means we tried publishing the document without a mandatory culture)
+
+                                //log that the whole content item has been unpublished due to mandatory culture unpublished
+                                Audit(AuditType.Unpublish, userId, content.Id, "Unpublished (mandatory language unpublished)");
+                                return new PublishResult(PublishResultType.SuccessUnpublishMandatoryCulture, evtMsgs, content);
+                            case PublishResultType.SuccessUnpublishCulture:
+                                //occurs when the last culture is unpublished
+
+                                Audit(AuditType.Unpublish, userId, content.Id, "Unpublished (last language unpublished)");
+                                return new PublishResult(PublishResultType.SuccessUnpublishLastCulture, evtMsgs, content);
+                        }
+                        
+                    }
+
+                    Audit(AuditType.Unpublish, userId, content.Id);
                     return new PublishResult(PublishResultType.SuccessUnpublish, evtMsgs, content);
                 }
 
@@ -1236,7 +1311,7 @@ namespace Umbraco.Core.Services.Implement
                         case PublishResultType.SuccessPublishCulture:
                             if (culturesPublishing != null)
                             {
-                                var langs = string.Join(", ", _languageRepository.GetMany()
+                                var langs = string.Join(", ", allLangs
                                     .Where(x => culturesPublishing.InvariantContains(x.IsoCode))
                                     .Select(x => x.CultureName));
                                 Audit(AuditType.PublishVariant, userId, content.Id, $"Published languages: {langs}", langs);
@@ -1245,7 +1320,7 @@ namespace Umbraco.Core.Services.Implement
                         case PublishResultType.SuccessUnpublishCulture:
                             if (culturesUnpublishing != null)
                             {
-                                var langs = string.Join(", ", _languageRepository.GetMany()
+                                var langs = string.Join(", ", allLangs
                                    .Where(x => culturesUnpublishing.InvariantContains(x.IsoCode))
                                    .Select(x => x.CultureName));
                                 Audit(AuditType.UnpublishVariant, userId, content.Id, $"Unpublished languages: {langs}", langs);
@@ -1259,14 +1334,14 @@ namespace Umbraco.Core.Services.Implement
 
             // should not happen
             if (branchOne && !branchRoot)
-                throw new Exception("panic");
+                throw new PanicException("branchOne && !branchRoot - should not happen");
 
             //if publishing didn't happen or if it has failed, we still need to log which cultures were saved
             if (!branchOne && (publishResult == null || !publishResult.Success))
             {
                 if (culturesChanging != null)
                 {
-                    var langs = string.Join(", ", _languageRepository.GetMany()
+                    var langs = string.Join(", ", allLangs
                         .Where(x => culturesChanging.InvariantContains(x.IsoCode))
                         .Select(x => x.CultureName));
                     Audit(AuditType.SaveVariant, userId, content.Id, $"Saved languages: {langs}", langs);
@@ -1297,6 +1372,8 @@ namespace Umbraco.Core.Services.Implement
             {
                 scope.WriteLock(Constants.Locks.ContentTree);
 
+                var allLangs = _languageRepository.GetMany().ToList();
+
                 foreach (var d in _documentRepository.GetContentForRelease(date))
                 {
                     PublishResult result;
@@ -1325,7 +1402,7 @@ namespace Umbraco.Core.Services.Implement
 
                             //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 impact = CultureImpact.Explicit(culture, _languageRepository.IsDefault(culture));
+                            var impact = CultureImpact.Explicit(culture, IsDefaultCulture(allLangs, culture));
                             var tryPublish = d.PublishCulture(impact) && _propertyValidationService.Value.IsPropertyDataValid(d, out invalidProperties, impact);
                             if (invalidProperties != null && invalidProperties.Length > 0)
                                 Logger.Warn<ContentService>("Scheduled publishing will fail for document {DocumentId} and culture {Culture} because of invalid properties {InvalidProperties}",
@@ -1340,7 +1417,7 @@ namespace Umbraco.Core.Services.Implement
                         else if (!publishing)
                             result = new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, d);
                         else
-                            result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId);
+                            result = CommitDocumentChangesInternal(scope, d, saveEventArgs, allLangs, d.WriterId);
 
 
                         if (result.Success == false)
@@ -1390,7 +1467,7 @@ namespace Umbraco.Core.Services.Implement
                             d.UnpublishCulture(c);
                         }
 
-                        result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId);
+                        result = CommitDocumentChangesInternal(scope, d, saveEventArgs, allLangs, d.WriterId);
                         if (result.Success == false)
                             Logger.Error<ContentService>(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result);
                         yield return result;
@@ -1416,7 +1493,7 @@ namespace Umbraco.Core.Services.Implement
         }
 
         // utility 'PublishCultures' func used by SaveAndPublishBranch
-        private bool SaveAndPublishBranch_PublishCultures(IContent content, HashSet<string> culturesToPublish)
+        private bool SaveAndPublishBranch_PublishCultures(IContent content, HashSet<string> culturesToPublish, IReadOnlyCollection<ILanguage> allLangs)
         {
             //TODO: This does not support being able to return invalid property details to bubble up to the UI
 
@@ -1426,7 +1503,7 @@ namespace Umbraco.Core.Services.Implement
             {
                 return culturesToPublish.All(culture =>
                 {
-                    var impact = CultureImpact.Create(culture, _languageRepository.IsDefault(culture), content);
+                    var impact = CultureImpact.Create(culture, IsDefaultCulture(allLangs, culture), content);
                     return content.PublishCulture(impact) && _propertyValidationService.Value.IsPropertyDataValid(content, out _, impact);
                 });
             }
@@ -1535,7 +1612,7 @@ namespace Umbraco.Core.Services.Implement
 
         internal IEnumerable<PublishResult> SaveAndPublishBranch(IContent document, bool force,
             Func<IContent, HashSet<string>> shouldPublish,
-            Func<IContent, HashSet<string>, bool> publishCultures,
+            Func<IContent, HashSet<string>, IReadOnlyCollection<ILanguage>, bool> publishCultures,
             int userId = Constants.Security.SuperUserId)
         {
             if (shouldPublish == null) throw new ArgumentNullException(nameof(shouldPublish));
@@ -1549,6 +1626,8 @@ namespace Umbraco.Core.Services.Implement
             {
                 scope.WriteLock(Constants.Locks.ContentTree);
 
+                var allLangs = _languageRepository.GetMany().ToList();
+
                 if (!document.HasIdentity)
                     throw new InvalidOperationException("Cannot not branch-publish a new document.");
 
@@ -1557,7 +1636,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 = SaveAndPublishBranchItem(scope, document, shouldPublish, publishCultures, true, publishedDocuments, evtMsgs, userId);
+                var result = SaveAndPublishBranchItem(scope, document, shouldPublish, publishCultures, true, publishedDocuments, evtMsgs, userId, allLangs);
                 if (result != null)
                 {
                     results.Add(result);
@@ -1588,7 +1667,7 @@ namespace Umbraco.Core.Services.Implement
                         }
 
                         // no need to check path here, parent has to be published here
-                        result = SaveAndPublishBranchItem(scope, d, shouldPublish, publishCultures, false, publishedDocuments, evtMsgs, userId);
+                        result = SaveAndPublishBranchItem(scope, d, shouldPublish, publishCultures, false, publishedDocuments, evtMsgs, userId, allLangs);
                         if (result != null)
                         {
                             results.Add(result);
@@ -1620,10 +1699,10 @@ namespace Umbraco.Core.Services.Implement
         // publishValues: a function publishing values (using the appropriate PublishCulture calls)
         private PublishResult SaveAndPublishBranchItem(IScope scope, IContent document,
             Func<IContent, HashSet<string>> shouldPublish,
-            Func<IContent, HashSet<string>, bool> publishCultures,
+            Func<IContent, HashSet<string>, IReadOnlyCollection<ILanguage>, bool> publishCultures,
             bool isRoot,
             ICollection<IContent> publishedDocuments,
-            EventMessages evtMsgs, int userId)
+            EventMessages evtMsgs, int userId, IReadOnlyCollection<ILanguage> allLangs)
         {
             var culturesToPublish = shouldPublish(document);
             if (culturesToPublish == null) // null = do not include
@@ -1636,13 +1715,13 @@ namespace Umbraco.Core.Services.Implement
                 return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, document);
 
             // publish & check if values are valid
-            if (!publishCultures(document, culturesToPublish))
+            if (!publishCultures(document, culturesToPublish, allLangs))
             {
                 //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, saveEventArgs, userId, branchOne: true, branchRoot: isRoot);
+            var result = CommitDocumentChangesInternal(scope, document, saveEventArgs, allLangs, userId, branchOne: true, branchRoot: isRoot);
             if (result.Success)
                 publishedDocuments.Add(document);
             return result;
@@ -2343,6 +2422,9 @@ namespace Umbraco.Core.Services.Implement
             _auditRepository.Save(new AuditItem(objectId, type, userId, ObjectTypes.GetName(UmbracoObjectTypes.Document), message, parameters));
         }
 
+        private bool IsDefaultCulture(IReadOnlyCollection<ILanguage> langs, string culture) => langs.Any(x => x.IsDefault && x.IsoCode.InvariantEquals(culture));
+        private bool IsMandatoryCulture(IReadOnlyCollection<ILanguage> langs, string culture) => langs.Any(x => x.IsMandatory && x.IsoCode.InvariantEquals(culture));
+
         #endregion
 
         #region Event Handlers
@@ -2497,7 +2579,9 @@ namespace Umbraco.Core.Services.Implement
         /// <param name="culturesPublishing"></param>
         /// <param name="savingEventArgs"></param>
         /// <returns></returns>
-        private PublishResult StrategyCanPublish(IScope scope, IContent content, bool checkPath, IReadOnlyList<string> culturesPublishing, IReadOnlyCollection<string> culturesUnpublishing, EventMessages evtMsgs, ContentSavingEventArgs savingEventArgs)
+        private PublishResult StrategyCanPublish(IScope scope, IContent content, bool checkPath, IReadOnlyList<string> culturesPublishing,
+            IReadOnlyCollection<string> culturesUnpublishing, EventMessages evtMsgs, ContentSavingEventArgs savingEventArgs,
+            IReadOnlyCollection<ILanguage> allLangs)
         {
             // raise Publishing event
             if (scope.Events.DispatchCancelable(Publishing, this, savingEventArgs.ToContentPublishingEventArgs()))
@@ -2510,7 +2594,7 @@ namespace Umbraco.Core.Services.Implement
 
             var impactsToPublish = culturesPublishing == null
                 ? new[] {CultureImpact.Invariant} //if it's null it's invariant
-                : culturesPublishing.Select(x => CultureImpact.Explicit(x, _languageRepository.IsDefault(x))).ToArray();
+                : culturesPublishing.Select(x => CultureImpact.Explicit(x, allLangs.Any(lang => lang.IsoCode.InvariantEquals(x) && lang.IsMandatory))).ToArray();
 
             // publish the culture(s)
             if (!impactsToPublish.All(content.PublishCulture))
@@ -2531,11 +2615,17 @@ namespace Umbraco.Core.Services.Implement
                 if (culturesPublishing == null)
                     throw new InvalidOperationException("Internal error, variesByCulture but culturesPublishing is null.");
 
-                if (content.Published && culturesPublishing.Count == 0 && culturesUnpublishing.Count == 0) // no published cultures = cannot be published
+                if (content.Published && culturesPublishing.Count == 0 && culturesUnpublishing.Count == 0)
+                {
+                    // no published cultures = cannot be published
+                    // This will occur if for example, a culture that is already unpublished is sent to be unpublished again, or vice versa, in that case
+                    // there will be nothing to publish/unpublish.
                     return new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content);
+                }
+                
 
                 // missing mandatory culture = cannot be published
-                var mandatoryCultures = _languageRepository.GetMany().Where(x => x.IsMandatory).Select(x => x.IsoCode);
+                var mandatoryCultures = allLangs.Where(x => x.IsMandatory).Select(x => x.IsoCode);
                 var mandatoryMissing = mandatoryCultures.Any(x => !content.PublishedCultures.Contains(x, StringComparer.OrdinalIgnoreCase));
                 if (mandatoryMissing)
                     return new PublishResult(PublishResultType.FailedPublishMandatoryCultureMissing, evtMsgs, content);
@@ -2676,6 +2766,7 @@ namespace Umbraco.Core.Services.Implement
         {
             var attempt = new PublishResult(PublishResultType.SuccessUnpublish, evtMsgs, content);
 
+            //TODO: What is this check?? we just created this attempt and of course it is Success?!
             if (attempt.Success == false)
                 return attempt;
 

src/Umbraco.Core/Services/PublishResultType.cs

@@ -49,6 +49,11 @@
         /// </summary>
         SuccessUnpublishMandatoryCulture = 6,
 
+        /// <summary>
+        /// The specified document culture was unpublished, and was the last published culture in the document, therefore the document itself was unpublished.
+        /// </summary>
+        SuccessUnpublishLastCulture = 8,
+
         #endregion
 
         #region Success - Mixed
@@ -113,9 +118,9 @@
         FailedPublishContentInvalid = FailedPublish | 8,
 
         /// <summary>
-        /// The document could not be published because it has no publishing flags or values.
+        /// The document could not be published because it has no publishing flags or values or if its a variant document, no cultures were specified to be published.
         /// </summary>
-        FailedPublishNothingToPublish = FailedPublish | 9, // TODO: in ContentService.StrategyCanPublish - weird
+        FailedPublishNothingToPublish = FailedPublish | 9, 
 
         /// <summary>
         /// The document could not be published because some mandatory cultures are missing.

src/Umbraco.Tests/Services/ContentServiceTests.cs

@@ -717,21 +717,8 @@ namespace Umbraco.Tests.Services
         [Test]
         public void Can_Unpublish_Content_Variation()
         {
-            // Arrange
+            var content = CreateEnglishAndFrenchDocument(out var langUk, out var langFr, out var contentType);
 
-            var langUk = new Language("en-GB") { IsDefault = true };
-            var langFr = new Language("fr-FR");
-
-            ServiceContext.LocalizationService.Save(langFr);
-            ServiceContext.LocalizationService.Save(langUk);
-
-            var contentType = MockedContentTypes.CreateBasicContentType();
-            contentType.Variations = ContentVariation.Culture;
-            ServiceContext.ContentTypeService.Save(contentType);
-
-            IContent content = new Content("content", Constants.System.Root, contentType);
-            content.SetCultureName("content-fr", langFr.IsoCode);
-            content.SetCultureName("content-en", langUk.IsoCode);
             content.PublishCulture(CultureImpact.Explicit(langFr.IsoCode, langFr.IsDefault));
             content.PublishCulture(CultureImpact.Explicit(langUk.IsoCode, langUk.IsDefault));
             Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
@@ -761,6 +748,185 @@ namespace Umbraco.Tests.Services
 
         }
 
+        [Test]
+        public void Can_Publish_Culture_After_Last_Culture_Unpublished()
+        {
+            var content = CreateEnglishAndFrenchDocument(out var langUk, out var langFr, out var contentType);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            var unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); //first culture
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            unpublished = ServiceContext.ContentService.Unpublish(content, langFr.IsoCode); //last culture
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishLastCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            published = ServiceContext.ContentService.SaveAndPublish(content, langUk.IsoCode);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            
+            content = ServiceContext.ContentService.GetById(content.Id); //reget
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            
+        }
+
+        
+
+        [Test]
+        public void Unpublish_All_Cultures_Has_Unpublished_State()
+        {
+            var content = CreateEnglishAndFrenchDocument(out var langUk, out var langFr, out var contentType);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(published.Success);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            var unpublished = ServiceContext.ContentService.Unpublish(content, langFr.IsoCode); //first culture
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.AreEqual(PublishedState.Published, content.PublishedState); //still published
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+
+            unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); //last culture
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishLastCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+            Assert.AreEqual(PublishedState.Unpublished, content.PublishedState); //the last culture was unpublished so the document should also reflect this
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+            Assert.AreEqual(PublishedState.Unpublished, content.PublishedState); //just double checking
+            Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode));          
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+        }
+
+        [Test]
+        public void Unpublishing_Mandatory_Language_Unpublishes_Document()
+        {
+            var langUk = new Language("en-GB") { IsDefault = true, IsMandatory = true };
+            var langFr = new Language("fr-FR");
+
+            ServiceContext.LocalizationService.Save(langFr);
+            ServiceContext.LocalizationService.Save(langUk);
+
+            var contentType = MockedContentTypes.CreateBasicContentType();
+            contentType.Variations = ContentVariation.Culture;
+            ServiceContext.ContentTypeService.Save(contentType);
+
+            IContent content = new Content("content", Constants.System.Root, contentType);
+            content.SetCultureName("content-fr", langFr.IsoCode);
+            content.SetCultureName("content-en", langUk.IsoCode);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(published.Success);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+            
+            var unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); //unpublish mandatory lang
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishMandatoryCulture, unpublished.Result);            
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode)); //remains published
+            Assert.AreEqual(PublishedState.Unpublished, content.PublishedState); 
+        }
+
+        [Test]
+        public void Unpublishing_Already_Unpublished_Culture()
+        {
+            var content = CreateEnglishAndFrenchDocument(out var langUk, out var langFr, out var contentType);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(published.Success);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            var unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); 
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            //Change some data since Unpublish should always Save
+            content.SetCultureName("content-en-updated", langUk.IsoCode);
+
+            unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); //unpublish again
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishAlready, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+
+            content = ServiceContext.ContentService.GetById(content.Id);
+            //ensure that even though the culture was already unpublished that the data was still persisted
+            Assert.AreEqual("content-en-updated", content.GetCultureName(langUk.IsoCode));
+        }
+
+        [Test]
+        public void Publishing_No_Cultures_Still_Saves()
+        {
+            var content = CreateEnglishAndFrenchDocument(out var langUk, out var langFr, out var contentType);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(published.Success);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            //Change some data since SaveAndPublish should always Save
+            content.SetCultureName("content-en-updated", langUk.IsoCode);
+
+            var saved = ServiceContext.ContentService.SaveAndPublish(content, new string [] { }); //save without cultures            
+            Assert.AreEqual(PublishResultType.FailedPublishNothingToPublish, saved.Result);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+            //ensure that even though nothing was published that the data was still persisted
+            Assert.AreEqual("content-en-updated", content.GetCultureName(langUk.IsoCode));
+        }
+
+
         [Test]
         public void Pending_Invariant_Property_Changes_Affect_Default_Language_Edited_State()
         {
@@ -811,17 +977,7 @@ namespace Umbraco.Tests.Services
         [Test]
         public void Can_Publish_Content_Variation_And_Detect_Changed_Cultures()
         {
-            // Arrange
-
-            var langGB = new Language("en-GB") { IsDefault = true };
-            var langFr = new Language("fr-FR");
-
-            ServiceContext.LocalizationService.Save(langFr);
-            ServiceContext.LocalizationService.Save(langGB);
-
-            var contentType = MockedContentTypes.CreateBasicContentType();
-            contentType.Variations = ContentVariation.Culture;
-            ServiceContext.ContentTypeService.Save(contentType);
+            CreateEnglishAndFrenchDocumentType(out var langUk, out var langFr, out var contentType);
 
             IContent content = new Content("content", Constants.System.Root, contentType);
             content.SetCultureName("content-fr", langFr.IsoCode);
@@ -832,8 +988,8 @@ namespace Umbraco.Tests.Services
 
             //re-get
             content = ServiceContext.ContentService.GetById(content.Id);
-            content.SetCultureName("content-en", langGB.IsoCode);
-            published = ServiceContext.ContentService.SaveAndPublish(content, langGB.IsoCode);
+            content.SetCultureName("content-en", langUk.IsoCode);
+            published = ServiceContext.ContentService.SaveAndPublish(content, langUk.IsoCode);
             //audit log will only show that english was published
             lastLog = ServiceContext.AuditService.GetLogs(content.Id).Last();
             Assert.AreEqual($"Published languages: English (United Kingdom)", lastLog.Comment);
@@ -3013,5 +3169,28 @@ namespace Umbraco.Tests.Services
             var repository = new DocumentRepository(accessor, AppCaches.Disabled, Logger, contentTypeRepository, templateRepository, tagRepository, languageRepository);
             return repository;
         }
+
+        private void CreateEnglishAndFrenchDocumentType(out Language langUk, out Language langFr, out ContentType contentType)
+        {
+            langUk = new Language("en-GB") { IsDefault = true };
+            langFr = new Language("fr-FR");
+            ServiceContext.LocalizationService.Save(langFr);
+            ServiceContext.LocalizationService.Save(langUk);
+
+            contentType = MockedContentTypes.CreateBasicContentType();
+            contentType.Variations = ContentVariation.Culture;
+            ServiceContext.ContentTypeService.Save(contentType);
+        }
+
+        private IContent CreateEnglishAndFrenchDocument(out Language langUk, out Language langFr, out ContentType contentType)
+        {
+            CreateEnglishAndFrenchDocumentType(out langUk, out langFr, out contentType);
+
+            IContent content = new Content("content", Constants.System.Root, contentType);
+            content.SetCultureName("content-fr", langFr.IsoCode);
+            content.SetCultureName("content-en", langUk.IsoCode);
+
+            return content;
+        }
     }
 }