Every time I try to write tests for something related to node access, I spend a good 30 minutes re-parsing what, exactly, the module is supposed to do.

Comments

xjm’s picture

Somewhat related (maybe a followup), I think NodeQueryAlterTest should possibly be NodeAccessQueryAlterTest.

xjm’s picture

Status:Active» Needs review
StatusFileSize
new5.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_access_test_docs-1674870-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, node_access_test_docs-1674870-1.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
Issue tags:+needs backport to D7

#3: node_access_test_docs-1674870-1.patch queued for re-testing.

jhodgdon’s picture

Status:Needs review» Needs work

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'));
}
xjm’s picture

Status:Needs work» Needs review
Issue tags:-needs backport to D7

#3: node_access_test_docs-1674870-1.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, node_access_test_docs-1674870-1.patch, failed testing.

xjm’s picture

Assigned:xjm» Unassigned
Issue tags:+Novice, +Needs reroll
pwieck’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new5.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,106 pass(es).
[ View ]

Reroll to get things back on track.

jhodgdon’s picture

Status:Needs review» Needs work

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

pwieck’s picture

Disregard this post

pwieck’s picture

Status:Needs work» Needs review
StatusFileSize
new2.83 KB
new5.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es).
[ View ]

@jhodgdon, I think this addresses all #6.

jhodgdon’s picture

Status:Needs review» Needs work

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!

pwieck’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,293 pass(es).
[ View ]

@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.
jhodgdon’s picture

Status:Needs review» Needs work

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". :)

xjm’s picture

Also, they're now "state variables" rather than "configuration variables". :)

jhodgdon’s picture

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...

xjm’s picture

@jhodgdon, it's not a change in terminology, it's a change in the functional code since this docs patch was written. This test now uses the Drupal 8 state system, not the Drupal 7 configuration variable system. See here:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

xjm’s picture

Also, a cleanup would be out of scope for this patch anyway. Presumably, anything that refers to configuration variables is referring to legacy D7 code that hasn't been converted to config or state yet. If not, then it's a docs bug for that specific place in the codebase.

theemg’s picture

Issue summary:View changes
Issue tags:+Needs reroll

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

vegantriathlete’s picture

theemg’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new5.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,751 pass(es).
[ View ]

Rerolled

theemg’s picture

StatusFileSize
new5.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,752 pass(es).
[ View ]

Missed slash in drupal global namespace.

cilefen’s picture

@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.

JacobSanford’s picture

Issue tags:-Needs reroll

Removing 'Needs reroll' as patch applies cleanly.

jhodgdon’s picture

Status:Needs review» Needs work

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."

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new6.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,018 pass(es).
[ View ]
new2.93 KB

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

jhodgdon’s picture

Status:Needs review» Needs work

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!

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new6.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,021 pass(es).
[ View ]
new510 bytes

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

jhodgdon’s picture

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

  • Commit 4000472 on 8.x by jhodgdon:
    Issue #1674870 by amitgoyal, theemg, pwieck, xjm: Fix up docs for...
jhodgdon’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Patch (to be ported)

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.

xjm’s picture

The original patch in #3 is probably more accurate for D7, so it's a matter of incorporating some of the subsequent improvements into that patch.

joshi.rohit100’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new5.64 KB
PASSED: [[SimpleTest]]: [MySQL] 41,165 pass(es).
[ View ]

D7 patch for this.
Please review now.

jhodgdon’s picture

Status:Needs review» Needs work

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!

xjm’s picture

Again, I'd suggest starting from the patch in #3 instead.