[D7] Anonymous publishing

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

Tested both with a clean D7 installation and with a more complex D7 distribution. Works fine, neat work!
Well documented and nice interface options that fulfill needs for many different use cases.

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

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

  FILE: /home/klausi/pareview_temp/anonymous_publishing.css
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   2 | ERROR | [x] Expected 1 space after colon in style definition; 0 found
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

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. anonymous_publishing_cron(): $cladh is a bad variable name. What does it mean? Use more descriptive words and avoid abreviations for variable names.
2. anonymous_publishing_cron(): do not run db_delete() queries directly against the node table! You must use node_delete_multiple() or node_delete() so that the proper API functions are invoked (deleting the fields in other tables etc.). Same for comments.
3. anonymous_publishing_uninstall(): why do you need global $conf? Why do you unset it in the end?
4. anonymous_publishing_uninstall(): do not run db_delete() queries against the variable table, use variable_del() instead.
5. "$verifymsg . ' ' . t('and the content is approved by by an administrator.')": do not concatenate translatable strings, use placeholders with t() instead.
6. "'#title' => 'Enter your e-mail address (not!):',": all user facing text must run through t() for translation. Please check all your strings.
7. "'#pre_render' => array('_honeyfield_pre_render'),": that function does not exist in your module, so you will get a PHP fatal error? What am I missing?
8. "md5(uniqid(mt_rand(), TRUE))": do not use md5() for anything security related. Use drupal_random_bytes() or a similar core function to generate secure random values and do something similar as in drupal_hmac_base64(). Or you might even want to use that function?
9. anonymous_publishing_cl_node_insert(): during a cron run the global user is anonymous, so if a node is created programmatically, then a mail will be sent out? If I use the Feeds module for example to import stuff during cron runs then the admin will get a mail for example? I think hook_node_insert() is wrong here, you want to react on actual form submissions (and nothing else), so you should add a submit handler to node creation forms.
10. _anonymous_publishing_cl_content_validate(): this screams SQL injection! Never concatenate variables into query strings such as $mail and $ip here, always use placeholders as you would do anywhere else.
11. "$ip = getenv("REMOTE_ADDR");": use ip_address() instead.
12. "'#default_value' => variable_get('anonymous_publishing_pet_options',": all variables defined by your module need to be removed in hook_uninstall(). Please check all your variables.
13. anonymous_publishing_realname table: user names can change, so instead of storing the user name you should store the user ID. This can be a potential security problem if users change their name and suddenly a comment is attributed to the wrong user.
14. anonymous_publishing_pet_realnames(): this looks vulnerable to XSS exploits. You are directly printing the node/comment title into HTML. If an attacker enters <script>alert('XSS');</script> as node title, then you will get a nasty javascript popup as admin. You need to sanitize user provided text before printing. Make sure to read https://drupal.org/node/28984 again. Same for the realname here, but that should go away anyway. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Ooops, I forgot that this is a full project already and usually we should not report security issues in public. But the module has no stable release yet, so reporting security issues in public is fine at this point according to our policy: https://www.drupal.org/security-advisory-policy

Hi,

In anonymous_publishing_admin_settings function, you are using user_load(0) function. Always people used to say that entity_load() functions have performace issue. so we can try to avoid it as much as possible. so i suggest you to use
drupal_anonymous_user() function which will return simple $user object and user_access() uses user roles to check the permission.

In anonymous_publishing_cl.dmin.inc file, line no 340, i feel you can still simply your code from

if (isset($form['ap_row'][$nid][$entry_key]['#type']) && 'hidden' == $form
['ap_row'][$nid][$entry_key]['#type']) {
// Hidden field.
}
else {
$row[] = drupal_render($form['ap_row'][$nid][$entry_key]);
}

to

if (isset($form['ap_row'][$nid][$entry_key]['#type']) && 'hidden' != $form
['ap_row'][$nid][$entry_key]['#type']) {
$row[] = drupal_render($form['ap_row'][$nid][$entry_key]);
}

Above are my suggestions..

Hi gisle,

This is a very nice module that you have made. I can see that this can become very popular and feature rich module in the future.

Here is my review:

