Quick Edit integration

Adding that tab is certainly easier than rendering a different revision for a different user, at least for now.

Afaik its possible on every page which have proper rendered fields, so its there automatically.

Wait, did you just said that quickedit is a problematic concept?

Well, maybe you just made clear that quickedit is a problematic concept for sites with an editorial workflow. Well, I'm sooo sad about that.

The root cause of the problem is not Quick Edit. It is our extremely ill-defined model of revisions.

Quick Edit does only one thing: it allows you to in-place edit the visible fields of an entity. Period. (The visible fields are the non-empty fields.)

The values for fields are determined by which revision of an entity is loaded. And this is where we enter the painful realm wild west of revisions in Drupal (8 or otherwise). The revision that is displayed is indeed the "default" revision. Which revision is the default does not depend on anything: not on time, nor the branch (we don't even have the concept of different branches of an entity), nor the current user, nor anything else.

Now, this is already quite bad. But to make it worse, the handling of does this entity type want a new revision to be created for every edit or not is not implemented in a generic way. In fact, it only exists for Node. So, Quick Edit also had to special-case this:

  protected function init(FormStateInterface $form_state, EntityInterface $entity, $field_name) {
    …
    if ($entity->getEntityTypeId() == 'node') {
      $node_type = $this->nodeTypeStorage->load($entity->bundle());
      $entity->setNewRevision($node_type->isNewRevision());
      $entity->revision_log = NULL;
    }
    …
  }

  protected function buildEntity(array $form, FormStateInterface $form_state) {
    …
    if ($entity->getEntityTypeId() == 'node' && $entity->isNewRevision() && $entity->revision_log->isEmpty()) {
      $entity->revision_log = t('Updated the %field-name field through in-place editing.', array('%field-name' => $entity->get($field_name)->getFieldDefinition()->getLabel()));
    }
    …
  }

And as you can tell, a new revision is created only if NodeType::isNewRevision() returns TRUE, which is the (misnamed) method that Gets whether a new revision should be created by default..

Conclusion: for all entity types except Node, and for Node also if the bundle (NodeType) defaults to "no new revision by default", Quick Edit will just edit the revision you see. Otherwise, it will create a new revision.

The above means that as soon as you have a complex workflow, you need to make sure that you either:

1. disable in-place editing completely
2. enhance the revision system to have more metadata, so that you for example know which in-place edits took place while another revision was being moderated, so that those changes can be suggested automatically to be incorporated also. And probably much more.
3. let Workbench Moderation render the appropriate entity depending on the current user's roles: if the current user is not allowed to moderate, and entity X has revisions in moderation, disable in-place editing for that user for entity X; if the current user is allowed to moderate, render the to-be-moderated revision instead, and add a Currently live tab to allow the moderator to see what is live currently — but also disable in-place editing there.
4. or some variation on the previous point

Use hook_entity_view_alter() to disable in-place editing for a specific entity by undoing what quickedit_entity_view_alter() did (i.e. unset $build['#attributes']['data-quickedit-entity-id']), and add cacheability metadata if necessary to $build.

Use hook_entity_build_defaults_alter() to change $entity to represent a different revision than the default and again add cacheability metadata if necessary to $build. This one needs double-checking though, not 100% certain about it.

For the time being, we've discussed just disabling Quick Edit entirely for any Moderation-enabled bundle. It sounds from #14 like the best way to do that is via hook_entity_view_alter(), so let's give that a shot and see if that works. Just from a user-expectation standpoint, I think mixing Quick Edit and forward revisions is asking for trouble.

So yeah what about doing that until we find a better solution for it?

Just from a user-expectation standpoint, I think mixing Quick Edit and forward revisions is asking for trouble.

I don't agree with this. However, I do think that a lot of UX work is necessary to make it clear and easy to understand. And I think that solving that is identical to (or at least largely overlapping with, if not a subset of) making Workbench Moderation easy to understand.

I suspect you're working on Workbench Moderation MVP, which means the absolute minimum of making it easy to understand (i.e. across a site, for every piece of content that is being moderated or could be moderated, alther what is shown to make that crystal clear). From that POV, I agree.

For the time being, we've discussed just disabling Quick Edit entirely for any Moderation-enabled bundle.

A less heavy handed approach would be to disable quick edit for any entity that has a forward revision. Not sure how feasible that is.

Should be totally doable.

Fair enough ... When an entity has a forward revision, how about changing the pencil to a red pencil (or whatever). The title text would say why quick edit is unavailable. If a person clicks on the red pencil, you can go to the edit form for the forward revision or pop a modal with explanatory text and offer more possible destinations.

So what about something like this.

Yep, something like that :)

Drupal surgery!

No, module_implements_alter() is just a sign you want to modify the modification another module is making.

This is the biggest weakness of the hook system: it's designed to allow modules to alter things. When >1 module wants to alter the same thing, you end up in this situation: module hook invocation order matters.

If all hook implementations merely add things, there is no problem.

This is how Drupal's hook system is designed to work.

This is the biggest weakness of the hook system: it's designed to allow modules to alter things. When >1 module wants to alter the same thing, you end up in this situation: module hook invocation order matters.

Well, there is no difference from hooks vs. events here. Its more about the general idea of mutability.

On the other hand, the event system is a bit better here. This module_implements_alter() code is basically the same as changing a weight in events, just crazy odd syntax.

basically the same as changing a weight in events, just crazy odd syntax.

Indeed.

Yeah, I just love your

You should not c&p anything.

You may also want to fix #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be, it's very closely related.

There we go.

+++ b/workbench_moderation.module
@@ -41,6 +42,24 @@ function workbench_moderation_entity_base_field_info(EntityTypeInterface $entity
+  /** @var \Drupal\workbench_moderation\InlineEditingDisabler $inline_editing_disabler */
+  $inline_editing_disabler = \Drupal::service('workbench_moderation.inline_editing_disabler');

+++ b/workbench_moderation.services.yml
@@ -18,6 +18,9 @@ services:
+  workbench_moderation.inline_editing_disabler:
+    class: \Drupal\workbench_moderation\InlineEditingDisabler

It's in-place editing, not
>inline editing.