When a replication occurs the following steps are taken.

  1. Check for conflicts (i.e. existing conflicts)
  2. Update the source workspace (e.g. pull upstream into active)
  3. Check for conflicts (i.e. conflicts discovered after updating)
  4. Deploy changes to the target workspace (e.g. push active to upstream)

The problem is that nothing is done if conflicts exist, thus the user unknowingly may push conflicts to an upstream workspace, such as the Live workspace.

Suggested Resolution

The suggested resolution is to allow the user to decide whether or not to proceed with pushing upstream if there are conflicts. The revised steps in bold should look like:

  1. Check for conflicts (i.e. existing conflicts)
  2. Abort replication if there are conflicts and "abort on conflicts" flag is true (i.e. return failed replication log)
  3. Update the source workspace (e.g. pull upstream into active)
  4. Check for conflicts (i.e. conflicts discovered after updating)
  5. Abort replication if there are conflicts and "abort on conflicts" flag is true (i.e. return failed replication log)
  6. Deploy changes to the target workspace (e.g. push active to upstream)

Consequently, this means the caller of ReplicatorManager needs to handle the failed replication log appropriately.

Part 1: Revise ReplicatorManager

For ReplicatorManager this means giving control back to the caller of ReplicatorManager::replicate in the event of conflicts. Specifically, this looks like:

  1. Add a flag to the ReplicatorManager to tell it to proceed with replicating upstream after an update even if there are conflicts.
  2. If there are conflicts in ReplicatorManager::replicate and the abort flag is set to abort on conflicts, return a failed replication log and do *not* proceed with replicating upstream, i.e. abort just after the "update" call.

Note: if it is possible to have the "abort on conflicts" flag keep track of a sequence index this would allow handling the case where: (a) a user attempts replication, (b) it aborts due to conflicts, (c) the user reviews the conflicts, (d) the user attempts replication again, but there were new conflicts introduced since step "b" and they unknowingly replicate those conflicts upstream.

Part 2: Revise Callers of ReplicatorManager

For callers of ReplicatorManager, they will need to know that when a failed replication log is returned they need to handle it properly. I don't believe there currently is handling for failure, so we will want to introduce handling for conflicts as well as a general case (assuming it makes sense to also introduce that now).

These include:

  1. Drupal\workspace\Plugin\RulesAction\ReplicateContent
  2. Drupal\workspace\EventSubscriber\WorkbenchModerationSubscriber
  3. Drupal\deploy\Entity\Form\ReplicationForm
  4. Drupal\deploy\Form\ReplicationActionForm

Here is how we will handle each case in this ticket:

  1. Drupal\workspace\Plugin\RulesAction\ReplicateContent: Deal with later; Find a way to cancel the Rules action
  2. Drupal\workspace\EventSubscriber\WorkbenchModerationSubscriber: Use hook entity type alter to allow the user to pass the "override" flag if there are conflicts
  3. Drupal\deploy\Entity\Form\ReplicationForm: Deal with later
  4. Drupal\deploy\Form\ReplicationActionForm: Deal with later

DONE:

  1. Show a message to the user if there are conflicts and given them the option of proceeding with replication even if there are conflicts
  2. Show a message to the user when replication fails due to conflicts
  3. Allow the user to see a list of conflicts in a Workspace
  4. If a replication fails, the WorkbenchModerationSubscriber will need to revert back the state.
  5. If the list of conflicted revisions does not have a link to the entity, so there issue where the link would go to an entity that doesnt exist on the active workspace

TODO:

  1. Write tests
  2. Create tickets for the "deal with later" items like handling situations like when Rules is used or Deploy