1. There are places you check
   if ($user->uid) {
   or
   if ($user->uid == 0) {
2. To make it more readable, you can change that to:
   

   user_is_anonymous()
   user_is_logged_in()

3. Why do you make checks like this one:
   if (form['#form_id']) && $form_id == $form['#form_id']
   I think they are unnecessary.
4. Why do you unset $form here:
   

   function anonymous_publishing_pet_admin_privacy_submit($form, &$form_state) {
     unset($form);

5. in anonymous_publishing_cl_form_alter()
   You unnecessarily check
   if (0 == $user->uid) {
   a second time.
6. This is a bad practice in my opinion:
   drupal_substr($form_id, 0, 13) === 'comment_node_'
   It is not immediately obvious to me that the check is correct, and I have to count the characters.
   This is better:
   strpos($form_id, 'comment_node_') === 0

   This is a very strange string replace:
   $type = preg_replace('/^comment_node_.*_form$/', 'comment', $form_id);
   Why not just:
   $type = 'comment'

   Or if you need to be 100% certain of the above, you can do:

   if (preg_match('/^comment_node_.+_form$/', $form_id)) {
     $type = 'comment';

   Better yet, if you restructure the code a bit, you can have no checks at all, and just use hook_form_BASE_FORM_ID_alter():

   function anonymous_publishing_pet_node_form_alter(&$form, &$form_state) {
     anonymous_publishing_alter_comment_or_node_form($form, $form_state, $form['#bundle']);
   }
   
   function anonymous_publishing_pet_comment_form_alter(&$form, &$form_state) {
     anonymous_publishing_alter_comment_or_node_form($form, $form_state, 'comment');
   }
   
   function anonymous_publishing_pet_alter_comment_or_node_form(&$form, &$form_state, $type) {
     if (user_is_logged_in()) {
       if (anonymous_publishing_pet_content_type_allowed($type)) {
         _anonymous_publishing_pet_node_form($form);
         if ($type === 'comment') {
           // We disable the author field in the comment form.
           $form['author']['name']['#access'] = FALSE;
         }
       }
     }
   }
   

   Also for anonymous_publishing_cl:

   function anonymous_publishing_cl_node_form_alter(&$form, &$form_state) {
     anonymous_publishing_cl_alter_comment_or_node_form($form, $form_state, $form['#bundle'], 'node');
   }
   
   function anonymous_publishing_cl_comment_form_alter(&$form, &$form_state) {
     anonymous_publishing_cl_alter_comment_or_node_form($form, $form_state, 'comment', 'comment');
   }
   
   function anonymous_publishing_cl_alter_comment_or_node_form(&$form, &$form_state, $type, $entity_type) {
     if (user_is_anonymous()) {
       if (anonymous_publishing_cl_content_type_allowed($type)) {
         // ...
       }
     }
   }
   

7. In hook uninstall you have:
   cache_clear_all('variables', 'cache');
   Variable cache is in cache_bootsrap, and it is already cleared in variable_del()
8. The update hooks are in reverse order. It doesn't bother me particularly, but this is the first module I see with update hooks reversed like that.
9. The submodules could be split in separate directories. If this module is extended more, the files might become many, and people will get lost looking at them.

Frankly, I see no major problems, and most of the things I found are just suggestions that you may or may not agree with. They can be added in the module issue queue and dealt with later. The module is already good enough for promotion to full project, and you have helped the community by making multiple in-depth reviews with lots of follow-ups.

Oops, apparently this is already a full project. It's no wonder it looks so extensive and polished. I am not sure what is happening, and what status should be set.
Edit: OK, I read the issue description, and I still think I am correct in marking this RTBC.

Assigning to myself for my next full review.

As this project gets a lot of commits, this review is being done on

~/tmp/anonymous_publishing$ git branch -v
* 7.x-1.x 1dca49e #2287023 by gisle: Making PAReview happy.

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

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.

CSS and JS messages are false positives. #options warning is also a false positive.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

The IP column in the schema won't work with IPv6 addresses b/c of the column length.

Why aren't nid, cid, and uid indexed? hook_schema() also allows foreign key definitions.

I'm not seeing a hook_node_delete, hook_comment_delete, or hook_user_delete anywhere. Do you need to cleanup your tables when this happens?

I'm not seeing a hook_node_update anywhere. Do you need to cleanup your tables if someone manually assigns a user to a node?

Core doesn't do the cache_clear_all('variables', 'cache'); when using variable_del in any of its hook_uninstall instances. That $cid is also in cache_bootstrap $bin

Using #attached is preferred over drupal_add_css() whenever possible.

It's best to use the node constant NODE_PUBLISHED when checking $node->status, rather than a simple boolean test.

Where is $user->hostname coming from? I have never seen this on $user before on Drupal 7 sites. Check out the ip_address() function.

You could use element level validation on the email form elements.

_anonymous_publishing_cl_content_validate() has a db_query() that is building up a dynamic query; use db_select() instead.

anonymous_publishing_cl_admin_settings() uses a relative path instead of building up a proper $url with url(). Also elsewhere.

#default_value is plained already, see https://www.drupal.org/node/28984

anonymous_publishing_cl_admin_email() has some HTML in t(); best to avoid that if possible. Are the paragraph elements really needed (at least one was unbalanced, too)? Also elsewhere.

anonymous_publishing_cl_admin_moderation() has an untranslated string for '-deleted-';

_anonymous_publishing_send_email() uses $base_url. Can you just use the absolute option for url()?

It looks like all of the blockers from #15 are taken care of, nothing that I mention above is major.

Thanks for your contribution, gisle!

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.

I'm sorry, miscomment.