Apparently our test coverage doesn't extend to node revision links, they seem to be broken.
Failing test to come.

Files: 
CommentFileSizeAuthor
#69 interdiff-1863898-68.txt7.66 KBlokapujya
#69 1863898-68.patch22.12 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,846 pass(es).
[ View ]
#67 views_revision_link-1863898-67.patch21.79 KBpcambra
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,763 pass(es), 69 fail(s), and 0 exception(s).
[ View ]
#67 interdiff.txt4.35 KBpcambra
#63 views_revision_link-1863898-63.patch21.3 KBcriscom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,862 pass(es).
[ View ]
#61 1863898-61.patch21.3 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,801 pass(es).
[ View ]
#56 1863898-56.patch21.2 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1863898-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#56 interdiff.txt5.96 KBlokapujya

Comments

dawehner’s picture

StatusFileSize
new10.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1863898-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

/me sighs

Not sure whether you (other person) will find this code helpful, but maybe you do so here is it.

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new10.9 KB
FAILED: [[SimpleTest]]: [MySQL] 50,100 pass(es), 7 fail(s), and 1 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, vdc-1863898-2.patch, failed testing.

dawehner’s picture

Assigned:Unassigned» dawehner

Working on that ... what a pain.

dawehner’s picture

StatusFileSize
new11.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1863898.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is really just temporary code-tracking.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new808 bytes
new11.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,093 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-roll. Interdiff is for minor change in core.

Status:Needs review» Needs work

The last submitted patch, 1863898-6.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new9.29 KB
new16.57 KB
FAILED: [[SimpleTest]]: [MySQL] 57,571 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Updated the old view and minor nip tuck.

Status:Needs review» Needs work

The last submitted patch, 1863898-8.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new2 KB
new16.6 KB
FAILED: [[SimpleTest]]: [MySQL] 56,368 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

This will fix the test.

jibran’s picture

StatusFileSize
new2 KB

This will fix the test.

Status:Needs review» Needs work
Issue tags:-Needs tests, -VDC

The last submitted patch, 1863898-10.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

#10: 1863898-10.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +VDC

The last submitted patch, 1863898-10.patch, failed testing.

jibran’s picture

Well according to verbose messages the test is correct. I have the same 8 fails locally.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new16.44 KB
PASSED: [[SimpleTest]]: [MySQL] 56,728 pass(es).
[ View ]

Here's a re-roll of #10

I've fixed some minor things, like invoking camel cased views methods and fixing the "revisions" link as it should be in plural form, also the test view was disabled so no way to test the path.

Also removed testing the view link as the view op is always available (no particular permission for that).

jibran’s picture

Issue tags:-Needs tests
StatusFileSize
new3.98 KB
new17.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here is the reroll and some minor fixes.

jibran’s picture

StatusFileSize
new704 bytes
new17.84 KB
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 5 fail(s), and 49 exception(s).
[ View ]

:/

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-18.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new6.85 KB
new18.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,120 pass(es).
[ View ]

Here are a couple more fixes. This should also fix the RevisionLinkTest test too.