CommentFileSizeAuthor
#65 interdiff.txt3.76 KBjeqq
#65 workspace_should_report-2791789-65.patch41.17 KBjeqq
#59 interdiff.txt517 bytesjeqq
#59 workspace_should_report-2791789-59.patch41.05 KBjeqq
#54 interdiff.txt4.19 KBjeqq
#54 workspace_should_report-2791789-54.patch41.05 KBjeqq
#46 interdiff.txt2.08 KBjeqq
#46 workspace_should_report-2791789-46.patch40.58 KBjeqq
#45 interdiff.txt643 bytesjeqq
#45 workspace_should_report-2791789-45.patch40.7 KBjeqq
#41 workspace_should_report-2791789-41.patch40.87 KBjeqq
#40 interdiff.txt6.47 KBjeqq
#40 workspace_should_report-2791789-40.patch40.75 KBjeqq
#37 Screenshot from 2016-10-12 06-20-02.png42.31 KBjosephdpurcell
#37 interdiff_34-37.txt5.18 KBjosephdpurcell
#37 2791789_37.patch35.8 KBjosephdpurcell
#35 interdiff_32-34.txt682 bytesjosephdpurcell
#34 2791789-34.patch32.62 KBphenaproxima
#32 interdiff_31-32.txt1.95 KBjosephdpurcell
#32 2791789_32.patch32.62 KBjosephdpurcell
#31 interdiff_29-31.txt1.35 KBjosephdpurcell
#31 2791789_31.patch32.37 KBjosephdpurcell
#29 interdiff_28-29.txt5.86 KBjosephdpurcell
#29 2791789_29.patch32.2 KBjosephdpurcell
#28 interdiff-2791789-25-28.txt2.93 KBphenaproxima
#28 2791789-28.patch30.06 KBphenaproxima
#25 interdiff_22-25.txt4.39 KBjosephdpurcell
#25 2791789_25.patch27.04 KBjosephdpurcell
#23 Screenshot from 2016-10-05 12-30-10.png113.89 KBjosephdpurcell
#22 Screenshot from 2016-10-05 12-24-21.png113.51 KBjosephdpurcell
#22 Screenshot from 2016-10-05 12-24-05.png108.71 KBjosephdpurcell
#22 Screenshot from 2016-10-05 12-23-47.png89.68 KBjosephdpurcell
#22 interdiff_19-22.txt12.96 KBjosephdpurcell
#22 2791789_22.patch27.84 KBjosephdpurcell
#21 Screenshot from 2016-10-05 10-04-27.png42.84 KBjosephdpurcell
#19 Screenshot from 2016-10-04 15-51-57.png71.17 KBjosephdpurcell
#17 2791789_17.patch28.61 KBjosephdpurcell
#17 interdiff_16-17.txt7.01 KBjosephdpurcell
#15 interdiff_14-15.txt16.43 KBjosephdpurcell
#15 2791789_15.patch24.29 KBjosephdpurcell
#14 interdiff_12-14.txt2.18 KBjosephdpurcell
#14 2791789_13.patch15.07 KBjosephdpurcell
#13 interdiff_11-12.txt3.06 KBjosephdpurcell
#13 2791789_12.patch13.64 KBjosephdpurcell
#11 workspace-conflicts-exist-revised-copy-and-input.png93.4 KBjosephdpurcell
#11 2791789_11.patch11.69 KBjosephdpurcell
#9 workspace-conflicts-none.png61.63 KBjosephdpurcell
#9 workspace-conflicts-list.png24.7 KBjosephdpurcell
#9 workspace-conflicts-exist.png68.21 KBjosephdpurcell
#9 2791789_9.patch10.62 KBjosephdpurcell
#6 2791789.patch2.88 KBjosephdpurcell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
josephdpurcell’s picture

Issue summary: View changes

Updated description to reflect specifics of next steps and clarify the problem.

Note: instead of adding the "abort on conflicts" flag to the ReplicationTask, I'm adding it to the ReplicatorManager since this is a problem specific to the ReplicatorManager implementation--particularly, not all ReplicatorInterfaces will do an "update" before a "replicate".

josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

This is a sketch of what needs to happen in the ReplicationManager.

Next, we need to pull $is_aborted_on_conflict flag out to some shared state and then modify the callers of ReplicationManager to be able to change that flag based on user input, i.e. if the user does want to push conflict.s

josephdpurcell’s picture

Status: Active » Needs work
josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

This patch is very rough but at least gives a proof of concept of what the user might see. I've attached screenshots that show what the user would see if a Workspace has no conflicts, when there are conflicts, and then what the revision list currently looks like.

On a high level, here's what this patch does:

  • When the user goes to publish a workspace (i.e. edit workspace form)...
  • If there are no conflicts, a green message "No conflicts" is shown
  • If there are conflicts, a red message "Conflicts exist" is shown with a link to the list of conflicts
  • If there are conflicts, a checkbox is shown to the user to allow overriding the replication abort

