Problem/Motivation

At the moment there is no generic revision UI, this means that every module with revisionable entities will need to create their own UI similar to the Node revision overview page (node/{node_id}/revisions. This means quite a bit of boilerplate code, especially for modules with multiply revisionable entities.

Proposed resolution

Create a BaseRevisionOverviewController for deleting a revision or reverting to a revision.

Remaining tasks

  • Decide if we want to improve this in core
  • Create a plan
  • Implement the plan
Files: 
CommentFileSizeAuthor
#22 interdiff-implement_a_generic-2350939-20-22.txt3.03 KBedurenye
#22 implement_a_generic-2350939-22.patch21.52 KBedurenye
#20 implement_a_generic-2350939-20.patch21.41 KBedurenye
#16 interdiff-implement_a_generic-2350939-13-16.txt4.96 KBedurenye
#16 implement_a_generic-2350939-16.patch18.95 KBedurenye
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch implement_a_generic-2350939-16.patch. Unable to apply patch. See the log in the details link for more information. View
#13 implement_a_generic-2350939-13.patch18.28 KBedurenye
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,207 pass(es), 41 fail(s), and 1 exception(s). View
#8 generic-revision-trait-2350939.8.patch18.23 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,701 pass(es). View
#8 interdiff.txt5.41 KBlarowlan
#6 generic-revision-trait-2350939.1.patch17.33 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Comments

larowlan’s picture

Yes, we have no revision ui for block content, even though it supports revisions

Bojhan’s picture

Wait, but do we need to expose a UI for everything? Or is this just a 'in case you want it' kinda thing.

larowlan’s picture

I don't think block content revision ui belongs in core. But a reusable base class I could add on contrib sounds efficient.

jbekker’s picture

It should be usefull for custom entities as well, not just block content. I dont think we would need to expose a UI by default but a base class like larowlan says would be very helpful.
I'm looking forward to see some more opinions.

Xano’s picture

I agree with what has been said before. Having a generic base class that will only be used for node revisions by default offers developers a good starting point for making their own revision UI when they want to.

larowlan’s picture

Status: Active » Needs review
FileSize
17.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Here's a start

Status: Needs review » Needs work

The last submitted patch, 6: generic-revision-trait-2350939.1.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
18.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,701 pass(es). View
larowlan’s picture

Ok, green - any reviewers?

Xano’s picture

Status: Needs review » Needs work

Thanks for bringing this back up! I had already forgotten about it.

In general I'm not sure we should make this a route controller, and considering the complexity and the fact the trait calls entityManager() which is not defined anywhere in the trait (and I know why), making this a trait sounds messy as well. Can we make this an entity handler base class? The code in its current form isn't usable for non-entities anyway. Besides, using a class rather than a trait for something this complex will allow us to build slightly more solid code (no dark magic entityManager() for instance).

In any case, here is a short review of the patch:

  1. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +  public function revisionShow($revision_id) {
    +    $entity = $this->entityManager()->getStorage($this->getRevisionEntityTypeId())->loadRevision($revision_id);
    

    We generally start methods with a verb.

  2. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +    if (isset($page[$this->getRevisionEntityTypeId() . 's'][$entity->id()]['#cache'])) {
    

    unset() works fine if the value to unset does not exist yet, so we don't need a condition here.

  3. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +   * Page title callback for an entity revision.
    

    One-line summaries should also start with a verb.

  4. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +  /**
    +   * Determines if the user has permission to revert revisions.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *   The entity to check revert access for.
    +   *
    +   * @return bool
    +   *   TRUE if the user has revert access.
    +   */
    +  abstract protected function hasRevertRevisionPermission(EntityInterface $entity);
    +
    +  /**
    +   * Determines if the user has permission to delete revisions.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *   The entity to check delete revision access for.
    +   *
    +   * @return bool
    +   *   TRUE if the user has delete revision access.
    +   */
    +  abstract protected function hasDeleteRevisionPermission(EntityInterface $entity);
    

    Did you check if this can be done through access controllers? I understand it's probably difficult at the least; I just want to make sure that approach has been researched.

  5. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +  abstract protected function buildRevertRevisionLink(EntityInterface $entity, $revision_id);
    +
    +  /**
    +   * Builds a link to delete an entity revision.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *   The entity to build a delete revision link for.
    +   * @param int $revision_id
    +   *   The revision ID of the delete link.
    +   *
    +   * @return array
    +   *   A link render array
    +   */
    +  abstract protected function buildDeleteRevisionLink(EntityInterface $entity, $revision_id);
    

    The same goes for these link builders and entities' list builder classes.

  6. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,212 @@
    +  abstract protected function getRevisionDetails(EntityInterface $revision, $is_current = FALSE);
    

    Maybe rename this to getRevisionDescription(), or something similar that indicates the output is human-readable?

  7. +++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
    @@ -0,0 +1,33 @@
    + * Defines an interface for entities with timestamped revisions.
    

    This interface is not in an entity namespace, so we should not mention entities anywhere in the file. There is no technical reason for this interface to be entity-specific in its current form, except for my next comment.

+++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
@@ -0,0 +1,33 @@
+   * @return \Drupal\Core\Entity\EntityInterface
+   *   The called entity.

Should be @return $this.

miro_dietiker’s picture

We tried to start something similar in context of Diff contrib:
#2452523: [Meta] Offer a revisions tab for all entities

We would be very happy to see this happen in core.
Note our discussions that the revision summary is a problem and there should be a handler that allows alteration in contrib.
A problem is here that i see you referring to $revision->revision_log that is only present in node revision entities from Node.php baseFieldDefinitions().
All other entities will provide an empty description.
I'm confused the patch above only implements the whole functionality on Node only? I thought the issue is about displaying a revision tab for every entity type.

Berdir’s picture

The issue is essentially just about making the code generic.

But yes, without using it somewhere else, at least in a test module (we could try to integrate it automatically in entity_test for all revisionable entity types there?), we won't see stuff like the revision_log and uid that are node specific...

edurenye’s picture

Status: Needs work » Needs review
FileSize
18.28 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,207 pass(es), 41 fail(s), and 1 exception(s). View

Rebased.
Did all changes exept the 3, that I didn't know what to put there, was like this before, and can't get a verb for that.
And 4, 5, that I don't know how to do that.
Also in RevisionControllerTrait, I'm not sure if everything it's fine there.

Status: Needs review » Needs work

The last submitted patch, 13: implement_a_generic-2350939-13.patch, failed testing.

The last submitted patch, 13: implement_a_generic-2350939-13.patch, failed testing.

edurenye’s picture

Status: Needs work » Needs review
FileSize
18.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch implement_a_generic-2350939-16.patch. Unable to apply patch. See the log in the details link for more information. View
4.96 KB

Fixed the failing test, basically improved the reroll, added changes that were made in this time.

Status: Needs review » Needs work

The last submitted patch, 16: implement_a_generic-2350939-16.patch, failed testing.

mkalkbrenner’s picture

I like this approach. But it has to be adjusted to the latest changes introduced by #2465907: Node revision UI reverts multiple languages when only one language should be reverted.

+++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
@@ -0,0 +1,218 @@
+        $links = [];
+        if ($revert_permission) {
+          $links['revert'] = $this->buildRevertRevisionLink($entity, $vid);
+        }
+        if ($delete_permission) {
+          $links['delete'] = $this->buildDeleteRevisionLink($entity, $vid);
+        }

In order to increase the re-usage of revisionOverview() the list of links should be moved to a separate method with the trait.

edurenye’s picture

Status: Needs work » Needs review
FileSize
21.41 KB

I did the rebase and moved the links to another method.

mkalkbrenner’s picture

Status: Needs review » Needs work

I think we should go one step further. The operation link targets NodeRevisionDeleteForm, NodeRevisionRevertForm,
NodeRevisionRevertTranslationForm seem to be easily adjustable for different entities as well. We should therefor provide base classes or traits, too.

Just some nitpicks:

  1. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,249 @@
    +        '%date' => format_date($entity->getRevisionCreationTime())
    

    new code should use \Drupal\Core\Datetime\DateFormatter::format().

  2. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
    @@ -0,0 +1,249 @@
    +  protected function getLinks(EntityInterface $entity, $revision_id) {
    

    I suggest to name this method getOperations() or getOperationLinks().

  3. +++ b/core/modules/node/src/Controller/NodeController.php
    @@ -283,4 +190,112 @@ public function addPageTitle(NodeTypeInterface $node_type) {
    +              'url' => $has_translations ?
    +                Url::fromRoute('node.revision_revert_translation_confirm', ['node' => $entity->id(), 'node_revision' => $revision_id, 'langcode' => $langcode]) :
    +                Url::fromRoute('node.revision_revert_confirm', ['node' => $entity->id(), 'node_revision' => $revision_id]),
    

    leading whitespaces

edurenye’s picture

What you mean exactly? Like having a TranslatableRevisionControllerTrait that extends from RevisionControllerTrait for those entities that are translatable and add the methods there or just extract buildRevertRevisionTranslationLink from buildRevertRevisionLink in RevisionControllerTrait?

Those small fixes are solved in this patch.

mkalkbrenner’s picture

@edurenye:
The goal of this issue is to isolate the re-usable parts of node revision handling and make them available for all entities. But the current patch only targets the NodeController that lists node revisions. But reverting a revision is actually done by NodeRevisionRevertForm and NodeRevisionRevertTranslationForm.
If you want to implement a UI for any entity revisions, you need to implement their logic again. Therefor I suggest to create corresponding traits or something like a EntityRevisionRevertBaseForm.

edurenye’s picture

Status: Needs review » Postponed

We discussed about this topic in the #DrupalCampVienna2015, and we agreed to implement the generic revision UI in entity API and then move it to Core.

Here is the issue I created for that: #2625122: [Meta] Implement a generic revision UI

cilefen’s picture

Version: 8.0.x-dev » 8.1.x-dev

This would land in 8.1 at the earliest.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Where, if at all, does this fit into the Worfklow Initiative? Does the content_moderation approach over in #2725533: Add experimental content_moderation module supersede this?

larowlan’s picture

Issue tags: +Workflow Initiative

Let's see what they say :)

dawehner’s picture

One part, for example the generic revert controller could be totally brought in already into core.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Adding the related Diff module issue.