Comments

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Active » Needs review
FileSize
25.98 KB
FAILED: [[SimpleTest]]: [MySQL] 49,292 pass(es), 4 fail(s), and 1 exception(s). View
10.34 KB
24.57 KB
FAILED: [[SimpleTest]]: [MySQL] 49,265 pass(es), 4 fail(s), and 91 exception(s). View
tim.plunkett’s picture

Issue tags: +VDC

Status: Needs review » Needs work

The last submitted patch, vdc-1863906-1-WITH-A-HACK.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
26.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,324 pass(es). View

Status: Needs review » Needs work

The last submitted patch, vdc-1863906-4-HACK.patch, failed testing.

xjm’s picture

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
12.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1863906-8.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled for dropbutton, still hacky and blocked.

larowlan’s picture

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.

pcambra’s picture

Assigned: Unassigned » pcambra
FileSize
23.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1863906-replace_content_revisions-10.patch. Unable to apply patch. See the log in the details link for more information. View

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.

pcambra’s picture

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

dawehner’s picture

+++ 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.

Pancho’s picture

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.

Pancho’s picture

Status: Needs work » Needs review

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.

Status: Needs review » Needs work

The last submitted patch, 1863906-replace_content_revisions-10.patch, failed testing.

dawehner’s picture

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.

pcambra’s picture

Status: Needs work » Postponed

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

xjm’s picture

Issue summary: View changes
Issue tags: +beta target
xjm’s picture

Priority: Normal » Major
mgifford’s picture

Assigned: pcambra » Unassigned

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

mgifford’s picture

jibran’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Postponed » Active

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.

kim.pepper’s picture

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

Screenshot of views revisions table

The last submitted patch, 23: 1863906-revision-view-full.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: 1863906-revision-view-only.patch, failed testing.

jibran’s picture

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.

kim.pepper’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
30.33 KB
5.17 KB
54.86 KB

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

Content revision table with logs

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

Status: Needs review » Needs work

The last submitted patch, 27: 1863906-revision-view-27.patch, failed testing.

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.

xjm’s picture

Issue tags: -beta target

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.

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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
25 KB

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.

dagmar’s picture

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

Status: Needs review » Needs work

The last submitted patch, 32: 1863906-revision-view-61.patch, failed testing.

dagmar’s picture

Status: Needs work » Postponed

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.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Status: Postponed » Needs review
FileSize
24.94 KB

Status: Needs review » Needs work

The last submitted patch, 37: 1863906-revision-view-37.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
25.62 KB
1.6 KB

Status: Needs review » Needs work

The last submitted patch, 39: 1863906-revision-view-39.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
26.34 KB
1.93 KB

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

Status: Needs review » Needs work

The last submitted patch, 41: 1863906-revision-view-41.patch, failed testing.