view/revert/delete revisions per content type

This makes a lot of sense to me. If we all think this is a good idea, we'll certainly need an upgrade path.

This feature might have to wait until Drupal 8 though.

Looks good, and could be useful flexibility. I'd RTBC this apart from one minor niggle: When creating the user in node.test, it should enable perms just for "page", not for all types. You can then just use a static array of perms, too.

Looks good to me - RTBC conditional on passing the testbot.

Feature. freeze. Which of these two words we can't understand? We make exceptions for bugs / regressions / performance problems. This is neither. This is a genuine new feature.

Set back to 7.x, considering Dries original comment.

No. Dries said, if we all think. I certainly don't. We are not adding features.

I'm not sure if Dries meant that "everyone" = chx, but I think this warrants getting into D7 since it borders on a bug considering all the fieldable entities now and revisions is not behaving like other CRUD functions.

+1 from me.

Agreed with chx, feature requests go in Drupal 8. Also there's nothing in here which couldn't be implemented in contrib if people want it for D7.

There. Decided.

It's possible this might be one of those changes we could backport from D8, once D7 it out the door, but in the meantime we really need to not expend energy on feature patches 12 months after feature freeze.

#2: per-content-type-revision-controlsd7.patch queued for re-testing.

Just testing if the patch is ignored because the version is set to 8.x

and back. The answer is yes.

+++ b/modules/node/node.install
@@ -1,3 +1,4 @@
+

extra whitespace

