_node_revision_access is executing an unnecessary query when $op equals 'delete' or 'update'

Indeedn, It seems better that the if-statement first checks if op is delete or update. I tested the patch and all went fine. Seems good to me.

Based on experiences in #1064954: _node_revision_access() static usage I'd like to see some nice robust test coverage to go with this.

So to clarify my previous comment, it would be good to add test coverage for the particular flow branch of _node_revision_access() that the patch changes. In this, case, that's if the node is the current revision, and it is either the only revision, or the user is trying to update or delete it.

There are two non-mutually-exclusive possibilities for tests:

1. A unit(ish) test along the lines in #1064954: _node_revision_access() static usage, probably extending the new NodeRevisionPermissionsTestCase, that tests the return value of _node_access_revision() in all these cases.
2. A functional test that tries to delete the current revision of a node, etc. See node_menu() for where this function gets used as an access callback.

That test coverage looks good to me. So we just need the functional tests, then.

Couple minor cleanups:

+++ b/core/modules/node/node.testundefined
@@ -2422,6 +2422,17 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
+    // Test that user cannot delete/update current revision.

Let's make this "delete or update" so that it's a more complete sentence.

+++ b/core/modules/node/node.testundefined
@@ -2422,6 +2422,17 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
+    // Create a node with a single revision and make sure cannot view/update/delete it.

This comment is over 80 chars, so let's wrap it. I'd also say "view, update, or delete".

+++ b/core/modules/node/node.testundefined
@@ -2394,6 +2394,7 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
+    $current_account = $GLOBALS['user'];

@@ -2403,14 +2404,25 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
+    // Reset the current user back for further tests.
+    $GLOBALS['user'] = $current_account;

Let's remove this. Each test method gets its own session, so we don't need to restore the logged-in user. Also, we probably shouldn't muck with data for the test in the parent site environment.

Edit: Also, if need be, we can create a separate test method.

+++ b/core/modules/node/node.testundefined
@@ -2418,10 +2426,28 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
     // Test that the $account parameter defaults to the "logged in" user.
-    $original_user = $GLOBALS['user'];
-    $GLOBALS['user'] = $admin_account;
+    $this->drupalLogin($admin_account);
     $this->assertTrue(_node_revision_access($revision, 'view'), '_node_revision_access() returns TRUE when used with global user.');
-    $GLOBALS['user'] = $original_user;

Ah. I see now why you were using this. The purpose here was to test the function used the correct default account when the account wasn't passed (i.e. no third parameter for _node_revision_access($revision, 'view')). Running within the test case, it will actually default to the current user in the parent environment (not $admin_account), so the fact that the patch passes with this hunk is (I think) misleading. I think if we switched it to an unprivileged user and tested for FALSE instead, it would probably fail.

Since we're passing the account parameter in the assertions immediately below this hunk, I am not sure we should have the the drupalLogin() where it is.

I'd say we should open a followup for testing _node_revision_access($revision, 'view') without passing the account parameter, and maybe we should also move the functional assertions into their own method? Edit: we could even create that extra node in setUp(). What do you think?

#808730: Show the Revisions tab/page even when only one revision exists. changes the same line of code and has an extra use case.
Should we merge the two issues? Or better leave this as the 'test'-issue vs. the other 'patch'-issue
The patch-code seems to change regularly, whereas the test-code should be stable.

Novice issue cleanup.

Patch no longer applies.

Does this mean that everything needs to be redone?

Let start rerolling

OK, Teemu is taking this! He already started on it.

Sorry about forgetting to assign this to me :)

This one seems to be outdated and not relevant anymore. Feel free to reopen though if I'm mistaken.