[D7] - Simple User-Node Access (S.U.N.A.)

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxmarcoscano2017349git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Part 1: Ventral Review

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Review of the 7.x-1.x branch:

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  suna.admin.inc
  suna.install

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

FILE: /var/www/drupal-7-pareview/pareview_temp/suna.admin.inc
--------------------------------------------------------------------------------
FOUND 36 ERROR(S) AND 1 WARNING(S) AFFECTING 28 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
4 | ERROR | The second line in the file doc comment must be " * @file"
5 | ERROR | The third line in the file doc comment must contain a
| | description and must not be indented
19 | ERROR | Array closing indentation error, expected 2 spaces but found 4
24 | ERROR | Array closing indentation error, expected 2 spaces but found 4
36 | ERROR | Whitespace found at end of line
48 | ERROR | Array closing indentation error, expected 2 spaces but found 4
49 | ERROR | Whitespace found at end of line
54 | ERROR | Array closing indentation error, expected 2 spaces but found 4
55 | ERROR | Whitespace found at end of line
60 | ERROR | Array closing indentation error, expected 2 spaces but found 4
63 | ERROR | Whitespace found at end of line
67 | ERROR | Function comment short description must be on a single line
83 | ERROR | Function comment short description must be on a single line
83 | ERROR | Function comment short description must end with a full stop
85 | ERROR | Function doc comment must end on the line before the function
| | definition
90 | ERROR | Whitespace found at end of line
91 | ERROR | No space before comment text; expected "// no matter what the
| | user enters, we take only the first numeric value" but found
| | "//no matter what the user enters, we take only the first
| | numeric value"
91 | ERROR | Inline comments must start with a capital letter
91 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
92 | ERROR | A cast statement must be followed by a single space
97 | ERROR | Whitespace found at end of line
102 | ERROR | Function comment short description must end with a full stop
106 | WARNING | There must be no blank line following an inline comment
106 | ERROR | No space before comment text; expected "// this array has the
| | structure: array($nodeid => array($userid => timestamp ))" but
| | found "//this array has the structure: array($nodeid =>
| | array($userid => timestamp ))"
106 | ERROR | Inline comments must start with a capital letter
107 | ERROR | Whitespace found at end of line
109 | ERROR | Whitespace found at end of line
110 | ERROR | There should be no white space after an opening "{"
110 | ERROR | No space before comment text; expected "// if the node is
| | already referenced" but found "//if the node is already
| | referenced"
110 | ERROR | Inline comments must start with a capital letter
110 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
110 | ERROR | Comments may not appear after statements.
111 | ERROR | Whitespace found at end of line
116 | ERROR | Whitespace found at end of line
118 | ERROR | Closing brace indented incorrectly; expected 0 spaces, found 2
120 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/suna.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/suna.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
26 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/suna.module
--------------------------------------------------------------------------------
FOUND 35 ERROR(S) AND 3 WARNING(S) AFFECTING 21 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | The second line in the file doc comment must be " * @file"
5 | ERROR | The third line in the file doc comment must contain a
| | description and must not be indented
72 | ERROR | There should be no white space after an opening "("
72 | ERROR | There should be no white space before a closing ")"
83 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
83 | ERROR | Function comment short description must end with a full stop
87 | ERROR | Whitespace found at end of line
88 | ERROR | No space before comment text; expected "// we should act only
| | on content types defined with this setting" but found "//we
| | should act only on content types defined with this setting"
88 | ERROR | Inline comments must start with a capital letter
88 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
90 | ERROR | Whitespace found at end of line
93 | ERROR | Whitespace found at end of line
95 | ERROR | Whitespace found at end of line
96 | ERROR | There should be no white space after an opening "("
96 | ERROR | There should be no white space after an opening "("
96 | ERROR | There should be no white space before a closing ")"
107 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
107 | ERROR | Function comment short description must be on a single line
107 | ERROR | Function comment short description must end with a full stop
111 | ERROR | Whitespace found at end of line
115 | ERROR | There should be no white space after an opening "{"
115 | ERROR | No space before comment text; expected "// user access has
| | expired" but found "//user access has expired"
115 | ERROR | Inline comments must start with a capital letter
115 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
115 | ERROR | Comments may not appear after statements.
121 | ERROR | Whitespace found at end of line
126 | ERROR | Whitespace found at end of line
129 | ERROR | Whitespace found at end of line
133 | ERROR | Function comment short description must be on a single line
133 | ERROR | Function comment short description must end with a full stop
138 | ERROR | Whitespace found at end of line
150 | ERROR | Expected 2 space(s) before asterisk; 1 found
150 | ERROR | Function comment short description must end with a full stop
151 | ERROR | Expected 2 space(s) before asterisk; 1 found
153 | WARNING | Line exceeds 80 characters; contains 107 characters
153 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
153 | ERROR | No space before comment text; expected "// deletes a scheduled
| | job with these parameters. FALSE is returned if no job is
| | found for this pair nid/uid" but found "//deletes a scheduled
| | job with these parameters. FALSE is returned if no job is
| | found for this pair nid/uid"
153 | ERROR | Inline comments must start with a capital letter

