Improve documentation for the node_access_test module

Fixing up this documentation seems like an excellent idea, thanks!

I saw a couple of things in the patch that I think are not quite accurate:

a) @file header to node.module:

+ * This module's functionality depends on two configuration variables:
+ * - node_test_node_access_all: Used in NodeQueryAlterTest to enable the
+ *   node_access_all grant realm.

The variable I see in the code is node_test_node_access_all_uid, which seems to be setting the UID that has access to all content? (I'm looking at the 8.x code.)

b) grants hook:

+ * - node_access_test: Grants users view privileges when they have the

I think this is called node_access_test_author, not node_access_test

c) grants hook:

+ * - node_access_all: Provides grants for the user whose user ID matches the
+ *   'node_test_node_access_all' configuration variable. Access control on this

See note on variable in (a).

d) Nitpick: Refer to "the Node module" (capitalized).

e) I think the patch should not make this change in the code:

function node_access_test_permission() {
-  return array('node test view' => array('title' => 'View content'));
+  return array('node test view' => array('title' => 'View restricted content'));
 }

Reroll to get things back on track.

Thanks for the reroll! Setting status as per #6, assuming this latest patch is just a reroll.

Disregard this post

@jhodgdon, I think this addresses all #6.

Thanks! There are still a couple of problems in this patch (sorry, may have missed them in previous reviews):

a)

+ * @see Drupal\node\Tests\NodeQueryAlterTest
+ * @see Drupal\node\Tests\NodeAccessBaseTableTest

When using namespaces in documentation, always start with \ -- this affects several other lines in the patch as well.

b) I looked through the code and there is a third configuration variable in use:
node_access_test_secret_catalan
This is not mentioned in the @file header.

Other than that, this looks great!

@jhodgdon made #14 changes. I don't know if I have the 'node_access_test_secret_catalan:' right. Here is what I came up with.

/**
 * @file
 * Test module for testing the node access system.
 *
 * This module's functionality depends on two configuration variables:
 * - node_access_test.no_access_uid: Used in NodeQueryAlterTest to enable the
 *   node_access_all grant realm.
 * - node_access_test.private: When TRUE, the module controls access for nodes
 *   with a 'private' property set, and inherits the default core access for
 *   nodes without this flag. When FALSE, the module controls access for all
 *   nodes.
 * - node_access_test_secret_catalan: When set to TRUE and using the Catalan
 *   'ca' language code, makes all Catalan content secret.

Yeah, that's what I thought the Catalan thing was doing too. :)

The only thing now is that this line in the @file header:

+ * This module's functionality depends on two configuration variables:

should say "three" not "two". :)

Oh really? Where? I was looking at some API documentation today and it seemed to be all still called "configuration variables" -- that was the update function. If the terminology is being updated, it needs to be updated everywhere, not piecemeal...

I'm at the Austin sprint, I will be working on rerolling it.

Rerolled

Missed slash in drupal global namespace.

@theemg You did an awesome job on your first reroll. The actual details in question are beyond my ken at this stage. Perhaps @xjm or @jhodgdon can comment.

Removing 'Needs reroll' as patch applies cleanly.

Thanks for the rerolls!

I have reviewed the documentation for this module carefully, and it is mostly great. Still needs a few fixes:

a) In the @file doc block at the top, it talks about configuration variables, but they are actually state variables. This error is repeated in many function doc blocks below.

b) Same place - talks about there being 2 variables but then lists 3 variables. For future maintainability, maybe say "the following" or something like that instead of saying how many?

c) There's a doc line in node_access_test_node_grants that runs over 80 characters:

 *   'node_access_test.no_access_uid' configuration variable. Access control on this

Needs rewrapping.

d) There is only one code comment in node_access_test_node_grants(), which says "First, ..." and describes what the first node grant is. I think we should either take this comment out (the functionality is clearly documented now in the function header) or add comments for the other grants. Either way.

e) Missing from this patch: We need to remove the blank line here between documentation and function:

/**
 * Implements hook_node_predelete().
 */

function node_access_test_node_predelete(NodeInterface $node) {
  db_delete('node_access_test')->condition('nid', $node->id())->execute();
}

f) Missing from this patch:

/**
 * Helper for node insert/update.
 */
function _node_access_test_node_write(NodeInterface $node) {

This needs a new one-line doc that tells what the function does, starting with a 3rd-person verb. Probably something like "Saves the private status of the node in the database."

Please review the revised patch with fixes in #27. I have fixed all the points in a, b, c, d, e, f.

99% there!

 /**
  * Helper for node insert/update.
+ *
+ * Saves the private status of the node in the database.
  */
 function _node_access_test_node_write(NodeInterface $node) {

This "Saves the private status..." line needs to be the first line of the function documentation. I think actually that we could get rid of the "Helper for ..." line -- either it should list the actual functions that it is a helper for, or we should remove it, and if we keep it, it needs to go below the other line.

Thanks!

I have removed the "Helper for node insert/update" line as per #29. Please review.

Thanks! This one looks good to me. Go Amit!!!

Thanks again everyone! Committed to 8.x.

This also needs to be ported to 7.x, keeping in mind that @see references to test classes are probably different there, and there is no such thing as a "state variable" in 7. So just putting the same patch on for D7 is not going to work. It will need a little editing.

D7 patch for this.
Please review now.

In Drupal 7, "state variables" do not exist. So:

+ * This module's functionality depends on the following state variables:

That is not correct.

So it looks like the patch from Drupal 8 was just applied to Drupal 7 blindly. Before I give it a full review, can you please go through the documentation that is added and see if it actually all applies to Drupal 7? For instance, I am not sure if the test class names are the same, and I am not sure if all the functionality in the Drupal 8 version of the test module actually exists in the Drupal 7 version. Thanks!