[D7] Session node access

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

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.

Overall very clean code.

Manual review:

session_node_access.module:13 - Consider setting 'restrict access' => TRUE for the 'administer session node access' permission as this should only be granted to trusted users.

session_node_access.module:41 - This is not an implementation of hook_form() from the node api (https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...). It is a form builder and should be documented according to https://drupal.org/coding-standards/docs#forms

session_node_access.module:173 - Form submit handler should be documented according to https://drupal.org/coding-standards/docs#forms

session_node_access.module:249 - '=' is the default operation for condition() and does not need to be specified.

session_node_access.module:262 - Use db_query() for static select queries. See https://drupal.org/node/310075

In session_node_access.module
Line 267: table names should be enclosed in {curly_brackets} [sql_curly]
$result = db_query('SELECT * FROM session_node_access WHERE id = 0');

Reading through the code, I noticed this little thing:

// line 96-97
array_unshift($content_types, '');
unset($content_types[0]);

first you prepend an empty string (which gets index 0) then unset it?
A little further down on lines 119-120, 199-200 and 212-213, the same thing is done again. Why?

Next to that, I'm curious why you used a table with just 1 record (the queries have a fixed id = 0) and not a variable to store the settings?

> Content created by anonymous users is set to be unpublished until a mod reviews it. With this module the anonymous user gets to view/edit/delete their freshly created content without it to be accessible to anyone else.

Isn't this covered by Workbench module?

> I used workbench and didn't see the functionality you are referring to, maybe my use case is not explained well enough?

AFAIK with workbench access you can edit your own submission while it's in draft status.

> On top of that, even if workbench had this functionality implemented, it is a very complex set of modules and 'workbench access' depends on the main workbench module which means overhead and many hooks firing

That is a fair point!

This module looks like it has the same use case as The Sign-Up Problem (SUP), and like that, it uses a session cookie for persistence.

It looks pretty abandoned to me, so you may consider taken over its namespace.

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Unless you can argue that your module is unique and cannot be fitted into the namespace listed above, please open an issue in the most relevant issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

I think you misunderstood what I said.

I did not suggest that you do anything with the SUP code.

I suggested that you took over its namespace (i.e. that the abandoned SUP module was demoted to a sandbox, and then replaced by your module).

This is because we do not want to have too many abandoned modules as they pollute the Drupal.org project namespace. Instead, if a new and better module that does roughly the same thing appear, we prefer that the new module "takes over" the slot in the namespace occupied by the abandoned module.

However, if your module does not do roughly the same thing as SUP, my suggestion is moot.

If you are working on this project:

Please have a look at this
http://pareview.sh/pareview/httpgitdrupalorgsandboxgbyteco2212241git

If not please close this issue, to help others work maintaining the issue queue.

minor coding standard errors are not application blockers, please do a real manual review.

Hello,

At line: 124 of your module file you use the variable $option in order to build the options for the roles checkboxes options. But in your form instead of user roles in the available options also appear content types. You can use a different variable for these options to prevent that. In order to check this try to add a new Content Type and check your form again.

I also notice that its necessary to check options from all of the fieldsets in order to have the functionality you suggest. You can provide a validate function in order to specify that.

Best Regards.

Hello

In the module there is no implementation of hook_help. Which is very useful for the module information.
Please provide the hook_help in the module.

Thanks

Hello,

There is some error error when review it from pareview tool.

Error is
FILE: /var/www/drupal-7-pareview/pareview_temp/session_node_access.module
-------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------
243 | ERROR | Type hint "array" missing for $settings

Please fix this error.

Automated Review

No errors found: http://pareview.sh/pareview/httpgitdrupalorgsandboxgbyteco2212241git

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause

Master Branch

Yes: Follows

Licensing

Yes: Follows

3rd party assets/code

Yes: Follows

README.txt/README.md

Yes: Follows

Code long/complex enough for review

Yes: Follows

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Major finding, needs work
2. (+)

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Some suggestion:
- Check for settings array items exist to prevent warnings and notices like:

Notice: Undefined index: permissions in session_node_access_node_access() (line 279 of /sites/all/modules/custom/anonymous_session_node_access/session_node_access.module).
Warning: in_array() expects parameter 2 to be array, null given in session_node_access_node_access() (line 279 of /sites/all/modules/custom/anonymous_session_node_access/session_node_access.module).

- It wasn't bad to add module.install file and delete settings variables in hook_uninstall. At least for now for easier of testing and for future to not collect unnecessary data in db.

I just wanted to chime in and say I feel for you gbyte. After seeing this process I'd never want to submit a new project. Now I haven't looked at your code but the process as a whole seems slower than it needs to be, dare I say it.. nitpicky. Why can't we have a dev release on a public project page from the outset with rules and guidance given along the way to a full release. I think the community is expecting too much here... from volunteers.

From my understanding this review process does the following:

1. Checks for duplicate functionality
2. Checks for broken code
3. Conforms code to Drupal coding standards

I think that 1 and 2 should be the only criteria before a project is allowed a dev release. Then 3 is pursued before a full release.

In this situation this module would have been released a year ago and likely be just as bug free as it is today, with the addition of actual real world users. Developers who needed this particular module and tested it in the wild would have found real world bugs and possibly put this module in a better place than it is today.

A small, useful module that works as intended!

+1

Review of the 7.x-1.x branch (commit 9dd1fc9):

• Remove "version" from the ./session_node_access.info file, it will be added by drupal.org packaging automatically.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. session_node_access_user_node_access(): $node can also be a string, so you should prepare for that case. See https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...
2. session_node_access_node_insert(): I think this is the wrong approach to use the node insert hook. Consider cases where nodes are created in the background that the current user should not have edit access to. I would implement a custom submit callback on node forms instead. That way you only grant access to nodes that have actually been created with a form submission.

But that are not critical application blockers, so ...

Thanks for your contribution, gbyte.co!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.