WI: Phase H: Conflict management and local workspace merging support

We're a little past 8.5 at this point, so changing the target version in the issue summary. :)

When the Workflow roadmap was run past core committers last week, the feedback was that what's desired, at least as an "MVP" here, is a conflict detection mechanism, which can stop a user from making irreversible data loss mistakes. But the full shebang of conflict resolution system could come later.

@webchick, Workspaces already provides a conflict detection mechanism, with a constraint validator and a form alter which displays a message instead of showing the entity form if that entity has been edited in a different workspace.

Or do you mean something more like the conflict detection mechanism used by d.o for project issue nodes?

I've moved the Conflict module forward, but some help for the UI is still welcome :).

I had a session about it at Drupal Europe - https://www.youtube.com/watch?v=ixlLskzT9c4 , which is worth watching to see the current pretty advanced state of the module.

As a summary - it currently detects changes and conflicts and performs auto-merges wherever possible. If an auto-merge is impossible, which is considered a conflict, then the form with an overview of each conflict is returned to the user.

@amateescu and I started to discuss how to structure the core bits of this functionality so that it is not too opinionated and leaves contrib the ability to freely implement it without core getting in the way. The main takeaway point for core is that we want to provide conflict management without an actual implementation for conflict resolution. This is an approach that was discussed with @catch at various times and it seemed the most promising to achieve a balance between core stability and extensibility.

The bulk of the discussion revolved around EntityChangedConstraintValidator: we need to keep its behavior when the default revision is being updated, but in any other case, since we want to support concurrent editing properly, we need to rely on a more advanced logic to detect conflicts.

The outcome of our brainstorming is summarized by this diagram (hi-res version):

Basically we would have two distinct services for conflict detection and resolution and core would provide a (working) implementation only for the former.

@hchonov:

I was planning to have a look at the Conflict module to figure out whether this approach could problematic for it, but I noticed there were two 8.x branches so I thought it would be easier to just ask your opinion about this. Would it be easy to make 8.x-2.x leverage such an approach? And would it be a problem for 8.x-1.x if 8.7 were released implementing something along these lines?

@plach, the conflict module has two branches - 1.x and 2.x. 1.x has zero functionality, but for some unknown reasons to me multiversion depends on that branch.

I've implemented the whole functionality for conflict management in the 2.x branch. The good news are that the module looks pretty similar to your graph :). The only missing part is that context thing, which I don't understand what it is for, but we can talk further about it.

I would suggest, that you watch my session https://www.youtube.com/watch?v=ixlLskzT9c4 and that we schedule a call to discuss everything in more details. Probably you could also test the concurrent editing feature as shown on the video and play a bit with it and see if it looks good :).

And would it be a problem for 8.x-1.x if 8.7 were released implementing something along these lines?

Unfortunately I cannot speak for the 1.x branch.

The good news are that the module looks pretty similar to your graph :)

Yay!

The only missing part is that context thing, which I don't understand what it is for, but we can talk further about it.

That was only a way to visually differentiate between a form workflow and a REST one: if $entity->save() is invoked in an entity form submit handler, context = FORM; if it's invoked in the scope of a REST API endpoint, context = REST :)

I would suggest, that you watch my session https://www.youtube.com/watch?v=ixlLskzT9c4 and that we schedule a call to discuss everything in more details. Probably you could also test the concurrent editing feature as shown on the video and play a bit with it and see if it looks good :).

I'll have a look, however my main goal here is to ensure that no contrib blockers are introduced by this issue, so I'm fine even if the functionality that Conflict provides doesn't fully match my expectations :)

Unfortunately I cannot speak for the 1.x branch.

I had a quick look and it seems to only expose some APIs, so it shouldn't be affected by these changes.

#11: The diagram makes sense to me. There's one thing that I'd like to see more explicitly in there: how could a decoupled UI provide all the same capabilities as the built-in form-based UI? The 409 response would need to contain metadata, so that the client knows what caused the conflict, where to get the conflicting data, etc. Because the conflict resolver may be implemented on the client.

