[PP-1] Replace content revision table with a view

This is part of #1864980: [meta] Figure out how to integrate Views into core.

Also we have a diff library in core, would be nice to get a diff in a modal for the body field. Probably out of scope.

Here's a re-roll of this, still needs quite some work.

Also opened #2032977: Add views support for the missing columns of node_field_revision as a follow up of this one as we've lost some properties in views at some point.

Need to check also #1863898: Add test coverage for Views revision link handlers as the revision links seems to be being fixed there

+++ b/core/modules/node/node.moduleundefined
@@ -1351,16 +1351,6 @@ function node_menu() {
-  $items['node/%node/revisions'] = array(
-    'title' => 'Revisions',
-    'page callback' => 'node_revision_overview',
-    'page arguments' => array(1),
-    'access callback' => '_node_revision_access',
-    'access arguments' => array(1),
-    'weight' => 20,
-    'type' => MENU_LOCAL_TASK,
-    'file' => 'node.pages.inc',
-  );

I really hope that noone complains about the missing revision UI when views is not installed.

+++ b/core/modules/node/node.pages.incundefined
index 1fa8689..bf44f7f 100644
--- a/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.php

--- a/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.php
+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined

+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
@@ -32,6 +32,7 @@ class DefaultViewsTest extends ViewTestBase {

@@ -32,6 +32,7 @@ class DefaultViewsTest extends ViewTestBase {
     'backlinks' => array(1),
     'taxonomy_term' => array(1),
     'glossary' => array('all'),
+    'content_revisions' => array(1),
   );
 

We could also add node_field_revisions to HandlerAll test. In general I don't think have to add the view here as we have a good test coverage provided by node module anyway.

I really hope that noone complains about the missing revision UI when views is not installed.

Given that content is anyway pretty much unmanageable without Views now, I think that doesn't add much on top.

However, Views module shouldn't say "Create customized lists and queries from your database." as description. It should mention being needed for system views, too. Created #2037783: Make module description for Views reflect its importance to cover that aspect.

Let's test #10 to see how close we are.

Regarding the missing revision UI without Views:

Given that content is anyway pretty much unmanageable without Views now, I think that doesn't add much on top.

was a bit overstating. We should probably have some basic fallback here, too.

Mh I have to admit that sort of, because there is no other way to find the revisions of a node.
This is difficult to discuss.

Agreed, we need to provide a fallback for the node revisions.

I'm marking this "blocked" by #1863898: Add test coverage for Views revision link handlers & #2032977: Add views support for the missing columns of node_field_revision

Just unassigning issues that haven't been developed for a bit in the D8 queue.

Apparently #1863898: Add test coverage for Views revision link handlers got fixed in #2322949: Implement generic entity link view field handlers and from #1863898-87: Add test coverage for Views revision link handlers onward that was a test only patch. Turns out this has been unpostpone for last seven months.

I think starting form the scratch is a good idea here but this is belongs to 8.1.x now.

Starting from scratch, this creates a view with a 'View' button, instead of linking the revision date to view the revision.

Also, doesn't show the current revision, as this will need a plugin.

This depends upon #2630886: Correct the join from revision data table to revision base table so i included it in 1863906-revision-view-full.patch

Thanks @kim.pepper. I think we need an update hook to enable the view and an update path test to test it as well.

instead of linking the revision date to view the revision.

I think we should keep this as is for UX point of view.

This adds a Revision Log column, to keep feature parity with the existing list, and fixes a couple of test fails.

Added the upgrade path and test.

However, still getting a fatal:

Fatal error: Call to a member function isDefaultRevision() on a non-object in /vagrant/app/core/modules/node/src/Plugin/views/field/RevisionLink.php on line 30

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

Here is a new version of the patch using the same view provided in #27 plus some changes.

Since I'm also working on the #2015149: Replace dblog recent log entries with a view which also introduces a new view, the hook_update_N was replaced by a node.post_update.php file as was recommended there.

Also, I fixed one of the two errors that #27 exposes. The one related to Views.Drupal\views\Tests\DefaultViewsTest.

Added the upgrade path and test.

However, still getting a fatal:

Fatal error: Call to a member function isDefaultRevision() on a non-object in /vagrant/app/core/modules/node/src/Plugin/views/field/RevisionLink.php on line 30

Regarding this fatal, the cause is: When a revision is deleted, the views cache stills sees the deleted revision entity as a row but with a null value. You can replicate this by installing the patch, deleting a revision and trying visiting the revisions list again. You will see the same fatal error. But if you clear the cache, the fatal error vanish.

The fix to this problem could be related to cache invalidation tags, but I'm not sure if drupal core already has an example of this for in other view. As far I know, the cache for the view should be updated based on updates of the node.

Created #2841504: Cache tags for content revisions and contextual filters to track the issue with the cache tags.

So, it resulted that this is not a cache issue.

The problem here is the node_revisions view is using as base table node_field_revision which keeps older revisions even if they are deleted.

See this two issues:

#2818053: Entries in revision_data_table are not deleted when entity revisions are deleted
#2753971: ContentEntityStorageBase::deleteRevision() function does not remove node_field_revision entries

Fixing the problem on the other tickets will automatically fix the last error of this one.

#2753971: ContentEntityStorageBase::deleteRevision() function does not remove node_field_revision entries was committed to 8.4.x. Let see how the this work now.

Some extra progress. There is now a tests that enables views and run the same tests for revisions with and without views.

Nice work @dagmar all we need is a green patch now.

Rerolled above patch. Applies to 8.4.x and 8.5.x.

It's been a while, lets see where we stand with tests (patch still applies cleanly).

One question we could evaluate in the issue summary: Are we okay with the potential problem of modules like diff start to maybe have issues, as we are using a view now?

Fixing some failing tests. NodeRevisionsViewsTest still needs to be looked into.

This should fix MigrateUpgrade6Test

Regarding the last failing test, the problem is that the view in the patch requires the view all revisions permission, whereas the test tries to access the revisions page with a user with just the view page revisions permission, and thus you get a permission denied page, and the test fails.

However this is highlighting a fundamental problem with what this issue is trying to do (tests++):

Currently the node/%/revisions page requires the user to either have 'view all revisions' or 'view $type_id revisions' permission (see NodeRevisionAccessCheck::checkAccess()).

So unless we figure out how to configure the view to require a user permission that depends on what node type you're trying to access, I don't think we can do this (without getting rid of per node type view revisions permissions, which is not going to happen).

To my knowledge this is not currently possible... which is a bummer because its a blocker for this issue :(

Views access thingies are plugins, so technically, it's not very hard to create a custom access plugin that can figure that out I think. Only problem with that is that this access option (e.g. "Access revisions page for this content") will then *always* be an available option to site builders when deciding how to control access to a view.

Perhaps my comment on #2350939: Implement a generic revision UI (comment 57) could solve the problem mention in #56 and #57 in this issue.

By using a Block instead and having the routing logic handled by the Entiy Type then you can probably overcome the more complex access control issue and possibly other issues that might arrise.

Let's start with before/after screenshots and a proper reroll against 8.7-dev after some tests were renamed.

Before:

After:

I like the separate column for the log messages and that it consumes less vertical space now.
However,

1. We are now always showing the initial author of the node in the "Revision" column. Previously we showed the author of the actual revision, and that clearly made more sense.
2. There is no visual hint anymore signaling the current revision.
3. In a content moderation workflow, draft revisions used to have an operation named "Set as current revision" instead of "Revert", though the link goes to the same '/revisions/%revision/revert' path.
4. Then there is this minor placeholder problem.

Re the visual hint aspect:

• I personally always disliked the yellowish color, but it's definitely better than no visual hint. Also the phrase "Current revision" was replaced by a single "View" dropbutton, which is visually more appealing but adds to the problem.
• I initially also liked the fact that "View" is now the default operation rather than "revert." However, in an editorial setting, "Set as current revision" for drafts might be the dominant task. Additionally, the primary operations set apart drafts from current revision as well as from older revisions. We therefore might want to restore the old primary operations.
• If we have some other visual hint in place, we may keep the column redesign and the single "View" dropbutton for the current revision.

And here's the plain reroll of #54.

I tested #62 and looks like a good improvement for me! However it does not work with content translation enabled, after I add a translation I get this error:

Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("arg_0") to generate a URL for route "view.content_revisions.page_1". in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 182 of /home/d02hw/www/core/lib/Drupal/Core/Routing/UrlGenerator.php).

I think also the content revision view need to be filtered to the current language, maybe using contextual filter from URL?

• Switched author field - we need the revision author.
• Changed RevisionLinkRevert link handler to print "Revert" for past and "Set as current revision" for forward ("draft") revisions.

And here's the missing interdiff. Surprising how many tests are failing, but we'll get them tracked down.

Ah, schema.

Must have been too late last night. :/

Sorry for the noise. There we should go, finally.

Again, sorry for the noise. The view basically worked as it was properly exported, but there was something severely wrong with the YML.
Run a few local tests and hope now that we're finally back to three or four fails as in #62.
Ignore everything between #65 and #69, please.

While you never know, there should actually only be one remaining failure that is related to [#2831499].

Remaining deprecation notices (20)
The revision_user revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
The revision_created revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
The revision_log_message revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
The revision_user revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
The revision_created revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
The revision_log_message revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
...

I'm unsure why these deprecation notices are shown. Before running the version update from NodeRevisionsViewsUpdateTest, the codebase should be up to date, so I don't why the metadata keys are not set.

At least I updated the test to Drupal\FunctionalTests\Update\UpdatePathTestBase, fixed and simplified some other things.

I'm unsure why these deprecation notices are shown. Before running the version update from NodeRevisionsViewsUpdateTest, the codebase should be up to date, so I don't why the metadata keys are not set.

@Pancho, update path tests must be in the legacy group, see https://www.drupal.org/node/2985785

Thank you so much, @amateescu!
With your help this might finally, for the first time in this issue, go green...
I'm hopeful enough to set it to "Needs review."

And here are some new screenshots:

Before:

After:

• Don't know why the "Strip HTML" filter/rewrite doesn't work on the log messages. That's something we still have to solve.
• If we want to retain the yellowish rowstyle for the default revision, and the striped rowstyle in bartik, this might be solved in Twig or we could add a rowstyle plugin to views.
• For the dropbutton misalignment error, see #1890266: dropbutton text fails to retain .dropbutton-widget width.

I was trying this patch on a bilingual site and I found each revision to be displayed twice.

Thanks for testing and reporting. Next week I should find some time to research and fix this and other bugs.

Feel free to step in though...

Addressed #26.

Don't know why the "Strip HTML" filter/rewrite doesn't work on the log messages. That's something we still have to solve.

Revsion log field is \Drupal\Core\Field\Plugin\Field\FieldType\StringLongItem it doesn't allow tags so stripping them also doesn't make sense.

Updated revision view page breadcrumb.
Update page display name.

Updated IS with screenshots.

@jibran - Things look great with the new view. One thing I've noticed though is there's an encoding issue when the pagination kicks in. See Next › Last pageLast » in your screenshot. I applied the patch to try this myself and can confirm I also had the encoding issue.

Looks related to:

+++ b/core/modules/node/config/optional/views.view.content_revisions.yml
@@ -0,0 +1,791 @@
+          tags:
+            previous: '‹ Previous'
+            next: 'Next ›'
+            first: '« First'
+            last: 'Last »'

Thanks for all your work on this! Can we make the 'current revision' revert-able? If you have additional drafts ahead of the current revision, and decide you want to create a new draft based on what's current, you'd have to delete all revisions ahead of the 'current revision'. We allow both future and past revisions to be copied to latest, but we ignore current.

I've outlined an example use case here: #2350939-85: Implement a generic revision UI.

There's also another thread that covers the labeling that might be considered #2899719: Revision/version language on revision listing page is misleading with content moderation enabled.

Re-roll patch for 8.9 created.

More test fixes.

Re-rolling patch against 9.1.x-dev , kindly review a patch.

Patch from 90 applies cleanly to 8.9.3 as well. Thanks!

Re-rolling for latest dev branch and resolving failed test cases

Adding NorthAmerica2021 and Easy Out of the Box tags for visibility.
DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

I experience the same behavior as in #64. After a cache clear or two, it stops misbehaving. Going to guess something's up within the view.

Patch from #90 applies, but is malformed on 9.1.6.

Reroll from #90 for 9.1.x.

Queued patch in #101 for testing against 9.2.x, as generally speaking development on core features should be done against the latest branch.

Fixed tests for 9.1.x, Please have a look.

I think #2350939: Implement a generic revision UI makes this a wontfix.

As much as making this with a view is nice in some ways, we need a generic UI for revisions that works on all entity types. I don't see how we can do that with a View.

Also, we need the revisions table to be alterable by other modules (#2612222: Provide a better way to alter entity revision overview) such as Content Moderation (#3212602: [PP-?] Show revision state on the revisions overview tab for moderated nodes/entities).

We can have his as well as generic revision UI just like admin/content has both view and fallback. With we an always add/alter views field plugin to alter the page.

I think that's going to lead do bad UX, and exactly the opposite of this issue's 'Easy Out of the Box' tag!

Scenario:

1. user installs Drupal
2. node revisions are a view
3. user enables Content Moderation module
4. The revision status column doesn't show on the revisions page, because the view is overriding the alteration done using #2612222: Provide a better way to alter entity revision overview.
5. User needs to either:
-- a. understand the concept of 'a view at the same path overrides the route controller content' and know to delete the view, or
-- b. know how to edit the view to add the revisions status themselves

Re-rolled the patch for 9.4.

Also wondering if this is needed now that #2350939: Implement a generic revision UI landed.

While #2350939: Implement a generic revision UI is committed, it doesn't impact this issue because Node is still using the same UI, that will change in #3153559: [PP1] Switch Node revision UI to generic UI.

I would suggest that we postpone this issue on #3153559: [PP1] Switch Node revision UI to generic UI as it doesn't make sense to do anything more here until that issue is done, and then evaluate where things are at.