JISCPM

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove the translations folder, translations are done on http://localize.drupal.org
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./jiscpm_zipfile.php
  

• ./jiscpmtask/jiscpmtask.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function jsgantt_check_directory($form_element) {
  

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

  ./documentation/CHANGELOG.txt:                                          ASCII c program text, with no line terminators
  documentation/_notes/dwsync.xml
  icons/_notes/dwsync.xml
  images/_notes/dwsync.xml
  jiscpmattribute/_notes/dwsync.xml
  jiscpmbca/_notes/dwsync.xml
  jiscpmissue/_notes/dwsync.xml
  jiscpmppr/_notes/dwsync.xml
  jiscpmproject/_notes/dwsync.xml
  jiscpmrfc/_notes/dwsync.xml
  jiscpmrisk/_notes/dwsync.xml
  jiscpmsr/_notes/dwsync.xml
  jiscpmtask/_notes/dwsync.xml
  jiscpmtask/jsgantt/_notes/dwsync.xml
  jiscpmteam/_notes/dwsync.xml
  translations/_notes/dwsync.xml
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  documentation/INSTALL.txt:1:/* $Id: $ */
  jiscpmtask/jiscpmtask.js:1:// $Id: $
  jiscpmtask/jsgantt/README.txt:1:// $Id: README.txt,v 1.1.2.2 2009/07/02 09:08:53 magnity Exp $
  jiscpmteam/jiscpmteam.info:1:; $Id: $
  translations/de.po:1:# $Id: de.po,v 1.1.2.1 2009/02/19 23:35:40 derhasi Exp $
  

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. Get a review bonus and we will come back to your application sooner.

@scarer
I'm sorry but just quoting issues found by automated tools does not make your review "manual".
A manual review, as required for review bonus, means that your actually analysing the code for logic, security or API missusages the automated review could not find.

Also it's really not necessary to post each of your reviews into a single comment, just edit your issue summary and add it there.

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

don't block in-depht reviews because of these minor issues found by automated tools

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

Your forgot to add your reviews to the issue summary as outlined in #1410826: [META] Review bonus.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). 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. Get a review bonus and we will come back to your application sooner.

manual review:

1. jiscpmattribute_list_form(): doc block is missing. Also "@function" does not exist. See http://drupal.org/node/1354#functions . Please check the docblock of all your functions and format them accordingly - use @param to describe function parameters.
2. jiscpmattribute_list_form(): do not use #value, use #default_value instead.
3. jiscpm_module.info: why is this file needed?
4. CSS files: there are quite a few, maybe add a dedicated css subfolder?
5. If you depend on other modules you should state that in the info file. See http://drupal.org/node/231036
6. "Implementation of hook_help" should be "Implements hook_help().". See http://drupal.org/node/1354#hookimpl . Also elsewhere.
7. jiscpm_form_alter(): if you are targeting just one specific form you should use hook_form_FORM_ID_alter().
8. jiscpm_menu(): the closing ")" of a multi line array should be on a separate line, see http://drupal.org/coding-standards#array
9. jiscpm_dashboard(): why do you need drupal_set_title()? You can specify the page title in hook_menu()?
10. theme_jiscpm_link(): you should not do any logic in a theme function, that's what theme preprocess hooks are for. A theme fucntion should only arrange markup.
11. jiscpm_dependent_select_process(): doc block is useless, it just repeats the function name.
12. jiscpm_icon_add(): I think you should use function_exists() before you call anything from dynamic variables.
13. "array_key_exists('q', $params)": "isset($params['q'])" is shorter and easier to read.
14. jiscpm_icon(): do not create image markup yourself, use theme('image', ...) instead.
15. jiscpm_list(): is that function called anywhere?
16. jiscpm_risks(): you need to use db_rewrite_sql() if you query the node table. See http://drupal.org/writing-secure-code
17. jiscpm_risks(): the output of that functions is vulnerable to XSS exploits. You need to sanitize user provided data such as node titles before printing them. Please carefully read http://drupal.org/node/28984 and check all your code for vulnerabilities.
18. jiscpm_risks(): do not create link markup yourself, use l() instead.
19. "drupal_set_message("There are no risks", 'status');": all user facing text must run through t() for translation.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Interesting project scarer, thanks for building it and submitting it back to the community.

First off, in response to your last question, it appears the short function doc comment issues reported by drupalcs have been resolved.

There are still quite a few issues with this module. Here is a list of some of the issues and some general recommendations.

• Module doesn't work with only the minimum components enabled as mentioned in the readme file (JISC PM Project module and the JISC PM attribute module). The site crashes with an error about missing table "jiscpmproject". The error is originating from function jiscpm_projects() which implies a potential circular dependency between modules. This function, and any related functionality, should probably in the jiscpmproject module.
• As an FYI, most people prefer to actually delete the master branch rather than just having a readme file directing people to the correct branch. See this page for information on setting the default branch: http://drupal.org/node/1659588 and this page about deleting the master branch: http://drupal.org/node/1127732
• Function documentation comments do not need the @function identifier. See http://drupal.org/node/1354#functions
• Hook implementations do not need to document function parameters in the function doc. See http://drupal.org/node/1354#functions
• There are a number of variables that are used before being defined. These undefined variables can cause inconsistent behaviors in the system and can fill a sites/servers logs with warning messages depending on the PHP version and settings.
  Here are a few examples:
  • jiscpm.module: line 161: $w is incremented without first being initialized.
  • jiscpm.module: line 539: $output is concatenated when the variable hasn't been initialized.
  • jiscpm.module: line 933: $results is concatenated when the variable hasn't been initialized.
  • jiscpm.module: line 1106: $results is concatenated when the variable hasn't been initialized.
  • jiscpm.module: line 1106: $results is concatenated when the variable hasn't been initialized.

  TIP: When testing locally, make sure your PHP error reporting level is set to "E_ALL | E_STRICT | E_DEPRECATED" That way Drupal will output these warnings as messages in the watchdog while you're testing. Make sure you're periodically checking the watchdog and Apache logs for errors/warnings from your module files.

• jiscpm.module: line 292: Unless this module will never handle dates later than 2037, you should set the max range value to be something like
  

  $max_year = ((int)date('Y')) + 25;
  $options = drupal_map_assoc(range(1970, $max_year));
  

• jiscpm.module: line 142: Function jiscpm_dashboard() doesn't appear to be used and calls a non-existent theme function.
• jiscpm.module: line 527: It's generally considered a bad practice to assign values to variables at the same time you're passing them to a function call. Plus you're never using the variable that you're assigning. Instead of doing this:
  

  $o = theme_image($img_src, $alt = $title, $title = $title);
  

  Do something like this:

  $output = theme('image', $img_src, $title, $title);
  

I stopped reviewing at this point.

Here are some general pointers to use when looking at your module:

• Make sure your installation instructions identify all requirements and installation steps.
• Make sure you're properly segmenting your code so that all related functionality is logically grouped together.
• For complex modules like this, I would make the top level module an API base module that contains only general purpose API calls used by the other modules. Then make the other modules specialize in specific aspects of the system such as providing the dashboard or handling Projects.
• If every module in the package isn't required, make sure you have your dependencies declared and test every combination of modules being enabled/disabled.

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