Problem/Motivation

In testing workspaces, I found the following issue:

  1. Enable workspaces, standard profile.
  2. Create an article with title and body, "foo".
  3. Switch to the stage workspace.
  4. Update both the title and body to "bar".
  5. Deploy the stage workspace, switch back to the live workspace.
  6. The article will contain title "bar" but body will remain "foo.

This is caused by:

  1. WorkspacePublisher::publish loads the entity, sets it as the default revision and saves it.
  2. EntityStorageBase::doPreSave sees that no $entity->original exists, so it loads the default entity using loadUnchanged.
  3. Workspaces detects an entity is being loaded and swaps the default entity with the latest entity in the active workspace.
  4. Both $entity->original and $entity are the same and thus ContentEntityStorageBase::hasFieldValueChanged returns FALSE and fails to write to the shared entity tables.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 2998414-14.patch7.08 KBsam152
#14 interdiff.txt2.22 KBsam152
#12 2998414-12.patch7.03 KBsam152
#8 2998414-COMBINED-7.patch11.92 KBsam152
#8 2998414-REVIEW_ONLY-7.patch4.27 KBsam152
#5 2998414-5_TEST_ONLY.patch1.44 KBsam152

Comments

Sam152 created an issue. See original summary.

sam152’s picture

sam152 commented

Not sure what the fix for this is yet, seeing if I can reproduce this in a test.

sam152’s picture

sam152 commented
Related issues: +#2833084: $entity->original doesn't adequately address intentions when saving a revision, +#2859042: Impossible to update an entity revision if the field value you are updating matches the default revision.

I've dug into this and can confirm the body field doesn't update because in ContentEntityStorageBase::hasFieldValueChanged $original->body === $entity->body. Then the following code in EntityStorageBase::doPreSave runs:

    // Load the original entity, if any.
    if ($id_exists && !isset($entity->original)) {
      $entity->original = $this->loadUnchanged($id);
    }

Which workspaces subsequently alters and replaces with the latest revision in the current workspace. So $entity->original is no longer the default revision that is the target of the update, the the same revision that is being saved.

Adding some related issues, since they related to assumptions about $entity->original and its use in hasFieldValueChanged.

sam152’s picture

sam152 commented
Title: Workspaces fails to deploy content in the "body" field » Workspaces fails to deploy content in fields that required dedicated table storage
sam152’s picture

sam152 commented
Status: Active » Needs review
StatusFileSize
new 2998414-5_TEST_ONLY.patch1.44 KB

Test case for this issue.

sam152’s picture

sam152 commented
Issue summary: View changes

Updating IS with a summary of the root cause.

Status: Needs review » Needs work

The last submitted patch, 5: 2998414-5_TEST_ONLY.patch, failed testing. View results

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new 2998414-REVIEW_ONLY-7.patch4.27 KB
new 2998414-COMBINED-7.patch11.92 KB

Here is a fix blocked behind #2968452: Add a way to execute a function in the context of a specific workspace. I can't think of a better way of handling this, without completely rethinking $entity->original and removing the importance of this when entities are persisted. This way it's set to the revision we are updating from, in the context of the workspace we are targeting in a publish, not the active workspace.

The last submitted patch, 8: 2998414-REVIEW_ONLY-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

amateescu commented
Status: Needs review » Needs work

Just now I realize what @FabianX was trying to tell me a few months ago (probably in the issue where the new core module was introduced) :)

I think it would be better if we wrap the whole piece of code inside the first foreach to be executed in the context of the live workspace, that will make the save() process think that it's being done in Live, and there won't be any other code trying to interfere with entity loading.

Also, as a performance improvement, we can also do a multiple (default revision) load for the entity IDs that are about to be saved and assign the original property ourselves, just like the latest patch does.

sam152’s picture

sam152 commented

Thanks for the review!

Works for me. May as well wait for the blocker to be in before making those adjustments.

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new 2998414-12.patch7.03 KB

Addressing feedback now that the blocker is in. Assigning $entity->original is purely a performance optimisation as you suggest, the tests pass without it.

amateescu’s picture

amateescu commented

Looks great!

  1. +++ b/core/modules/workspaces/src/WorkspacePublisher.php
@@ -75,18 +85,25 @@ public function publish() {
+            // When pushing workspace-specific revisions to the default workspace

    This comment needs to be re-wrapped. 

  2. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
@@ -755,4 +755,34 @@ public function testFormCacheForRegularForms() {
+      'title' => 'Foo',
...
+      'body' => 'Foo',
...
+    $node->title = 'Bar';
+    $node->body = 'Bar';

    Just a cosmetic request: let's use different values for the title and the body field, like 'Foo title' and 'Foo body' :)

sam152’s picture

sam152 commented
StatusFileSize
new interdiff.txt2.22 KB
new 2998414-14.patch7.08 KB

Done and done :)

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Awesome!

  • catch committed f3a6d63 on 8.7.x 
    Issue #2998414 by Sam152, amateescu: Workspaces fails to deploy content...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.