Advanced Forum Basic Subscriptions

There are a few things that can be done to improve this module after passing it through pareview.

Your results from running through it are here: http://ventral.org/pareview/httpgitdrupalorgsandboxbagz1465984git

The most striking things are to get rid of LICENSE.txt and to move from the master branch in git.

Almost all of the errors appear to be CSS colour errors. Whereby you've used capital letters but drupal coding standards require lower case.
eg:
223 | ERROR | CSS colours must be defined in lowercase; expected #3c78a7 but | | found #3C78A7

Another error that can be easily resolved seems to be that you have extra lines at the end of your modules. Just remove all blank lines from the end of files and it will fulfill requirements!

Does this actually require Advanced Forum? Since AF builds on core forum and subscriptions are fairly low level, removing AF as a requirement would open this up to use by anyone using core forum.

I haven't actually looked at the code; no time now. I like the idea, though, and I think it's good to have it as a separate module.

Ah, I see. Yeah, some do use just core forum but that's usually people who want to keep things very simple, anyway.

I'll be keeping an eye on this to see if it could work with Artesian as well.

Hi

I like your code and think it is well written. I have done a first manual review of it :

1) advanced_forum_basic_subscriptions.install
/**
51 * Implements hook_uninstall().
52 */
53 function advanced_forum_basic_subscriptions_uninstall() {
54 }

Remove the function if empty

2) advanced_forum_basic_subscriptions.module
182 drupal_set_title($title);

drupal_set_title($node->title); // XSS vulnerability
drupal_set_title(check_plain($node->title)); // Correct

3) advanced_forum_basic_subscriptions.module
288 $message = "A new comment, $comment->subject, has been added to topic $node->title.";
You don't use this variable so you can remove it.

4) advanced_forum_basic_subscriptions.module
In advanced_forum_basic_subscriptions_subscribe and advanced_forum_basic_subscriptions_unsubscribe
you don't really check the input (arg(2)...).

5) advanced_forum_basic_subscriptions.module
490 $subscription = array();
Do you need it ?

FILE: .../pareview_temp/test_candidate/advanced_forum_basic_subscriptions.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
265 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
301 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
--------------------------------------------------------------------------------

this code was copied verbatim from the advanced forum module, so perhaps the XSS vulnerability exists in that module as well?

Where? I've grepped the source of AF for both drupal_set_title and $node->title and don't see it.

Term title and node title are very different. I suppose if you let random users add forums, this could be an issue (though I didn't check if it is sanitized elsewhere so it may be moot). But node titles of forum topics are normally entered by (potentially) untrusted users so that is a much more serious issue.

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxbagz1465984git for the list of errors being generated.

don't block in-depht reviews because of these minor issues

I see no security issues here, but some variables are loaded but not used and could be affecting performance a bit.
You can see these by configuring your IDE code inspections.

I've installed the module and it gives me errors in advanced_forum_basic_subscriptions_preprocess_advanced_forum_topic_list_view() because my path are aliased by pathauto, the hack should be earlier, not in preprocess.

But I'm not sure if this count in project application review...

Scenario:

1. Enable Path module
2. Install Advanced Forum Basic Subscriptions module
3. Create Forum with aliased path
4. Bug occurs when viewing forum

A couple minor issues.

1. Documentation comments for non-hook functions are missing documentation on parameters and return values. See http://drupal.org/node/1354#functions.
   In particular, the following functions should have their parameter/return values documented:
   • advanced_forum_basic_subscriptions_page
   • advanced_forum_basic_subscriptions_makelink
   • advanced_forum_basic_subscriptions_bulk_mail (just add a @see entry pointing at drupal_mail)
   • advanced_forum_basic_subscriptions_subscribe (there is a typo in the description here too)
   • advanced_forum_basic_subscriptions_unsubscribe (there is a typo in the description here too)
2. In function advanced_forum_basic_subscriptions_page() the result of calls to advanced_forum_basic_subscriptions_subscribe() are assigned to a variable, but that variable is never used. I would recommend adding a check for $result == FALSE and display a message to the user if the subscribe/unsubscribe fails.
3. In function advanced_forum_basic_subscriptions_comment_insert(), you are calling node_load twice in a row with the same argument and not doing anything with the result after the first call. You should remove one of the node_load() calls.

Make sure you specify the default branch on your project page and remove the master branch from your git repo. See http://drupal.org/node/1659588.

I'd like to see your callbacks to subscribe/unsubscribe be separate from the rendering of the page. As it's currently implemented you leave open the possibility of a user bookmarking a subscribe/unsubscribe operation. So I would recommend making the following changes related to this:

• In your hook_menu() implementation, have separate callback functions for the subscribe/unsubscribe links
• In your callback functions for subscribe/unsubscribe, do a drupal_goto() back to the original page (using the destination argument) so that the page URL doesn't change.
• When building your action links for subscribe/unsubscribe use drupal_get_destination() to append a return URL to your action link.

Other than that, I think this is getting close to a release. Thanks for your contributions.

I manually checked the code and couldn't find any particular errors. You seem to know how to properly use the db_query() function with the improvements found in the Drupal 7 version and that's really good. Knowing the tools you have in your possession and how to use them, makes you a better programmer by its own.

But make sure you clear the errors listed in http://ventral.org/pareview/httpgitdrupalorgsandboxbagz1465984git-7x-1x before settings this to "needs review". They are many and maybe boring at the beginning, but fixing these errors in my own project was more and more fun until the whole list cleared. I hope your list will also be empty as soon as possible and you will also have fun!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

fixed git branch