Next steps:

  • Avoid using $_SESSION to pass the state to ReplicationManager
  • Move the messages from drupal_set_message to a render array to avoid the problem where on form submission those messages show
  • Confirm that the copy/messages to the user make sense
  • Revise the list of conflicted revisions to be a table showing label, entity type, and date modified
  • If the list of conflicted revisions has a link to the revision, figure out how to ensure the user is in the correct workspace to view that revision
  • Display a message to the user if a replication is aborted (consider impacts to other modules)
  • Write tests
josephdpurcell’s picture

More specific next steps:

  1. Use drupal_static instead of $_SESSION
  2. Move the messages from drupal_set_message to a render array to avoid the problem where on form submission those messages show
  3. Change the copy for the warning message and abort flag to be more scary and say more words
  4. Revise the list of conflicted revisions to be a table showing label, entity type, and date modified
  5. If the list of conflicted revisions has a link to the revision, figure out how to ensure the user is in the correct workspace to view that revision
  6. Display a message to the user if a replication is aborted (consider impacts to other modules), also display one if there was a success (perhaps also link to the upstream)
  7. Write tests
josephdpurcell’s picture

Here are the latest changes:

  1. Fixed.
  2. Fixed.
  3. Fixed. I also changed the input from a checkbox to a radio to allow for more explanation of what each option is (see screenshot).
  4. TODO
  5. TODO -- suggestion is to not show the revision, but just link to the entity as it is in the workspace, i.e. show what will be displayed if those changes are pushed upstream
  6. TODO
  7. TODO
josephdpurcell’s picture

Issue summary: View changes

Here are the latest changes:

  1. Fixed in 11
  2. Fixed in 11
  3. Fixed in 11
  4. TODO
  5. TODO -- suggestion is to not show the revision, but just link to the entity as it is in the workspace, i.e. show what will be displayed if those changes are pushed upstream
  6. Partially fixed, see comment below
  7. TODO

So, the only part addressed here is item 6. This patch *should* work in theory, but for some reason the following is taking place:

  1. Create an entity on live
  2. Create a workspace with upstream live
  3. Switch to new workspace
  4. Update the new workspace
  5. Switch to live
  6. Edit the entity on live
  7. Switch to new workspace
  8. Edit the entity on new workspace
  9. Publish the new workspace

