In the View for the My Edits tab, the title link is set to " Link this field to its content revision." This setting turns the link into the format:

node/NID/revision/VID

But if there is only one revision on the node, this page returns Access Denied due to the logic in _node_revision_access():

<?php
   
if ($is_current_revision && (db_query('SELECT COUNT(vid) FROM {node_revision} WHERE nid = :nid', array(':nid' => $node->nid))->fetchField() == 1 || $op == 'update' || $op == 'delete')) {
?>

IMO, this is a Views bug, but we should simply work around it and link to the proper Node view page, as expected.

Why don't we just use the non-revisioned titles here?

Files: 
CommentFileSizeAuthor
#21 views_current_vid_link-1862014-d8.patch2.29 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 55,756 pass(es).
[ View ]
#20 views_current_vid_link.patch2.19 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views_current_vid_link.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 vdc-1862014-14.patch917 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,293 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 views-1862014-9.patch860 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]
#8 views-1862014-8.patch866 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 1,603 pass(es), 0 fail(s), and 24 exception(s).
[ View ]
#1 1862014-my-edits.patch34.15 KBagentrickard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862014-my-edits.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

agentrickard’s picture

Status:Active» Needs review
StatusFileSize
new34.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1862014-my-edits.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And a patch.

Status:Needs review» Needs work

The last submitted patch, 1862014-my-edits.patch, failed testing.

bbinkovitz’s picture

Ken, can you reroll this patch to not change the name of the view, as that may break other things? Or would you like me to try doing this?

agentrickard’s picture

Hm. I thought it was safer to change the view to a new file. If you can re-roll that way, please do.

I'd also like to get @stevector's thoughts about why this View uses the node_revision base table.

tim.plunkett’s picture

This looks like it might be a bug in Views in D7 and D8?

Compare http://drupalcode.org/project/views.git/blob/refs/heads/7.x-3.x:/modules... and http://drupalcode.org/project/views.git/blob/refs/heads/7.x-3.x:/modules...

Not sure why the first handler even exists? I must be missing something.

tim.plunkett’s picture

Title:My eidts links go to revisions but links may fail» My edits links go to revisions but links may fail

Also, rerolling the patch so the diff means something would be great.

tim.plunkett’s picture

Title:My edits links go to revisions but links may fail» Revision handler makes assumptions about path
Project:Workbench» Views
Version:7.x-1.x-dev» 7.x-3.x-dev
Assigned:Unassigned» dawehner

I think http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/node/li... might have the same problem, checking with the master :)

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new866 bytes
FAILED: [[SimpleTest]]: [MySQL] 1,603 pass(es), 0 fail(s), and 24 exception(s).
[ View ]

Well, let's try this.

tim.plunkett’s picture

StatusFileSize
new860 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Whoops, I meant this

agentrickard’s picture

That seems to fix the issue.

Status:Needs review» Needs work

The last submitted patch, views-1862014-9.patch, failed testing.

agentrickard’s picture

Status:Needs work» Reviewed & tested by the community
dawehner’s picture

Project:Views» Drupal core
Version:7.x-3.x-dev» 8.x-dev
Component:Code» node system
Issue tags:+Needs tests

Awesome, let's port that to d8.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review
Issue tags:+VDC
StatusFileSize
new917 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,293 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Still needs tests.

agentrickard’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Needs work

Sorry. This doesn't actually work. The logic is too simple:

<?php
if ($nid != $vid)
?>

This is an insufficient check, because if you create node 1, it works. Then revision node 1 to have vid 2.

Then create node 2, it gets vid 3 and the link breaks again.

The way core works this in D7 is to literally check the COUNT of rows in the {node_revision} table. Without patching core, I'm not sure we have any choice but to do that.

stevector’s picture

Assigned:dawehner» Unassigned

Yeah, this logic is not sufficient. The real question is "is this vid which is coming from {node_revision} also in {node}"

I've got a patch in the State Machine queue that is grappling with the same issue:

#1775540: Additional fields joins break with the latest version of Views

That handler is asking "is this vid which is coming from {node_revision} also in {node} and does it have a status of 1" The patch includes a test, which is now passing thanks to ugly additional queries that directly check the node table. This is needed there because these broke with a recent Views update.

<?php
  $this
->additional_fields['node_vid'] = array('table' => 'node', 'field' => 'vid');
 
$this->additional_fields['node_status'] = array('table' => 'node', 'field' => 'status');
?>

Tim or dawehner, do know what could be done in the handler or elsewhere to get this additional field piece working again? If so, that could be the fix here as well as in the State Machine patch.

agentrickard’s picture

@stevector

Even that condition can cause a break in expected functionality. The vid may be in both tables and the link would still take you to an access denied, because the access check is for COUNT of rows in {node_revision}.

stevector’s picture

agentrickard, I think I wasn't clear in my last comment. If the vid for the given result is in both tables, then the link should go to node/1. If the vid for this result is not in both tables then there must be more than one vid (or something else has gone seriously wrong) and this vid is not the one in the node table so the link should go to node/%/revision/%/view.

agentrickard’s picture

Yes.

quicksketch’s picture

StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views_current_vid_link.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a D7 patch for Views. I'll work on porting to D8.

quicksketch’s picture

Title:Revision handler makes assumptions about path» Node revision Views handler links to access denied page when only one revision exists
Version:7.x-dev» 8.x-dev
Component:node system» node.module
Status:Needs work» Needs review
StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,756 pass(es).
[ View ]

D8 port. I have to say: the D8 version of Views is refreshingly similar to the D7 module.

danmuzyka’s picture

Assigned:Unassigned» danmuzyka

Assigning.

danmuzyka’s picture

Assigned:danmuzyka» Unassigned

Ran out of time during code sprint. Assigning back to anonymous.

dawehner’s picture

Status:Needs review» Needs work

I don't see how this can cover all the cases, see issue summary.

dawehner’s picture

Issue summary:View changes

Clarifies.

xjm’s picture

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

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