#12: the arrows starting in that "Context" thing are "Form" and "REST". So in other words: "is the entity being saved via a form or via REST?" is what it's about.

[...] Because the conflict resolver may be implemented on the client.

Definitely, this is exactly what I had in mind. However all that functionality will be provided by a contrib module. I imagine that it would leverage the conflict detection service to retrieve a list of conflicts and represent them in the HTTP response in any way it sees fit.

how could a decoupled UI provide all the same capabilities as the built-in form-based UI

@Wim Leers, the ContentEntityConflictHandler from the conflict module 2.x branch already has such an API:

  /**
   * Returns the entity conflicts.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity_local_original
   *   The locally edited entity.
   * @param \Drupal\Core\Entity\EntityInterface $entity_local_edited
   *   The original not edited entity used to build the form.
   * @param \Drupal\Core\Entity\EntityInterface $entity_server
   *   The unchanged entity loaded from the storage.
   *
   * @return array
   *   An array of conflicts, keyed by the field name having as value the
   *   conflict type.
   */
  public function getConflicts(EntityInterface $entity_local_original, EntityInterface $entity_local_edited, EntityInterface $entity_server);

And the conflict types that might be returned are:

  /**
   * A constant indicating a server only change.
   */
  const CONFLICT_TYPE_SERVER_ONLY = 'server-only-change';

  /**
   * A constant indicating a merge conflict.
   */
  const CONFLICT_TYPE_BOTH = 'server-and-local-change';

CONFLICT_TYPE_SERVER_ONLY conflicts are being merged automatically by the conflict module in order to prevent reverts. This is however valid only when talking about concurrent editing :).

I think having those PHP APIs is not a strong enough guarantee that a conflict resolution UI can be implemented on top of a REST API too.

I think this would need at least one functional test where a 409 response is sent, and its body includes metadata to resolve the conflict. The conflicts to resolve should already be in there — otherwise the 409 response would need to link to some other URL where that information can be fetched, this seems unnecessary.

Well, actually judging by the diagram above, we could make conflict info part of the validation error, this way they can be sent along with the 409 body even with core alone. Contrib can improve on that if more info is needed to actually build a conflict management UI on the client-side.

That seems like a fine MVP :)

But Symfony’s validation errors only allow strings. Meaning it’s very hard to convey complex structures that way: they’d have to be parsed out again by an API client. Still, that’s not this issue’s fault obviously!

@hchonov:

1.x has zero functionality, but for some unknown reasons to me multiversion depends on that branch.

Please refrain from mocking other people's work.

Please refrain from mocking other people's work.

@amateescu, can you differentiate between mocking and stating the facts? There is absolutely no way to perform any conflict resolution with the 1.x branch. But feel free to prove me wrong.

I fully respect the work on the couple of interfaces in the 1.x branch! But interfaces and functionality is something different.

1.x has zero functionality,

In Conflict 1.x there are 4 services, so zero functionality is not a fact, it's your opinion.

but for some unknown reasons to me multiversion depends on that branch.

Multiversion does depend on it, yes, that is a fact. However the "for some unknown reasons to me" does sounds like mocking. As if you're saying "WTF would anyone want to depend on that stupid version of this module for".

There is absolutely no way to perform any conflict resolution with the 1.x branch.

That is correct, but Multiversion, and the deploy suite of modules still uses these 4 services to map, track, and handle conflicts.

Anyway, this is digressing from the discussion here. I think the diagram in #11 is really good, and helps explain the plan, especially around what is in and out of core.

In Conflict 1.x there are 4 services, so zero functionality is not a fact, it's your opinion.

How and what for can we use those 4 services?

Multiversion does depend on it, yes, that is a fact. However the "for some unknown reasons to me" does sounds like mocking. As if you're saying "WTF would anyone want to depend on that stupid version of this module for".

Did I say that or it is just you putting words in my mouth? If I wanted to say that then I would've said it, but instead I very specifically said "unknown reasons". If it suits you to interpret it differently only to start a big and meaningless discussion, then this is your own decision.

I've never said those words and it is up to you to choose how to interpret them. But there was absolutely no hidden meaning.