Part 2: Manual Testing

If remove a content type, you must delete the variable. For example, I removed the content type "article", but the variable suna_access_article remains

If i use

git clone http://git.drupal.org/sandbox/marcoscano/2017349.git simple_user_node_access__s_u_n_a_

it fetch only a .info file. Please fix.

Hi marcoscano,

I've manually reviewed your module and found the following issues:

1) After enabling access for articles, I tried setting the days field to an integer and received this error:

Strict warning: Only variables should be passed by reference in suna_page_submit() (line 90 of /Users/nkerr/Sites/acquia-drupal/sites/all/modules/suna/suna.admin.inc).

I suggest reevaluating how you process this field. Perhaps it would be better to use a date element? Also, there is no default value for this field, even though a user would expect the time they entered to be displayed.

2) Using the variables table to save large amounts of settings or data is inadvisable. It would be better to use a database table to save the settings, and even better to use ctools for import/export goodness.

Hope this helps.
Norman

Ventral Review

There are still some coding style issues with your project:

FILE: README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
28 | WARNING | Line exceeds 80 characters; contains 82 characters
32 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

And there is also a warning from DrupalPractice:

FILE: suna.admin.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
129 | WARNING | Form error messages are user facing text and must run through
| | t() for translation
--------------------------------------------------------------------------------

Manual review

• Move the checkbox for enabling suna on a content type to the edit page. We dont need a new tab just for one checkbox

• In suna_install(), don't set defaults for all your variables. All variables can have default provided when you call variable_get('varname', 'defaultvalue'). I think you can also remove suna_node_type_insert() for the same reason. The _delete is fine though.
• Why is $type hardcoded in suna_form_node_type_form_alter()? Leftover debug code? Also variable_del('suna_article'); in suna_save_type_config().
• In suna_node_view_alter(), don't call exit() directly. Just returning drupal_access_denied() should be sufficient.
• Also in suna_node_view_alter(), can the if statement be rewritten as
  

    if (user_access('administer nodes') || user_access('bypass suna content access') || suna_has_access($uid, $nid)) {
  

• Any user-facing content like $string_out should be passed through the t() function.

I have a few questions about the general approach. Managing the scheduled jobs seems to be a large part of what this module is doing. On the project page you state there are no new DB tables, but I think this is a case where a small table would make the module much more efficient. Storing a uid / nid / timestamp combo in a table, and then clearing it out during hook_cron() would be much simpler and more reliable. Potentially very large serialized arrays for each node id is a tough way to do it.

This module intentionally works outside the standard Drupal access control system, so no integration with other modules (like views) is possible out of the box. I don't think this is a blocking issue, but you may want to warn your users about this on the project page.

----
Top Shelf Modules - Crafted, Curated, Contributed.