Don't propagate or save content when publishing an unsaved draft

craftcms · Feb 5, 2024 · 8fd13ff · 8fd13ff
1 parent 59c4355
commit 8fd13ff
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 32 deletions.
diff --git a/CHANGELOG-WIP.md b/CHANGELOG-WIP.md
@@ -437,6 +437,7 @@
 - `craft\services\AssetIndexer::indexFolderByListing()` now has a `$volume` argument in place of `$volumeId`.
 - `craft\services\AssetIndexer::storeIndexList()` now has a `$volume` argument in place of `$volumeId`.
 - `craft\services\Elements::duplicateElement()` now has an `$asUnpublishedDraft` argument, and no longer has a `$trackDuplication` argument.
+- `craft\services\Elements::saveElement()` now has a `$saveContent` argument.
 - `craft\services\Plugins::getPluginLicenseKeyStatus()` now returns a `craft\enums\LicenseKeyStatus` case.
 - `craft\services\ProjectConfig::saveModifiedConfigData()` no longer has a `$writeExternalConfig` argument, and no longer writes out updated project config YAML files.
 - `craft\services\Users::activateUser()` now has a `void` return type, and throws an `InvalidElementException` in case of failure.

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,7 @@
 - Renamed `craft\log\Dispatcher::getTargets()` to `getDefaultTargets()`. ([#14283](https://github.com/craftcms/cms/pull/14283))
 - `craft\elements\NestedElementManager` now passes the owner element to the `valueSetter` closure.
 - `craft\helpers\Cp::elementCardHtml()` now accepts an `attributes` config key.
+- `craft\services\Elements::saveElement()` now has a `$saveContent` argument.
 - Fixed a bug where newly-created inline Matrix entries could have validation errors.
 - Fixed a bug where selecting a field type via keyboard was unreliable if a field type’s icon contained a `<style>` tag.
 - Fixed an error that occurred if a component select was used for components with non-numeric IDs.

diff --git a/src/services/Drafts.php b/src/services/Drafts.php
@@ -394,7 +394,9 @@ public function removeDraftData(ElementInterface $draft): void
         }
 
         try {
-            if ($draft->hasErrors() || !Craft::$app->getElements()->saveElement($draft, false)) {
+            // no need to propagate or save content here – and it could end up overriding any
+            // content changes made to other sites from a previous onAfterPropagate(), etc.
+            if ($draft->hasErrors() || !Craft::$app->getElements()->saveElement($draft, false, false, saveContent: false)) {
                 throw new InvalidElementException($draft, "Draft $draft->id could not be applied because it doesn't validate.");
             }
             Db::delete(Table::DRAFTS, [

diff --git a/src/services/Elements.php b/src/services/Elements.php
@@ -1223,6 +1223,8 @@ private function ensureBulkOp(callable $callback): void
      * (this will happen via a background job if this is a web request)
      * @param bool $forceTouch Whether to force the `dateUpdated` timestamp to be updated for the element,
      * regardless of whether it’s being resaved
+     * @param bool|null $crossSiteValidate Whether the element should be validated across all supported sites
+     * @param bool $saveContent Whether the element’s content should be saved
      * @return bool
      * @throws ElementNotFoundException if $element has an invalid $id
      * @throws Exception if the $element doesn’t have any supported sites
@@ -1235,6 +1237,7 @@ public function saveElement(
         ?bool $updateSearchIndex = null,
         bool $forceTouch = false,
         ?bool $crossSiteValidate = false,
+        bool $saveContent = true,
     ): bool {
         // Force propagation for new elements
         $propagate = !$element->id || $propagate;
@@ -1250,6 +1253,7 @@ public function saveElement(
             $updateSearchIndex,
             forceTouch: $forceTouch,
             crossSiteValidate: $crossSiteValidate,
+            saveContent: $saveContent,
         );
         $element->duplicateOf = $duplicateOf;
         return $success;
@@ -3365,6 +3369,8 @@ public function propagateElement(
      * @param array|null $supportedSites The element’s supported site info, indexed by site ID
      * @param bool $forceTouch Whether to force the `dateUpdated` timestamp to be updated for the element,
      * regardless of whether it’s being resaved
+     * @param bool $crossSiteValidate Whether the element should be validated across all supported sites
+     * @param bool $saveContent Whether the element’s content should be saved
      * @return bool
      * @throws ElementNotFoundException if $element has an invalid $id
      * @throws UnsupportedSiteException if the element is being saved for a site it doesn’t support
@@ -3378,6 +3384,7 @@ private function _saveElementInternal(
         ?array $supportedSites = null,
         bool $forceTouch = false,
         bool $crossSiteValidate = false,
+        bool $saveContent = true,
     ): bool {
         /** @var ElementInterface|DraftBehavior|RevisionBehavior $element */
         $isNewElement = !$element->id;
@@ -3424,9 +3431,6 @@ private function _saveElementInternal(
             return false;
         }
 
-        // Get the field layout
-        $fieldLayout = $element->getFieldLayout();
-
         // Get the sites supported by this element
         $supportedSites = $supportedSites ?? ArrayHelper::index(ElementHelper::supportedSitesForElement($element), 'siteId');
 
@@ -3487,6 +3491,7 @@ private function _saveElementInternal(
             $originalFirstSave,
             $originalPropagateAll,
             $forceTouch,
+            $saveContent,
             $trackChanges,
             $dirtyAttributes,
             $updateSearchIndex,
@@ -3632,19 +3637,21 @@ private function _saveElementInternal(
                     }
                 }
 
-                // Set the field values
-                $content = [];
-                if ($fieldLayout) {
-                    foreach ($fieldLayout->getCustomFields() as $field) {
-                        if ($field::dbType() !== null) {
-                            $serializedValue = $field->serializeValue($element->getFieldValue($field->handle), $element);
-                            if ($serializedValue !== null) {
-                                $content[$field->layoutElement->uid] = $serializedValue;
+                if ($saveContent) {
+                    // Set the field values
+                    $content = [];
+                    if ($fieldLayout) {
+                        foreach ($fieldLayout->getCustomFields() as $field) {
+                            if ($field::dbType() !== null) {
+                                $serializedValue = $field->serializeValue($element->getFieldValue($field->handle), $element);
+                                if ($serializedValue !== null) {
+                                    $content[$field->layoutElement->uid] = $serializedValue;
+                                }
                             }
                         }
                     }
+                    $siteSettingsRecord->content = $content ?: null;
                 }
-                $siteSettingsRecord->content = $content ?: null;
 
                 // Save the site settings record
                 if (!$siteSettingsRecord->save(false)) {
@@ -3692,6 +3699,7 @@ private function _saveElementInternal(
                                     $siteId,
                                     $siteElement,
                                     crossSiteValidate: $runValidation && $crossSiteValidate,
+                                    saveContent: $saveContent,
                                 )) {
                                     throw new InvalidConfigException();
                                 }
@@ -3832,7 +3840,8 @@ private function _saveElementInternal(
      * @param array $supportedSites The element’s supported site info, indexed by site ID
      * @param int $siteId The site ID being propagated to
      * @param ElementInterface|false|null $siteElement The element loaded for the propagated site
-     * @param bool $crossSiteValidate Whether we should "live" validate the element across all sites
+     * @param bool $crossSiteValidate Whether the element should be validated across all supported sites
+     * @param bool $saveContent Whether the element’s content should be saved
      * @retrun bool
      * @throws Exception if the element couldn't be propagated
      */
@@ -3842,6 +3851,7 @@ private function _propagateElement(
         int $siteId,
         ElementInterface|false|null &$siteElement = null,
         bool $crossSiteValidate = false,
+        bool $saveContent = true,
     ): bool {
         // Make sure the element actually supports the site it's being saved in
         if (!isset($supportedSites[$siteId])) {
@@ -3926,23 +3936,25 @@ private function _propagateElement(
             return $attribute !== 'title' && $attribute !== 'slug';
         }));
 
-        // Copy any non-translatable field values
-        if ($isNewSiteForElement) {
-            // Copy all the field values
-            $siteElement->setFieldValues($element->getFieldValues());
-        } else {
-            $fieldLayout = $element->getFieldLayout();
-
-            if ($fieldLayout !== null) {
-                // Only copy the non-translatable field values
-                foreach ($fieldLayout->getCustomFields() as $field) {
-                    // Has this field changed, and does it produce the same translation key as it did for the initial element?
-                    if (
-                        $element->isFieldDirty($field->handle) &&
-                        $field->getTranslationKey($siteElement) === $field->getTranslationKey($element)
-                    ) {
-                        // Copy the initial element’s value over
-                        $siteElement->setFieldValue($field->handle, $element->getFieldValue($field->handle));
+        if ($saveContent) {
+            // Copy any non-translatable field values
+            if ($isNewSiteForElement) {
+                // Copy all the field values
+                $siteElement->setFieldValues($element->getFieldValues());
+            } else {
+                $fieldLayout = $element->getFieldLayout();
+
+                if ($fieldLayout !== null) {
+                    // Only copy the non-translatable field values
+                    foreach ($fieldLayout->getCustomFields() as $field) {
+                        // Has this field changed, and does it produce the same translation key as it did for the initial element?
+                        if (
+                            $element->isFieldDirty($field->handle) &&
+                            $field->getTranslationKey($siteElement) === $field->getTranslationKey($element)
+                        ) {
+                            // Copy the initial element’s value over
+                            $siteElement->setFieldValue($field->handle, $element->getFieldValue($field->handle));
+                        }
                     }
                 }
             }
@@ -3959,7 +3971,15 @@ private function _propagateElement(
         $siteElement->propagating = true;
         $siteElement->propagatingFrom = $element;
 
-        if ($this->_saveElementInternal($siteElement, $crossSiteValidate, false, null, $supportedSites) === false) {
+        $success = $this->_saveElementInternal(
+            $siteElement,
+            $crossSiteValidate,
+            false,
+            supportedSites: $supportedSites,
+            saveContent: $saveContent
+        );
+
+        if (!$success) {
             // if the element we're trying to save has validation errors, notify original element about them
             if ($siteElement->hasErrors()) {
                 return $this->_crossSiteValidationErrors($siteElement, $element);