+++ b/modules/node/node.install
@@ -423,3 +424,33 @@ function node_install() {
+      db_delete('role_permission')
+      ->condition('rid', $rid)
+      ->condition('permission', $action . ' revisions')
+      ->condition('module', 'node')
+      ->execute();

See comments below, but if we retain 'view content revisions' as 'view any content revisions' - this will need updating.

+++ b/modules/node/node.module
@@ -1529,15 +1529,6 @@ function node_permission() {
-    'view revisions' => array(
-      'title' => t('View content revisions'),
-    ),
-    'revert revisions' => array(
-      'title' => t('Revert content revisions'),
-    ),
-    'delete revisions' => array(
-      'title' => t('Delete content revisions'),
-    ),

Could we retain these but rename them to 'Revert any content revisions' etc. That will simplify permission administration for sites with a large number of content types.
Whilst we're at it should we also introduce 'View own revisions' etc too?

+++ b/modules/node/node.module
@@ -2972,6 +2964,15 @@ function node_list_permissions($type) {
+    "view $type revisions" => array(
+      'title' => t('View %type_name revisions', array('%type_name' => $info->name)),
+    ),
+    "revert $type revisions" => array(
+      'title' => t('Revert %type_name revisions', array('%type_name' => $info->name)),
+    ),
+    "delete $type revisions" => array(
+      'title' => t('Delete %type_name revisions', array('%type_name' => $info->name)),
+    ),

These should follow the format of the existing ones - ie %type_name: View revisions - and so on - it makes the ui at admin/people/permissions much cleaner.

+++ b/modules/node/node.pages.inc
@@ -505,12 +505,13 @@ function node_revision_overview($node) {
+  if ((user_access('revert ' . $type . ' revisions') || user_access('administer nodes')) && node_access('update', $node)) {

If we retain the existing permission as 'Revert any content revisions' this will need updating.

+++ b/modules/node/node.pages.inc
@@ -505,12 +505,13 @@ function node_revision_overview($node) {
+  if ((user_access('delete ' . $type . ' revisions') || user_access('administer nodes')) && node_access('delete', $node)) {

ditto

+++ b/modules/node/node.test
@@ -145,8 +145,9 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+    $setperms = array('view page revisions', 'revert page revisions', 'delete page revisions', 'edit any page content', 'delete any page content');

being picky but I don't like the variable name here.

I think we need another test here too - to test that the user without the appropriate permission gets a 404.

Powered by Dreditor.

+++ b/modules/rdf/rdf.testundefined
@@ -120,7 +120,7 @@ class RdfRdfaMarkupTestCase extends DrupalWebTestCase {
-    $admin_user = $this->drupalCreateUser(array('edit own article content', 'revert revisions', 'administer content types'));
+li    $admin_user = $this->drupalCreateUser(array('edit own article content', 'revert article revisions', 'administer content types'));
     $this->drupalLogin($admin_user);
 
     $langcode = LANGUAGE_NONE;

Typo

0 days to next Drupal core point release.

t($role->name); - we probably can't use t() here. t() can hit the theme system and end up invoking hooks. But the translated string is probably never going to be necessary so I reckon that could be skipped.

-  if ((user_access('revert revisions') || user_access('administer nodes')) && node_access('update', $node)) {
+  if ((user_access('revert ' . $type . ' revisions') || user_access('revert any revisions') || user_access('administer nodes')) && node_access('update', $node)) {
     $revert_permission = TRUE;
   }
   $delete_permission = FALSE;
-  if ((user_access('delete revisions') || user_access('administer nodes')) && node_access('delete', $node)) {

This is getting very verbose - could we maybe add a _node_revision_access($operation); helper?

Everything else looks fine.

Is it worth adding tests that confirm the permissions granularity works?

Sorry one more thing. The {roles} table changed in Drupal 7, so this needs to be _update_7007_user_roles() - since that's where the column was added.

Related #282404: Add view [TYPE] content permission

sub

+++ b/modules/node/node.installundefined
@@ -469,3 +469,69 @@ function _update_7000_node_get_types() {
+function _update_7000_user_roles($membersonly = FALSE, $permission = NULL) {

Should be called _update_8000_user_roles() to match name of calling function.

+++ b/modules/node/node.installundefined
@@ -469,3 +469,69 @@ function _update_7000_node_get_types() {
+  $query = db_select('role', 'r');
+  $query->addTag('translatable');

This can be chained.

+++ b/modules/node/node.installundefined
@@ -469,3 +469,69 @@ function _update_7000_node_get_types() {
+        ->fields(array(
+          'rid' => $rid,
+          'permission' => $action . ' ' . $type . ' revisions',
+          'module' => 'node'

For readability I'd define the array outside of the fields call.

The rest looks good to me.

-22 days to next Drupal core point release.

+++ b/modules/node/node.install
@@ -469,3 +469,98 @@ function _update_7000_node_get_types() {
+  $node_types = db_query('SELECT * FROM {node_type}')->fetchAllAssoc('type', PDO::FETCH_OBJ);

Chained item should probably on the next line and indented.

+++ b/modules/node/node.install
@@ -469,3 +469,98 @@ function _update_7000_node_get_types() {
+  $query = db_select('role', 'r')
+  ->addTag('translatable')
+  ->fields('r', array('rid', 'name'))
+  ->orderBy('weight')
+  ->orderBy('name');
...
+        db_insert('role_permission')
+        ->fields($fields)
+        ->execute();
+      }
+      db_delete('role_permission')
+      ->condition('rid', $rid)
+      ->condition('permission', $action . ' revisions')
+      ->condition('module', 'node')
+      ->execute();

Chained items need indentation

+++ b/modules/node/node.install
@@ -469,3 +469,98 @@ function _update_7000_node_get_types() {
+        $fields = array(
+                       'rid' => $rid,
+                       'permission' => $action . ' ' . $type . ' revisions',
+                       'module' => 'node'
+                       );

Too much indentation here.

I'd also change permission line to "{$action} {$type} revisions" for readability.

-22 days to next Drupal core point release.

+++ b/modules/node/node.install
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+   ->fetchAllAssoc('type', PDO::FETCH_OBJ);
...
+   ->addTag('translatable')
+   ->fields('r', array('rid', 'name'))
+   ->orderBy('weight')
+   ->orderBy('name');
...
+         'rid' => $rid,
+         'permission' => $action . ' ' . $type . ' revisions',
+         'module' => 'node'
...
+         ->fields($fields)
+         ->execute();
...
+       ->condition('rid', $rid)
+       ->condition('permission', $action . ' revisions')
+       ->condition('module', 'node')
+       ->execute();

Needs more indentation, should always be indented by two spaces (only on here).

+++ b/modules/node/node.install
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+         );

Needs out-denting, the closing parenthesis of an array should be on equal level with the opening parenthesis.

-22 days to next Drupal core point release.

+++ b/modules/node/node.module
@@ -1817,10 +1817,12 @@ function theme_node_search_admin($variables) {
+    if ((isset($map[$op]) && !user_access($map[$op])) && (isset($map[$op]) && !user_access($map1[$op])) && !user_access('administer nodes')) {

Two checks for isset($map[$op]), assuming the second one is supposed to be against $map1.

That's serious nitpicking, but nevertheless it should be fixed.

-22 days to next Drupal core point release.

Hey realityloop, this sounds like a nice feature! The patch looks pretty complete already. Here are some additional code style pointers:

+++ b/modules/node/node.installundefined
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+ * Utility function: fetch the node types directly from the database.

I'd suggest removing the phrase "Utility function" as these words don't really add information, and we're currently stripping stuff like that out in the docs cleanup sprint. Also, should begin with a 3rd person present indicative verb tense: "Fetches..."

+++ b/modules/node/node.installundefined
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+ * Utility function: Retrieve an array of roles matching specified conditions.

"Retrieves..."

+++ b/modules/node/node.installundefined
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+ * Migrate from global revision permissions to revisions per node type.

"Migrates..."

+++ b/modules/node/node.installundefined
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
\ No newline at end of file

Missing newline. Just hit enter at the end of node.install to put the cursor on the next line.

+++ b/modules/node/node.moduleundefined
@@ -3956,4 +3976,4 @@ function node_locale_language_delete($language) {
\ No newline at end of file

Another missing newline for node.module.

+++ b/modules/node/node.testundefined
@@ -129,14 +129,14 @@ class NodeLoadHooksTestCase extends DrupalWebTestCase {
+class NodeRevisionsByTypeTestCase extends DrupalWebTestCase {

Your test case classes need docblocks.

+++ b/modules/node/node.testundefined
@@ -145,8 +145,8 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $web_user = $this->drupalCreateUser(array('view revisions', 'revert revisions', 'edit any page content',
-                                               'delete revisions', 'delete any page content'));
+    $web_user = $this->drupalCreateUser(array('view page revisions', 'revert page revisions', 'edit any page content',
+                                               'delete page revisions', 'delete any page content'));

Funky indentation here; the subsequent lines should be indented by only two additional characters. You could put the permissions arrays on their own line, perhaps; that would be more readable. Also, these lines are over 80 chars by a lot.

+++ b/modules/node/node.testundefined
@@ -262,6 +262,145 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+class NodeRevisionsAllTestCase extends DrupalWebTestCase {

Your test case classes should have docblocks.

+++ b/modules/node/node.testundefined
@@ -262,6 +262,145 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+    $web_user = $this->drupalCreateUser(array('view page revisions', 'revert page revisions', 'edit any page content',
+                                               'delete page revisions', 'delete any page content'));

Same note as above about indentation/wrapping/char limit.

+++ b/modules/node/node.testundefined
@@ -262,6 +262,145 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+   * Check node revision related operations.
...
+    $power_web_user = $this->drupalCreateUser(array('view any revisions', 'revert any revisions',
+                                                    'delete any revisions', 'edit any page content', 'delete any page content'));
...
+    $this->assertRaw(t('@type %title has been reverted back to the revision from %revision-date.',
+                        array('@type' => 'Basic page', '%title' => $nodes[1]->title,
+                              '%revision-date' => format_date($nodes[1]->revision_timestamp))), t('Revision reverted.'));

Funky indentations again; same suggestion. Also, the second one is kind of hard to parse, so maybe you could put one array key on each line? That would work for the userLogin()s as well, actually.

+++ b/modules/node/node.testundefined
@@ -262,6 +262,145 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+   * Check node revision related operations.

"Checks..."

Generally: Going over 80 chars with the assertions is okay if they're easy to read, but when you do wrap them, wrap to 80 chars and put array elements on separate lines. Also, see http://drupal.org/coding-standards#array.

Thanks!

Nice, that looks a lot better. Here's a few things that slipped through:

+++ b/modules/node/node.installundefined
@@ -444,7 +444,7 @@ function node_install() {
+ * Fetch the node types directly from the database.

@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+ * Fetch the node types directly from the database.
...
+ * Retrieve an array of roles matching specified conditions.

+++ b/modules/node/node.testundefined
@@ -129,14 +129,166 @@ class NodeLoadHooksTestCase extends DrupalWebTestCase {
+ * Test actions against revisions for user with access to revisions for a single content type.
...
+ * Test actions against revisions for user with access to all revisions.

Should be "Fetches," "Retrieves," "Tests," etc. (The idea is that implied subject is "This function.")

+++ b/modules/node/node.moduleundefined
@@ -3956,4 +3976,4 @@ function node_locale_language_delete($language) {
\ No newline at end of file

The one end-of-file newline got fixed, but this one is still missing.

+++ b/modules/node/node.testundefined
@@ -129,14 +129,166 @@ class NodeLoadHooksTestCase extends DrupalWebTestCase {
+    $web_user = $this->drupalCreateUser(array(
+                                          'view page revisions',
+                                          'revert page revisions',
+                                          'edit any page content',
+                                          'delete page revisions',
+                                          'delete any page content'
+                                        ));
...
+    $this->assertRaw(t('@type %title has been reverted back to the revision from %revision-date.',
+                        array('@type' => 'Basic page',
+                          '%title' => $nodes[1]->title,
+                          '%revision-date' => format_date($nodes[1]->revision_timestamp)
+                        )),t('Revision reverted.'));
...
+    $this->assertRaw(t('Revision from %revision-date of @type %title has been deleted.',
+                        array('%revision-date' => format_date($nodes[1]->revision_timestamp),
+                          '@type' => 'Basic page',
+                          '%title' => $nodes[1]->title
+                        )), t('Revision deleted.'));

@@ -145,8 +297,13 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+    $web_user = $this->drupalCreateUser(array(
+                                          'view page revisions',
+                                          'revert page revisions',
+                                          'edit any page content',
+                                          'delete page revisions',
+                                          'delete any page content'
+                                        ));

These are much easier to read. However, they're indented too much. They should only be indented by two chars from the first line.

+++ b/modules/node/node.testundefined
@@ -129,14 +129,166 @@ class NodeLoadHooksTestCase extends DrupalWebTestCase {
+ * Test actions against revisions for user with access to revisions for a single content type.

Should start with "Tests..." Also, try to reword it a little so it's under 80 chars.

+++ b/modules/node/node.testundefined
@@ -187,6 +344,11 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+    $power_web_user = $this->drupalCreateUser(array('view any revisions', 'revert any revisions',
+                                                    'delete any revisions', 'edit any page content', 'delete any page content'));

Let's break this out into separate lines like you did for the others.

+++ b/modules/node/node.installundefined
@@ -469,3 +469,99 @@ function _update_7000_node_get_types() {
+ * @see Function duplicated so D8 to D9 upgrade path doesn't break.

Oh, I'm confused about this but I think I forgot to mention it before... what is this an @see to? Usually @see is a function name or a url.

For the update hook, I'd suggest reformatting it like:

/**
 * Fetches the node types directly from the database.
 *
 * Duplicated for the D9 upgrade path.
 *
 * @see _update_7000_node_get_types()
 * @ingroup update-api-7.x-to-8.x
 */

+++ b/modules/node/node.install
@@ -469,3 +469,100 @@ function _update_7000_node_get_types() {
+          'permission' => $action . ' ' . $type . ' revisions',
...
+        ->condition('permission', $action . ' revisions')

+++ b/modules/node/node.pages.inc
@@ -505,12 +505,13 @@ function node_revision_overview($node) {
+  if ((user_access('revert ' . $type . ' revisions') || user_access('revert any revisions') || user_access('administer nodes')) && node_access('update', $node)) {
...
+  if ((user_access('delete ' . $type . ' revisions') || user_access('delete any revisions') || user_access('administer nodes')) && node_access('delete', $node)) {

+++ b/modules/node/node.test
@@ -129,14 +129,168 @@ class NodeLoadHooksTestCase extends DrupalWebTestCase {
+    $this->drupalGet("node/$node->nid/revisions/$node->vid/view");
...
+    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));
...
+    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));
...
+    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage1';
...
+    $this->drupalGet('node/' . $node->nid);
...
+    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage2';
...
+    $this->drupalGet('node/' . $node->nid);

Two different coding styles are used:

'string ' . $variable;

and
"string $variable";

I would recommend standardizing, and if using the latter (my preference), wrap variables with {}
"string {$variable}";

+++ b/modules/node/node.module
@@ -1817,10 +1817,12 @@ function theme_node_search_admin($variables) {
+    $map = array('view' => 'view any revisions', 'update' => 'revert any revisions', 'delete' => 'delete any revisions');
+    $map1 = array('view' => 'view ' . $type . ' revisions', 'update' => 'revert ' . $type . ' revisions', 'delete' => 'delete ' . $type . ' revisions');

If you're making other arrays more readable, then these two could certainly be improved.

Getting even more nit-picky here. Otherwise it's looking pretty good.

-23 days to next Drupal core point release.

Attaching screenshot.

Updated issue summary.

Polishing markup and clarity.

Here's some user testing that might be helpful from patch testers.

1. Create a Drupal 8.x sandbox site.
2. Create a few user accounts with various revision permissions.
3. Apply the patch and run update.php.
4. Ensure that the permissions are updated correctly.
5. Add some per-content type revision permissions and ensure permissions behave as expected.

I have no idea if update.php is even working, but we can find out. :)

I have updated the issue summary with a little more detail and a link to the current patch.

Updated issue summary.

+++ b/modules/node/node.installundefined
@@ -444,7 +444,7 @@ function node_install() {
+ * Fetches the node types directly from the database.

+++ b/modules/node/node.installundefined
@@ -469,3 +469,100 @@ function _update_7000_node_get_types() {
+ * Fetches the node types directly from the database.

fetches -> fetch

+++ b/modules/node/node.testundefined
@@ -198,7 +371,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
     // Confirm that revisions revert properly.
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));

Not sure how these changes are related...

Other than that, looks pretty good, though I haven't actually tested it; just code review.

#87 is incorrect. @dmitrig01, please refer to http://drupal.org/node/1354#functions. The correct verb form for the one-line summary is a third-person present tense indicative, e.g., "Does foo." (The implied subject is "the function.")

See also #1310084: [meta] API documentation cleanup sprint.

Oh, re #78 and #87 -- it is correct that you should not change coding standards that are on lines that aren't a part of your functional patch. The reason for this is it can make your patch take longer to review and makes it more likely to need rerolls if other patches target those lines. See this post for more detail: http://webchick.net/please-stop-eating-baby-kittens

+++ b/modules/node/node.testundefined
@@ -187,8 +348,20 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalGet("node/$node->nid/revisions/$node->vid/view");
+    $this->drupalGet("node/{$node->nid}/revisions/$node->vid/view");

@@ -198,7 +371,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));
+    $this->drupalPost("node/{$node->nid}/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));

@@ -206,7 +379,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));
+    $this->drupalPost("node/{$node->nid}/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));

@@ -225,7 +398,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage1';
+    $new_title = "{$this->randomName(10)}testNodeRevisionWithoutLogMessage1";

@@ -235,7 +408,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalGet('node/' . $node->nid);
+    $this->drupalGet("node/{$node->nid}");

As far as I can tell, these are all the changes with the curlies that are not needed. It would be better to remove these to make your patch less co-distruptive with other potential patches, as explained in webchick's post. :)

As an aside, I'd actually disagree with #78's choice of {}. I think concatenation with . is a more common pattern in core, and since that's the style that's already used in that test, then it would be best to continue that style. But that is a minor and bikesheddable point.

+++ b/modules/node/node.installundefined
@@ -469,3 +469,100 @@ function _update_7000_node_get_types() {
+      // We only translate the built in role names

Oh, just noticed a missing period here.

Hi, I am finding the permissions for DELETE and REVERT aren't working properly. In fact it seems that none of the checkboxes for per content type revision permissions are working after I apply this patch. I'll explain further, bear with me as I'm a n00b tester :)

I created a default D8 install via the instructions at http://drupal.org/node/28245. I then created some random content, various roles and users all with varying levels of Revision permissions. For example I created the roles of: test_revision_review, test_revision_revert, and test_revision_delete, all of which had the corresponding permissions as their title suggests.

In short - the patch works properly after applied in retaining the ticked checkboxes on the permissions screen (see screenshot). However I have found upon further testing that the REVERT and DELETE permissions are not working.

For example: if a user has REVERT only permissions, they will receive "Access Denied" when going to node/41/revisions/41/revert. I then assigned VIEW permissions to this same user and still received "Access Denied" when going to node/41/revisions/41/revert, same problem when I enabled all Revision permissions for all roles.

After enabling all permissions I was only able view the revert screen, but still not able to actually revert or delete any revisions.

I am not familiar enough with the Drupal permission system so I'm not certain if what is happening here is in fact the way permissions are designed to work? Perhaps the user also needs Edit permissions? I hope my report helps rather than confuses.

--Tony

After applying patch #84 view/revert/delete revisions global configurations are cleared.

I did a little more poking around, unchecked all the boxes (and cleared caches, and logged out/logged back in), and after all of the boxes are unchecked my test user still has permissions to view the revisions as in screenshot http://drupal.org/files/880940_permissions4.png above, which they should no longer have.

After unchecking all the boxes

User can stil see this

These conditions are true for both per content type revision permissions as well as the global "View any revisions, Revert any revisions, Delete any revisions" checkboxes as shown in anthbel's post above. Nothing I do (as the administrator) within those boxes now seems to affect what the test user can see on the revisions page as is shown in the above screenshot. Also, as a testing note - I am using two separate browsers for testing - Firefox for Admin tasks and Safari for user testing, etc. I'm going to take a step back now as I'm not so sure that what I've worked on here today has actually helped or not :/. Thanks for letting me play with the big kids for a while! :) --Tony

Setting back to Needs Work to follow up on the issues seen in testing, above, and I suppose for the minor code cleanups while we're at it. :)

I had a different result than anthbel with the permissions checkboxes. After applying the patch on my sandbox - the Revision permissions were retained globally, as well as populated to each content type of Basic Page and Article. For example - if my test_revision_delete role had Revision DELETE permissions prior to the patch, they retained their global Revision DELETE permission and gained Revision DELETE permission on each content type after applying the patch, updating, and clearing caches.

Updated issue summary.

Updated issue summary.

For the sake of completeness, and because I'm new and want to do this properly - I started with a fresh install, and repeated all the same steps as I did in #93 and still got the same result.

-- added: Just remembered about, and tried to Rebuild Permissions http://d8.sandbox.dev/admin/reports/status/rebuild, which did not fix the access issues. I'll be following this issue to test later when ready.

Thanks to all the help with testing above (thanks tons to @drupleg and @anthbel!), I took a closer look at the update hooks. I think I have found at least one thing we can fix:

+++ b/modules/node/node.installundefined
@@ -469,3 +469,100 @@ function _update_7000_node_get_types() {
+/**
+ * Migrates from global revision permissions to revisions per node type.
+ */
+function node_update_8000() {
+  $actions = array('view', 'delete', 'revert');
+  $types = array_keys(_update_8000_node_get_types());
+
+  foreach ($actions as $action) {
+    $roles = array_keys(_update_8000_user_roles(FALSE, $action . ' revisions'));
+    foreach ($roles as $rid) {
+      foreach ($types as $type) {
+        $fields = array(
+          'rid' => $rid,
+          'permission' => "{$action} {$type} revisions",
+          'module' => 'node'
+        );
+        db_insert('role_permission')
+          ->fields($fields)
+          ->execute();
+      }
+      db_delete('role_permission')
+        ->condition('rid', $rid)
+        ->condition('permission', "{$action} revisions")
+        ->condition('module', 'node')
+        ->execute();
+    }
+  }
+}

Rather than doing all this, why don't we simply update entries for the three existing permissions from (e.g.) 'view revisions' to 'view any revisions'? That should resolve some of what we're seeing above. Also, I think that would eliminate the need to look up either node types or roles, unless I'm overlooking something. Three update queries instead of lots and lots of inserts and deletes (and less code for whatever bug to be hiding in).

That leaves the step of debugging the malfunctioning permission behavior @drupleg documents. There are two possibilities: Some strange records were getting to the DB during the update, or the permissions themselves are bugged. Let's fix the update hook first, and then do some testing again after that.

Re: #98 -- rebuilding permissions should not actually affect this. It's sort of misnamed. :)

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Cool, let's get some testing again.

Updated issue summary.

Updated issue summary.

Tagging as novice to test and document the patch and upgrade path. Follow the testing instructions in the issue summary above.

+++ b/core/modules/node/node.installundefined
@@ -469,3 +469,96 @@ function _update_7000_node_get_types() {
+/**
+ * Fetches the node types directly from the database.
+ *
+ * Duplicated for the D9 upgrade path.
+ *
+ * @see _update_7000_node_get_types()
+ * @ingroup update-api-7.x-to-8.x
+ */
+function _update_8000_node_get_types() {
+  $node_types = db_query('SELECT * FROM {node_type}')
+    ->fetchAllAssoc('type', PDO::FETCH_OBJ);
+
+  // Create default settings for orphaned nodes.
+  $all_types = db_query('SELECT DISTINCT type FROM {node}')->fetchCol();
+  $extra_types = array_diff($all_types, array_keys($node_types));
+
+  foreach ($extra_types as $type) {
+    $type_object = new stdClass();
+    $type_object->type = $type;
+    // In Drupal 6, whether you have a body field or not is a flag in the node
+    // type table. If it's enabled, nodes may or may not have an empty string
+    // for the bodies. As we can't detect what this setting should be in
+    // Drupal 7 without access to the Drupal 6 node type settings, we assume
+    // the default, which is to enable the body field.
+    $type_object->has_body = 1;
+    $type_object->body_label = 'Body';
+    $node_types[$type_object->type] = $type_object;
+  }
+  return $node_types;
+}
+
+/**
+ * Retrieves an array of roles matching specified conditions.
+ *
+ * @ingroup update-api-7.x-to-8.x
+ */
+function _update_8000_user_roles($membersonly = FALSE, $permission = NULL) {
+  $query = db_select('role', 'r')
+    ->addTag('translatable')
+    ->fields('r', array('rid', 'name'))
+    ->orderBy('weight')
+    ->orderBy('name');
+  if (!empty($permission)) {
+    $query->innerJoin('role_permission', 'p', 'r.rid = p.rid');
+    $query->condition('p.permission', $permission);
+  }
+  $result = $query->execute();
+
+  $roles = array();
+  foreach ($result as $role) {
+    switch ($role->rid) {
+      // We only translate the built in role names.
+      case DRUPAL_ANONYMOUS_RID:
+        if (!$membersonly) {
+          $roles[$role->rid] = $role->name;
+        }
+        break;
+      case DRUPAL_AUTHENTICATED_RID:
+        $roles[$role->rid] = $role->name;
+        break;
+      default:
+        $roles[$role->rid] = $role->name;
+    }
+  }
+
+  return $roles;
+}
+
+/**
+ * Migrates from global revision permissions to revisions per node type.
+ */
+function node_update_8000() {
+  $actions = array('view', 'delete', 'revert');
+  $types = array_keys(_update_8000_node_get_types());
+
+  foreach ($actions as $action) {
+    $roles = array_keys(_update_8000_user_roles(FALSE, "{$action} revisions"));
+    foreach ($roles as $rid) {
+      $fields = array(
+        'rid' => $rid,
+        'permission' => "{$action} any revisions",
+        'module' => 'node'
+      );
+      db_update('role_permission')
+        ->fields($fields)
+        ->condition('rid', $rid)
+        ->condition('permission', "{$action} revisions")
+        ->condition('module', 'node')
+        ->execute();
+    }
+  }
+}

Hmm, okay, I still think we can get rid of all of this and replace it with three update queries. Translate to DB API:

UPDATE role_permission SET permission = 'view any revisions' WHERE permission LIKE 'view revisions';

Reference: http://drupal.org/node/310080

Is there a reason not to do this?

Edit: dreditor fail.

Re #105 -- could you either make the patch with git diff, or squash it? It's quite confusing to review with two commits in there. I almost posted "Errrr but it's still there, " but fortunately I scrolled to the bottom and saw the second commit. :)

Excellent! The new update hook looks spot on. Two minor things related to it:

+++ b/core/modules/node/node.installundefined
@@ -444,7 +444,7 @@ function node_install() {
 /**
- * Utility function: fetch the node types directly from the database.
+ * Fetches the node types directly from the database.

These lines are outside the scope of our patch now, so we should drop this change.

+++ b/core/modules/node/node.installundefined
@@ -469,3 +469,18 @@ function _update_7000_node_get_types() {
+/**
+ * Migrates from global revision permissions to revisions per node type.
+ */

I guess we need to change this summary now, since it's just renames permissions instead of migrating them.

Also, these lines:

+++ b/core/modules/node/node.testundefined
@@ -187,8 +348,20 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalGet("node/$node->nid/revisions/$node->vid/view");
+    $this->drupalGet("node/{$node->nid}/revisions/$node->vid/view");

@@ -198,7 +371,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));
+    $this->drupalPost("node/{$node->nid}/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));

@@ -206,7 +379,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));
+    $this->drupalPost("node/{$node->nid}/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));

@@ -225,7 +398,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage1';
+    $new_title = "{$this->randomName(10)}testNodeRevisionWithoutLogMessage1";

@@ -235,7 +408,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
-    $this->drupalGet('node/' . $node->nid);
+    $this->drupalGet("node/{$node->nid}");

These changes are also outside the scope of the patch, so let's revert them.

Those fails are testbot issues. I've requested a retest, but maybe best to wait until tomorrow. :)

The patch in #111 already includes automated tests and has been reviewed for coding standards. It should be manually tested to ensure it behaves as expected.

Follow these steps to test the patch:

1. Create a Drupal 8.x sandbox site.
2. Create a few user accounts with various revision permissions.
3. Apply the patch and run update.php.
4. Ensure that the permissions are updated correctly.
5. Add some per-content type revision permissions and ensure permissions behave as expected.

Steps 1-4 work as expected, though there is a warning about whitespace in step 3: "warning: 1 line adds whitespace errors"
Step 5 does not work as expected.

Any role that has the 'view revisions' permission is able to view the revision list as expected, but the delete and revert operations are not showing up for anyone but my administrator user account.

Looks like the patch needs more work, then. Untagging until #115 is addressed. It's documented in @drupleg's review in #95, too, so looks like it has persisted from earlier versions of the patch.

Thanks @realityloop! First off, this patch is pretty thorough already and I very much like the extensive test coverage. Detailed review follows, including both questions about the functionality and minor stylistic points that are probably originally from the existing code. (The latter cleanups would be a good novice task).

1. +++ b/core/modules/node/node.installundefined
   @@ -558,6 +558,21 @@ function node_update_8002() {
   +function node_update_8003() {
   

   Since we have this in here, we probably also need to add upgrade path test coverage. (I know you are very excited!) Reference: http://drupal.org/node/1429136

2. +++ b/core/modules/node/node.moduleundefined
   @@ -1923,14 +1923,19 @@ function theme_node_search_admin($variables) {
   +  $map1 = array(
   
   +++ b/core/modules/node/node.testundefined
   @@ -2333,11 +2508,18 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
   +  // Map revision permission names to node type revision access ops.
   +  protected $map1 = array(
   

   Let's specify "for page nodes" in the comment, and give the map a more descriptive name than $map1 in both the test and the API function. Maybe $page_map or something?

3. +++ b/core/modules/node/node.moduleundefined
   @@ -3164,6 +3169,25 @@ function node_list_permissions($type) {
   +    "delete $type revisions" => array(
   +      'title' => t('%type_name: Delete revisions', array('%type_name' => $info->name)),
   +      'description' => t('Requires <em>Delete own content</em> or <em>Delete any content</em> to nodes of this type.'),
   

   I'm still not quite sure this is the expected behavior. Is this currently the case for "delete any revisions" as well?

4. +++ b/core/modules/node/node.moduleundefined
   @@ -3164,6 +3169,25 @@ function node_list_permissions($type) {
   +    "delete $type revisions" => array(
   
   +++ b/core/modules/node/node.pages.incundefined
   @@ -594,16 +594,18 @@ function node_revision_overview($node) {
   -  if ((user_access('revert revisions') || user_access('administer nodes')) && node_access('update', $node)) {
   +  if ((user_access("revert {$type} revisions") || user_access('revert any revisions') || user_access('administer nodes')) && node_access('update', $node)) {
   ...
   -  if ((user_access('delete revisions') || user_access('administer nodes')) && node_access('delete', $node)) {
   +  if ((user_access("delete {$type} revisions") || user_access('delete any revisions') || user_access('administer nodes')) && node_access('delete', $node)) {
   

   This is totally unrelated to this patch, but it's bugging me and I want to make note of it: Why does administer nodes get AND-ed with the node access? Why is that the expected behavior? Shouldn't it be in its own "or"? Shouldn't administer nodes work regardless of node access? What happens elsewhere in the module?

5. +++ b/core/modules/node/node.testundefined
   @@ -164,8 +168,15 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
   -    $web_user = $this->drupalCreateUser(array('view revisions', 'revert revisions', 'edit any page content',
   -                                               'delete revisions', 'delete any page content'));
   +    $web_user = $this->drupalCreateUser(
   +      array(
   +        'view page revisions',
   +        'revert page revisions',
   +        'delete page revisions',
   +        'edit any page content',
   +        'delete any page content'
   +      )
   +    );
   

   Technically this change is out of scope, but I'm +1 because it's significantly more readable this way. ;)

6. +++ b/core/modules/node/node.testundefined
   @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
   +   * Checks node revision related operations.
   

   "Related operations" is kind of strange and vague. Maybe "Tests node revision operations" or "Tests node revision CRUD operations"?

7. +++ b/core/modules/node/node.testundefined
   @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
   +    // Get last node for simple checks.
   

   Needs an article ("Get the last node...").

8. +++ b/core/modules/node/node.testundefined
   @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
   +    // Confirm that revisions revert properly.
   ...
   +    // Confirm revisions delete properly.
   

   I'd say "are properly reverted" and "are properly deleted" (I was a bit confused at first by the text).

9. +++ b/core/modules/node/node.testundefined
   @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
   +    $this->assertTrue(($nodes[1]->body['und'][0]['value'] == $reverted_node->body['und'][0]['value']), t('Node reverted correctly.'));
   

   We should probably use LANGUAGE_NOT_SPECIFIED rather than 'und' here.

10. +++ b/core/modules/node/node.testundefined
    @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +    $this->assertTrue(db_query('SELECT COUNT(vid) FROM {node_revision} WHERE nid = :nid and vid = :vid',
    +      array(
    +        ':nid' => $node->nid,
    +        ':vid' => $nodes[1]->vid
    +      ))->fetchField() == 0, t('Revision not found.'));
    

    Let's put at least the final argument on a separate line for better readability.

11. +++ b/core/modules/node/node.testundefined
    @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +   * Checks that revisions are correctly saved without log messages.
    

    I'd clarify this a little, since it took me a good 5 mins. to understand what this test was doing (granted it is 6a). :P How about:
    Checks that empty log messages are saved correctly in new and updated revisions.
    (Not positive it fits in 80 chars, so double-check.)

12. +++ b/core/modules/node/node.testundefined
    @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +    $this->assertEqual($node_revision->log, $log, t('After an existing node revision is re-saved without a log message, the original log message is preserved.'));
    

    Why is this the expected behavior?

13. +++ b/core/modules/node/node.testundefined
    @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +/**
    + * Tests actions against revisions for user with access to all revisions.
    + */
    +class NodeRevisionsAllTestCase extends DrupalWebTestCase {
    

    Scanning the code for this class, it looks pretty familiar. I assume it's based on the existing revision tests davereid and I worked on when we fixed that critical recently? (I did not actually open the codebase and read since it is before 6a.) ;) Two questions:

    1. What are the differences between this test case and the other?
    2. If we are doing a lot of the same stuff in both classes, maybe we should define a shared base class?

14. +++ b/core/modules/node/node.testundefined
    @@ -197,6 +208,158 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +    // Create initial node.
    ...
    +    // Get original node.
    ...
    +      // Create revision with random title and body and update variables.
    

    We should have articles in the inline comments ("an initial node," "the original node," "a revision," "a random title and body").

15. +++ b/core/modules/node/node.testundefined
    @@ -206,12 +369,24 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    +    // Create and login user.
    +    $power_web_user = $this->drupalCreateUser(
    +      array(
    +        'view any revisions',
    +        'revert any revisions',
    +        'delete any revisions',
    +        'edit any page content',
    +        'delete any page content'
    +      )
    +    );
    +    $this->drupalLogin($power_web_user);
    

    1. Nitpick: "log in" should be two words. (I realize this may predate this patch). ;)
    2. A slightly more specific name than "power web user" might be good as well. Maybe $content_admin or something.
    3. Finally, which account was logged in previously, and which permissions did it have? Can node and revision permission be tested separately?

16. +++ b/core/modules/node/node.testundefined
    @@ -206,12 +369,24 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    -    $this->drupalGet("node/$node->nid/revisions/$node->vid/view");
    ...
    +    $this->drupalGet("node/{$node->nid}/revisions/{$node->vid}/view");
    ...
    -    $this->drupalGet("node/$node->nid/revisions");
    +    $this->drupalGet("node/{$node->nid}/revisions");
    

    These changes seem out of scope and should be reverted.

17. +++ b/core/modules/node/node.testundefined
    @@ -206,12 +369,24 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    -    $this->assertText($node->body[LANGUAGE_NOT_SPECIFIED][0]['value'], t('Correct text displays for version.'));
    ...
    +    $this->assertText($node->body['und'][0]['value'], t('Correct text displays for version.'));
    
    @@ -222,7 +397,7 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
    -    $this->assertTrue(($nodes[1]->body[LANGUAGE_NOT_SPECIFIED][0]['value'] == $reverted_node->body[LANGUAGE_NOT_SPECIFIED][0]['value']), t('Node reverted correctly.'));
    +    $this->assertTrue(($nodes[1]->body['und'][0]['value'] == $reverted_node->body['und'][0]['value']), t('Node reverted correctly.'));
    

    We should be using the constant here, not its value. What's the justification for this change?

18. +++ b/core/modules/node/node.testundefined
    @@ -2333,11 +2508,18 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
    -  // Map revision permission names to node revision access ops.
    +  // Map revision permission names to any node revision access ops.
    

    Let's say "global" rather than "any" here.

19. +++ b/core/modules/node/node.testundefined
    @@ -2405,16 +2586,17 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
    -    $this->assertFalse(_node_revision_access(FALSE, 'view', $admin_account), '_node_revision_access() returns FALSE with an invalid node.');
    

    This assertion does not seem to be replaced with anything. What's the case for removing it?

20. Finally, in general, the assertion messages (the final argument as displayed in the test runner) do not need to be translated with t(). They appear both ways in the patch (and in the current codebase), but in the lines we are adding and updating, let's remove t() from the messages for better performance and decoupling. (Note: Remove the t() on message texts only in the lines added or updated by this patch, not from any other lines in the file.) Reference: http://drupal.org/simpletest-tutorial-drupal7#t

Phew! As I mentioned above, some of these are minor cleanups, so tagging as novice for cleaning up the stylistic points above. Please include an interdiff with any updated patch so realityloop and I can easily review the changes. Please also indicate the # for each of my points above that you address for clarity.

Thanks @realityloop! The patch looks much cleaner now. However, there are still a number of changes that are not related to the issue, which make the patch harder to review:

+++ b/core/modules/node/node.testundefined
@@ -206,30 +374,52 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
-    $this->drupalGet("node/$node->nid/revisions");
+    $this->drupalGet("node/{$node->nid}/revisions");
...
-      $this->assertText($log, t('Log message found.'));
+      $this->assertText($log, 'Log message found.');
...
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), t('Revert'));
+    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/revert", array(), 'Revert');
...
-    $this->assertTrue(($nodes[1]->body[LANGUAGE_NOT_SPECIFIED][0]['value'] == $reverted_node->body[LANGUAGE_NOT_SPECIFIED][0]['value']), t('Node reverted correctly.'));
+    $this->assertTrue(($nodes[1]->body[LANGUAGE_NOT_SPECIFIED][0]['value'] == $reverted_node->body[LANGUAGE_NOT_SPECIFIED][0]['value']), 'Node reverted correctly.');
...
-    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/delete", array(), t('Delete'));
+    $this->drupalPost("node/$node->nid/revisions/{$nodes[1]->vid}/delete", array(), 'Delete');

@@ -255,9 +445,9 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
-    $this->assertText($new_title, t('New node title appears on the page.'));
+    $this->assertText($new_title, 'New node title appears on the page.');
...
-    $this->assertEqual($node_revision->log, $log, t('After an existing node revision is re-saved without a log message, the original log message is preserved.'));
+    $this->assertEqual($node_revision->log, $log, 'After an existing node revision is re-saved without a log message, the original log message is preserved.');

These changes (while correct) are not within the scope of the patch, and should be removed.

I took pictures before and after the patch to see what changed and sure enough the revisions permissions were there after the patch :)

Before:

After:

I don't know what else you changed but it seems good to me, if you want anymore testing just say so, and specify what you want me to test :)

Since I am not fully sure what the patch does (and its been a while since I looked at this issue) I don't think I am quite qualified to call this RTBC, sorry.

Updated issue summary.

#132: 880940-per-content-type-revision-controls-132.patch queued for re-testing.

Argument 1 passed to node_save() must be an instance of Drupal\node\Node, instance of stdClass given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/node/node.test on line 280 and defined

This is because $updated_node isn't an entity

    $updated_node = (object) array(
      'nid' => $node->nid,
      'vid' => $node->vid,
      'uid' => $node->uid,
      'type' => $node->type,
      'title' => $new_title, 
      'log' => '',
    );
    node_save($updated_node);

Drupal 7 module to add this functionality (until we all upgrade to D8), can be found as a patch to the D6 module at http://drupal.org/node/1211568

This is just a note for those of us who need this, but can't wait until the marvelous D8 is available. :)

I removed some out-of-scope changes (attached). Patch is much smaller now. :)

@realityloop and I discussed this issue in IRC, and I think what this issue really needs is more tests. An upgrade path test, for starters, now that we have a process for adding them.

Then, I'd also like to see more functional tests. Review the following comments to see what needs additional coverage:

• #93
• #94
• #95
• #97
• #98
• #115
• #116
• #117

There should be test cases for some of the bugs described in some of these comments. Additionally, it would be good to get some really detailed manual testing steps that folks have done (for which permissions they configured, etc.) and codify the expected behavior in tests.

Additional manual testing for various combinations of permissions would also be good (not just for the upgrade path). Please document any manual testing done in the issue.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php
@@ -16,9 +16,16 @@ class NodeRevisionPermissionsTest extends NodeTestBase {
+    'view' => "view page revisions",
+    'update' => "revert page revisions",
+    'delete' => "delete page revisions"

We usually prefer single quotes.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php
@@ -85,10 +92,10 @@ class NodeRevisionPermissionsTest extends NodeTestBase {
     $permutations = $this->generatePermutations($parameters);

$type_map is not added to $parameters anywhere, so it doesn't seem like the new permissions are acutally tested, even though the assertions are changed to say that.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionsTest.php
@@ -10,11 +10,12 @@ namespace Drupal\node\Tests;
+  protected $profile = "standard";

Why is this needed?

+++ b/core/modules/node/node.module
@@ -1772,13 +1773,27 @@ function theme_node_search_admin($variables) {
+  if (isset($node->type)) {
+    $type = $node->type;
+  }
+  else {
+    // Set type to fake on invalid node so Node revision permissions test
+    // doesn't fail.
+    $type = 'fake';
+  }

I don't really grasp why this is needed. We should try to avoid stuff like this.

+++ b/core/modules/node/node.module
@@ -3063,6 +3078,25 @@ function node_list_permissions($type) {
+    "view any revisions" => array(
+      'title' => t('View any revisions', array('%type_name' => $type->name)),
+    ),
+    "revert any revisions" => array(
+      'title' => t('Revert any revisions', array('%type_name' => $type->name)),
+    ),
+    "delete any revisions" => array(
+      'title' => t('Delete any revisions', array('%type_name' => $type->name)),
+    ),

These are already in node_permissions() this should be dropped here.

+++ b/core/modules/node/node.pages.inc
@@ -633,12 +633,13 @@ function node_revision_overview($node) {
+  if (user_access('administer nodes') || (user_access("revert {$type} revisions") || user_access('revert any revisions') && node_access('update', $node))) {
...
+  if (user_access('administer nodes') || (user_access("delete {$type} revisions") || user_access('delete any revisions') && node_access('delete', $node))) {

You can leave out the curly braces around {$type}

I added a test for the issue in #93. The test shows that the revision permissions for a specific content type work for that content type but not for another.

Split out the tests to prove they work.

Working on the clean-up points in #147

All issues in #147 have been addressed.

Unassigning for review

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php
@@ -12,13 +12,20 @@ namespace Drupal\node\Tests;
+  protected $profile = "standard";

@@ -32,19 +39,28 @@ class NodeRevisionPermissionsTest extends NodeTestBase {
+    $types = array('page', 'article');

This is needed because we use the 'page' and 'article' node types, right? Is there any reason for that, or could we just as well use WebTestBase::drupalCreateContentType to create our own content type. That would definitely be more efficient than using standard profile.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php
@@ -12,13 +12,20 @@ namespace Drupal\node\Tests;
+    'view' => 'view any revisions',
+    'update' => 'revert any revisions',
+    'delete' => 'delete any revisions',

I've read this a couple times now, and found a bit strange every time, but by now I am pretty certain that this should rather be "view all revisions". I'm not a native speaker, but "any" in this case just doesn't feel right. I might be wrong, of course.

In response to #154:

1) That's a possibility, but I don't know that it's a necessity.

2) You are correct. It could be "view all revisions" or "view any revision".