You'll see a success message, which makes sense since the moderation state of the new workspace successfully transitioned. However, it doesn't appear the abort step is being run, which is very confusing (i.e. line 85 of ReplicationManager doesn't trigger).

So, two things need done:

1. If a replication fails, the WorkbenchModerationSubscriber will need to revert back the state (is this possible?).
2. Figure out why the replication isn't aborting if there are conflicts.

This seems like a perfect case for a test!

Note: I've moved the list of TODOs to the description.

josephdpurcell’s picture

josephdpurcell’s picture

So, it turns out the check for the replication log status was incorrect. I identified the same problem with Deploy #2809325: Correctly access replication log status value.

So, this patch fixes that part. Still need to do tasks 4-8 in the description.

josephdpurcell’s picture

This patch uses a list builder for the list of conflicts.

josephdpurcell’s picture

Issue summary: View changes

Updating description to clarify whats next.

josephdpurcell’s picture

This patch takes care of reverting the workflow state on failed replication, however it uses a static variable which is... not ideal.

I've added a TODO which is to reconcile the WorkspaceForm and WorkbenchModerationSubscriber messages (see screenshot).

Two more items to go plus writing tests. I'm not sure I will get to these items, so if someone picks them up message me and I can give some clarity on where to get started.

josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

Here is a screenshot showing the case where multiple messages show at the same time:

  1. Failed replication message from WorkbenchModerationSubscriber::onTransition
  2. Successful save of any edits to the workspace, such as replication settings, from WorkspaceForm::save
  3. Conflicts exist warning message from WorkspaceForm::form
josephdpurcell’s picture

Issue summary: View changes

Updating the TODO list, which contains all the summarized info from the comments.

josephdpurcell’s picture

Regarding the problem of the conflicts listing being linked out to entities that may not exist on the current workspace, I took a look this morning and see the following options:

  1. Have the conflicts list show just text about the conflict (e.g. label, entity type, modified) with no actions available.
  2. Have the conflicts list show text about the conflict as well as an "Operations" column that will allow the user to switch to the workspace that has the entity and go to the view of that entity

I like the first option since it keeps it simple (see screenshot). However, the second option may be more useful to the user. The problem with the second option is we would likely have to write a form of somekind to handle the switching of the workspace and redirect to the entity. This may pose a usability problem of not being able to easily get back to the conflicts list. Perhaps there is a view that displays the revision regardless of which workspace is active?

I could use some input on this as well as how we might consolidate the messages (see screenshot).

josephdpurcell’s picture

Alright, I believe all the major issues are tackled aside from writing tests.

Here are screenshots of what it looks like:

  • Editing a workspace and there are no conflicts screenshot
  • Editing a workspace and there are conflicts, or you've submitted the form without selecting the "go ahead and push conflicts" button screenshot
  • You've successfully published a workspace and its content is replicated upstream screenshot

Setting this as ready to review to get some eyes on it.

josephdpurcell’s picture

FileSize
113.89 KB
phenaproxima’s picture

  1. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -19,14 +31,65 @@ class WorkspaceForm extends ContentEntityForm {
    +          'There are @count conflicts. The replication logic has arbitrarily chosen the entity revision that should be used and it may not have been the one you desired. Proceeding with replication will push these conflicts upstream, which may cause a loss of data. See the full list of conflicts <a href=":link">here</a>.',
    

    This verbiage seems a little intimidating and obscure for the average user, even a techy one :) I would recommend something like this:

    "There are [link to conflict list]3 conflict(s) with the [upstream] workspace[/link]. Pushing changes to [upstream] may result in unexpected behavior or data loss, and cannot be undone. Please proceed with caution."

  2. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -82,32 +145,102 @@ class WorkspaceForm extends ContentEntityForm {
    +        if ($previous_workflow_state == NULL) {
    

    Should be !isset() or === because any falsy value will == NULL.

josephdpurcell’s picture

Those two items are taken care of in this patch.

I also remove the conflict list template file that isn't used, and modify the list builder to use entity keys.

jeqq’s picture

Status: Needs review » Needs work

This is a quick code review, will do a more detailed review of the functionality later.

  1. +++ b/src/Controller/Component/ConflictListBuilder.php
    @@ -0,0 +1,225 @@
    +    foreach ($entities as $entity) {
    +      $row = $this->buildRow($entity);
    +
    +      if ($row = $this->buildRow($entity)) {
    +        $build['table']['#rows'][$entity->id()] = $row;
    +      }
    +    }
    

    Variable $row is defined twice.

  2. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -19,14 +31,66 @@ class WorkspaceForm extends ContentEntityForm {
    +    $this->entityManager = $entity_manager;
    

    Remove this service if it is not used or inject the EntityTypeManager.

  3. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -19,14 +31,66 @@ class WorkspaceForm extends ContentEntityForm {
    +            ':link' => \Drupal::url('entity.workspace.conflicts', ['workspace' => $workspace->id()]),
    

    Use Drupal\Core\Url insted.

  4. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -82,32 +146,102 @@ class WorkspaceForm extends ContentEntityForm {
    +  /**
    +   * Generate a message render array with the given text.
    +   *
    +   * @param string $type
    +   *   The type of message: status, warning, or error.
    +   * @param string $message
    +   *   The message to create with.
    +   *
    +   * @see \Drupal\Core\Render\Element\StatusMessages
    +   */
    

    Add @return array.

  5. +++ b/src/WorkspaceListBuilder.php
    @@ -89,7 +90,13 @@ class WorkspaceListBuilder extends EntityListBuilder {
    +      'url' => $entity->urlInfo('conflicts', ['workspace' => $entity->id()]),
    

    Use Drupal\Core\Entity\EntityInterface::toUrl() instead, Drupal\Core\Entity\EntityInterface::urlInfo() is deprecated.

jeqq’s picture

Issue tags: +Needs tests
phenaproxima’s picture

Added support for the event stuff over in Replication: #2814055: Allow modules to react to replication events.

josephdpurcell’s picture

This patch addresses some of the code cleanup items.

jeqq’s picture

Status: Needs review » Needs work
+++ b/src/WorkspaceListBuilder.php
@@ -85,11 +85,17 @@ class WorkspaceListBuilder extends EntityListBuilder {
+        'url' => $entity->url('activate-form', ['query' => ['destination' => $entity->url('collection')]]),
...
+      'url' => $entity->url('conflicts', ['workspace' => $entity->id()]),

I would insist on replacing deprecated urlInfo() and url() with \Drupal\Core\Entity\EntityInterface::toUrl

I've done some manual testing. Seems that changes are pulled when a conflict occurs (but they shouldn't). Steps to reproduce:
1. Enable Deploy module
2. Create a node on Live
3. Deploy to Stage
4. Edit the node on Live
5. Edit the node on Stage
6. Deploy from Live to Stage

An error message will be displayed: Deployment error. On live workspace now I can see changes made on Stage workspace - that means that changes were not pushed from Live to stage but they were pulled from Stage to Live.

On Live workspace configuration page I see the conflict error message: There are 1 conflict(s) with the Stage workspace. Pushing changes to Stage may result in unexpected behavior or data loss, and cannot be undone. Please proceed with caution. , on Stage workspace configuration page: There are no conflicts.

Now, if I select No, go ahead and push any conflicts to the upstream. on Live and do deploy, it won't work because the entity has been modified with changes from Stage.

I think we need tests for this.

josephdpurcell’s picture

I completely agree--I used the wrong method. This patch uses toUrl.

I agree that tests on this are a high priority given its complexity.

The issue described above with Deploy module is expected. Essentially what is happening is the default behavior of replication has changed by this patch from "don't abort on conflicts" to "abort on conflicts."

In the spirit of not altering behavior of other modules (i.e. deploy) I'm going to change the default back. This is a straightforward change and doesn't prevent us from still meeting the objective of this ticket.

josephdpurcell’s picture

This patch sets the default value of "workspace_is_aborted_on_conflicts" to FALSE. This preserves the prior behavior of replicating conflicts upstream.

IMPORTANT! Any module calling this should be updated to handle this flag and by default set it to TRUE so that conflicts are not pushed upstream without the user's consent.

josephdpurcell’s picture

Status: Needs work » Needs review

Forgot to set to needs review.

phenaproxima’s picture

Fixed a one-character typo that broke the event dispatching stuff.

josephdpurcell’s picture

FileSize
682 bytes

Here's the interdiff.

jeqq’s picture

Status: Needs review » Needs work
  1. +++ b/src/Entity/Form/WorkspaceForm.php
    @@ -19,14 +30,62 @@ class WorkspaceForm extends ContentEntityForm {
    +   *   The confict tracking service.
    

    A typo here.

  2. +++ b/src/Form/UpdateForm.php
    @@ -163,7 +163,7 @@ class UpdateForm extends ConfirmFormBase {
    +      if (($response instanceof ReplicationLogInterface) && ($response->get('ok')->value === TRUE)) {
    
    +++ b/src/Plugin/RulesAction/ReplicateContent.php
    @@ -99,7 +99,7 @@ class ReplicateContent extends RulesActionBase implements ContainerFactoryPlugin
    +    if ($result->get('ok')->value === TRUE) {
    

    Let's not check the type, use just "==" because sometimes the value can be "1" for $response->get('ok')->value.

@josephdpurcell Is there a way to detect the conflict before setting the success replication message in UdateForm::submitForm()?. At the moment if we have a conflict and try to update the workspace, it shows the success message: Stage has been updated with content from Live. but actually it doesn't replicate the entity because of the conflict. I can see the conflict message only if I go to workspace edit page. This looks wrong in my opinion from UX perspective. The solution would be to display the conflict message with the link to the workspace edit page, where the user can select the solution.

josephdpurcell’s picture

  1. Typo is fixed.
  2. Changed, but I caution: loose truthy checks are scary: http://php.net/manual/en/types.comparisons.php. If Entity API is sometimes returning an integer and sometimes a boolean for the value of a boolean field, that should be a bug in Entity API, imo.
  3. I followed the pattern of the Workspace edit form, so now if there are conflicts after doing update it will say: "Test 7 has been updated with content from Live, but there are 1 conflict(s) with the Live workspace." (see screenshot)
jeqq’s picture

Status: Needs review » Needs work

@josephdpurcell This still needs tests. Do you have time to implement tests for it? If not I'll work on this.

josephdpurcell’s picture

Unfortunately I do not have time for tests today. I may be able to pitch in on this later this week.

In the back of my mind for testing, I considered one scenario that involved taking a few different entity types (menu items, block content, taxonomy terms, nodes), creating a conflict between workspaces, and ensuring their labels show up in the workspace's conflict list. It seemed like a pretty straightforward high-level test to write.

jeqq’s picture

Added a test and fixed bugs discovered while implementing itt. The test implements the use-case described by @josephdpurcell in #39.

jeqq’s picture

Status: Needs review » Needs work

The last submitted patch, 41: workspace_should_report-2791789-41.patch, failed testing.

The last submitted patch, 41: workspace_should_report-2791789-41.patch, failed testing.

The last submitted patch, 41: workspace_should_report-2791789-41.patch, failed testing.

jeqq’s picture

jeqq’s picture

Status: Needs review » Needs work

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

josephdpurcell’s picture

And this is why testing is important! Thank you so much @jeqq, that is an important find.

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

The last submitted patch, 46: workspace_should_report-2791789-46.patch, failed testing.

jeqq’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 54: workspace_should_report-2791789-54.patch, failed testing.

The last submitted patch, 54: workspace_should_report-2791789-54.patch, failed testing.

The last submitted patch, 54: workspace_should_report-2791789-54.patch, failed testing.

jeqq’s picture

Status: Needs review » Needs work

The last submitted patch, 59: workspace_should_report-2791789-59.patch, failed testing.

The last submitted patch, 59: workspace_should_report-2791789-59.patch, failed testing.

The last submitted patch, 59: workspace_should_report-2791789-59.patch, failed testing.

jeqq’s picture

The test is failing because on the conflicts list page (for Live workspace), after trying to replicate content from Live to Stage(we have 3 conflicts - the correct number of conflicts), labels are not those expected. The expected labels should be those created on Live workspace but here we get, randomly, labels from Live or Stage. I'm not sure if this is the expected behaviour.

@josephdpurcell What do you think about this, maybe this is how it was supposed to work?

josephdpurcell’s picture

@jeqq, interesting. My initial reaction is the "randomness" would be caused by the conflict resolution logic, but given that ConflictListBuilder::load is querying for a specific revision I'm not sure that logic applies.

For reference, let's clarify what the conflict resolution logic looks like so that we know if we're just querying for the entity we know what logic is followed:

  1. Entity 1 is created in Live with revision A
  2. Entity 1 is replicated to Stage and stays at revision A
  3. Entity 1 is updated on Live to revision B
  4. Entity 1 is updated on Stage to revision C
  5. At this point the revision the user sees on Live is B and on Stage is C
  6. When Stage is updated the following happens:
    1. Calculate which revisions are in Live that are not in Stage: assuming we've never run an "update" on Stage before, revisions A and C are in Stage but revision B is not
    2. Load the missing revision B
    3. Save that revision in the Stage workspace
    4. Save will query to see which is the default revision (link)
    5. The getDefaultRevision will build the revision tree and then sort them (link) based on this logic:
      • Non-deleted always win over deleted
      • Higher ASCII sort on revision hash wins
    6. Since C has the highest ASCII value it will become the default revision
  7. At this point Entity 1 has revisions A, B, and C that exist in Stage
  8. Also at this point, the revision the user sees on Live is B and on Stage is C

This is of course simplified.

Now, looking at ConflictListBuilder::load, I don't think it would use the logic above since it's not loading the entity by ID:

+        $entity_revisions[] = $this->entityTypeManager
+          ->getStorage($rev_info['entity_type_id'])
+          ->useWorkspace($workspace_id)
+          ->loadRevision($rev_info['revision_id']);

We would have to look at the conflict tracking logic (link) to see what $rev_info['revision_id'] would be stored as.

Conclusion

All that aside, here is what I think might be a path forward:

Keep the Label of the entity consistent between revisions (i.e. let Stage and Live always have the same Label of the entity) so that we have a consistent identifier of the entity that the user will see.

Use the Body field as the variant, i.e. freely edit the Body field of any revisions to generate conflicts.

Have the test validate the Labels we expect to see on the conflicts list.

I hope these thoughts are helpful.

jeqq’s picture

I've modified the test, now labels remain the same between workspaces. Is changed the body field for the node, descriptions for taxonomy term and menu link content (as @josephdpurcell proposed in #64).

  • jeqq committed 397b02a on 8.x-1.x authored by josephdpurcell
    Issue #2791789 by josephdpurcell, jeqq, phenaproxima: Workspace should...
jeqq’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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