How and what for can we use those 4 services?

I don't think this is a conversation for this issue, I'm just correcting your "fact" of no functionality.

Did I say that

No, sorry, but that is how it came across, that's how I felt. I felt mocked.

Where you said "Unfortunately I cannot speak for the 1.x branch." it was perfect, I wish you'd said this in #12.

No, sorry, but that is how it came across, that's how I felt. I felt mocked.

Why you? In which way did I offended you? If I did, then apologize and I can ensure you that it was not on purpose.

When I wrote "unknown reasons" I meant unknown to me because I thought it was clear that I cannot speak for anyone else.

Also note despite our previous conflicts I only brought up the 1.x branch because @plach explicitly asked about this, not because I wanted to cause any conflict here (no pun intended).

So I hope we can return to actually discussing the contents of this issue which is actually the reason why I came to this issue, before you two started this unnecessary conflict. As always, I look forward to your input on the actual subject and hope we can find agreement here.

After a long and productive call with @hchonov, we came up with a common interface for the different requirements against a conflict detection / auto-resolution interface in core.

Requirements to take into consideration:

• We have to be able to detect conflicts on multiple levels
  • Entity level: e.g. translation has been added.
  • Field level: field values changed concurrently
• We need to be able to apply resolution strategies based on rather arbitrary circumstances:
  • Entity type
  • Per field type
  • Per field instance
  • My current use case: based on selected filter format
• Implementation should not assume a specific workflow:
  • Autosave use case: everything happens during form submit
  • Manual merge: page callback invokes conflict resolution and allows review before actually saving
  • Force merge that prefers a certain direction
  • …
• Some implementations might affect others:
  • A blacklist might stop all automatic resolution attempts of any other resolver strategies.
  • A specific field resolution strategy might cover some conflicts, while the rest is left to the default resolution.
• Application of certain strategies might depend on arbitrary context
  • Autosave scenario takes the form configuration into account and auto-merges fields that are not visible.
  • Revision tree force-push/pull auto-accepts changes from a specific side.

Based on this we drafted a first suggestion for an event based approach. There would be one new event type in core called EntityConflictEvent:

• Conflicts: A list of conflicts:
  • Conflict type (field conflict, added translation …)
  • Additional conflict information, like field name …
  • To use them in a REST/decoupled environment these list items need to be serialisable.
• Left entity: The left part of the comparison.
• Right entity: The right part of the comparison.
• Base entity: The initial version. The lowest common ancestor in a revision tree scenario.
• Result entity: Initially a copy of the base entity, modified during event propagation.
• Context: A ParameterBag of arbitrary contexts used by specific implementations.
  • Autosave form state and configuration
  • Preferred direction in case of force push/pull

Ways listeners can act on this event:

• Just pass it through.
• Detect a conflict, and add it to the list of conflicts.
• Detect a conflict, resolve it by modifying the result entity.
• Detect a conflict, add it to the list and invoke stopPropagation to make sure nobody else auto-resolves it.

The different components that require conflict resolution would just fire the event and use the result information in a way they seem fit. The event propagation itself should just result in a list of conflicts and an automatic merge result (that may be used or not), but no persistent changes. How and when the conflicts and the merge result are used is up to the implementation.

In our drafted scenario, field level conflict resolution would not happen in Drupal core, but the conflict 2.0 module would provide a listener to this event, that itself would use a similar pattern to invoke different resolvers for all conflicted fields.

After a little more discussion we refined the approach:

The event should be split in two. The first one to detect conflicts, the second one to apply automatic resolution.
Both would contain the Left, Right and Base entities as well as the Context bag. But the first one would collect the list of conflicts, while the second one assembles the resulting entity.

Woah, exciting to see this much progress! 👏

I've posted a patch based on #27 and #28 in conflict 2.x - #3043782: Conflict discovery and conflict resolution through pub/sub.

Reviews are welcome :).

When we have something we agree on, then we could extract some parts to the core.

Implemented the first patch by porting @hchonov's work in the conflict module and adding some test cases.

Fixed test namespaces.

Here's the initial implementation issue: #3553775: Add a conflict management API