dawehner’s picture

  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
    @@ -21,29 +21,48 @@
    +  public $countRevisions;

    No need for a public property.

  2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
    @@ -21,29 +21,48 @@
    +  function preRender(&$values) {

    There should be a visibility.

  3. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php
    @@ -21,29 +21,48 @@
    +      $this->countRevisions = db_query("SELECT nid, COUNT(vid) as count FROM {node_field_revision} WHERE nid IN (:nids) GROUP BY nid", array(':nids' => array_unique($nids)))->fetchAllKeyed();

    Let's use an injected database connection.

  4. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkDelete.php
    @@ -20,18 +20,19 @@
    +    if (!($access = (user_access("delete $type revisions") || user_access('delete all revisions') || user_access('administer nodes')) && node_access('delete', $revision_node))) {

    +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkRevert.php
    @@ -20,18 +20,19 @@
    +    if (!($access = (user_access("revert $type revisions") || user_access('revert all revisions') || user_access('administer nodes')) && node_access('update', $node))) {

    We can also replace user_access with \Drupal::currentUser()->hasPermission

jibran’s picture

Assigned:dawehner» jibran
Status:Needs review» Needs work

working on it.

jibran’s picture

Assigned:jibran» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.52 KB
new20.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 24 fail(s), and 15 exception(s).
[ View ]

Fixed #21 and some clean up.

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-23.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
new20.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

Fixed tests and typo :S

dawehner’s picture

  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
    @@ -55,11 +56,21 @@ public function render(ResultRow $values) {
       protected function renderLink($node, ResultRow $values) {

    Can we also typehint on the actual code?

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/RevisionLinkTest.php
    @@ -0,0 +1,106 @@
    +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[0]->vid->value . '/delete');
    +    $this->assertNoLinkByHref($uri['path'] . '/revision/' . $this->revisions[0]->vid->value . '/revert');
    ...
    +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/view');
    +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/delete');
    +    $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/revert');
    ...
    +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[2]->vid->value . '/delete');
    +    $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[2]->vid->value . '/revert');
    ...
    +          $this->assertLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/' . $operation);
    ...
    +          $this->assertNoLinkByHref($uri['path'] . '/revisions/' . $this->revisions[1]->vid->value . '/' . $operation);

    Can't we just use getRevisionId() ?

jibran’s picture

StatusFileSize
new4.03 KB
new21.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 296 fail(s), and 962 exception(s).
[ View ]

Fixed #27.

damiankloip’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
@@ -43,6 +44,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+    $this->ensureMyTable();

Do we really need to add this call here? We probably don't really even need the query method? or empty query method?

Otherwise, this is looking pretty good.

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-27.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new1.07 KB
new21.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,944 pass(es), 140 fail(s), and 82 exception(s).
[ View ]

I have removed query method. And reverting the typehinit of renderLink to match the parent definition.

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-30.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new631 bytes
new21.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,149 pass(es), 6 fail(s), and 84 exception(s).
[ View ]

or empty query method?

Let's try that.

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-32.patch, failed testing.

damiankloip’s picture

I vote just reverting the changes to the query method and leaving it how it was. I think that will work for us?

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new514 bytes
new21.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.node-module.1863898-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reverted.

jibran’s picture

It is a quite day for VDC team anyone want to RTBC.

jibran’s picture

StatusFileSize
new507 bytes
new21.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.node-module.1863898-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reverted all the changes of query method.

Status:Needs review» Needs work

The last submitted patch, drupal8.node-module.1863898-37.patch, failed testing.

damiankloip’s picture

Sorry, let's go with the patch in #35! I guess we do need to ensure the table..

damiankloip’s picture

Status:Needs work» Reviewed & tested by the community

rtbc for #35.

catch’s picture

Status:Reviewed & tested by the community» Needs review
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLinkRevert.php
@@ -20,18 +20,19 @@
+    if (!($access = ($this->currentUser->hasPermission("revert $type revisions") || $this->currentUser->hasPermission('revert all revisions') || $this->currentUser->hasPermission('administer nodes')) && node_access('update', $node))) {

Why's the access check being taken out of access() and put into the rendering?

damiankloip’s picture

Because access() is only called when handlers are initialised. If it failed here the handler would be removed from the view altogether. This needs to use the current node to determine the access, as well as user permissions.

catch’s picture

Won't preRender() run before renderLink() though? If so we're counting all the revisions for all the nodes when generating the delete/revert links, even if the user never has a chance of seeing them.

Also it might be too early here, but I can't see the access check being added back for the view link?

-  public function access() {
-    return user_access('view revisions') || user_access('administer nodes');
+  /**

+  /**
+   * {@inheritdoc}
+   */
   protected function renderLink($data, ResultRow $values) {
-    list($node, $vid) = $this->get_revision_entity($values, 'view');
-    if (!isset($vid)) {
-      return;
-    }
+    list($node, , $vid) = $this->getRevisionEntity($values, 'view');

     // Current revision uses the node view path.
-    $path = 'node/' . $node->nid;
-    if (!$node->isDefaultRevision()) {
+    $uri = $node->uri();
+    $path = $uri['path'];
+    if ($this->countRevisions[$node->id()] > 1 && $node->getRevisionId() != $vid) {
       $path .= "/revisions/$vid/view";
     }

damiankloip’s picture

I spoke to catch about this, we need to add back access. We have two levels here, whether we can view the node or not and whether we have a revision to access etc.. We need to check user_access still, as probably node_access() to determine if we even show a view link. So I think the current logic to revision linking is ok, we just need to add back the access stuff in some form. However, probably not in the access() method like before?

jibran’s picture

Status:Needs review» Needs work

The last submitted patch, 35: drupal8.node-module.1863898-35.patch, failed testing.

xjm’s picture

Component:node.module» node system
Issue summary:View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)

xjm’s picture

Priority:Normal» Major
Issue tags:+beta target

This seems like a major regression to me, and it's also blocking views conversions.

kim.pepper’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 37: drupal8.node-module.1863898-37.patch, failed testing.

kim.pepper’s picture

Issue tags:+Needs reroll
lokapujya’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new21.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/node/src/Plugin/views/field/RevisionLink.php.
[ View ]

Re-roll. I ran out of time to figure out what to do about access(). Had to change node_access to node.access(). But, I think tests are going to fail there.

Status:Needs review» Needs work

The last submitted patch, 52: 1863898-52.patch, failed testing.

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new21.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,565 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Fixed a double use statement.

Status:Needs review» Needs work

The last submitted patch, 54: 1863898-54.patch, failed testing.

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new5.96 KB
new21.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1863898-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Get the re-roll up to date with Core changes.

Jalandhar’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Patch needs reroll.

mgifford’s picture

Status:Needs work» Needs review

mgifford queued 56: 1863898-56.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 56: 1863898-56.patch, failed testing.

lokapujya’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new21.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,801 pass(es).
[ View ]

Rerolled.

criscom’s picture

Issue tags:+Amsterdam2014

Testing

criscom’s picture

StatusFileSize
new21.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,862 pass(es).
[ View ]

Rerolled and patched successfully with no conflicts/errors.

jhedstrom’s picture

From #52

I ran out of time to figure out what to do about access().

Has the link access issue been resolved in the subsequent rerolls (it's hard to tell without interdiffs)? That comment is the only mention of catch's concern from #43.

lokapujya’s picture

access() is only called on handler initialization, so the access check was moved to renderLink(). My understanding is that we need a better place for the access check, so that preRender doesn't get called?

lokapujya’s picture

There must be similar access checks somewhere that give a clue how this should be done.

pcambra’s picture

Issue tags:-Amsterdam2014+Needs issue summary update
StatusFileSize
new4.35 KB
new21.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,763 pass(es), 69 fail(s), and 0 exception(s).
[ View ]

Just making a couple of nitpick changes to get the ball rolling on this.

Regarding the access discussion, last comment from @damiankloip is #1863898-44: Views revision link handlers are broken which suggests that we need to get the access check back, it appears that there are two kind of checks here:

  1. Generic user permissions, i.e. $this->currentUser->hasPermission('delete all revisions')
  2. Specific check for the user in the context, i.e. $revision_node->access('delete')

Could we add 1) back to access() method and keep 2) in the renderLink() method?
I understand this is an issue just for the Revert and Delete revision links.

Status:Needs review» Needs work

The last submitted patch, 67: views_revision_link-1863898-67.patch, failed testing.

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new22.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,846 pass(es).
[ View ]
new7.66 KB

I need to re-review this myself to make sure it's still relevant, but here is a re-roll.