OpenTok TokShow

Hi tokbox,

Thank for your work.

• You don't have a install file. You should create on for cleanup your variable when the module will be uninstalled (with hook_uninstall() )
• You use Location() PHP function. You should use the drupal_goto function
• In your tokshow_admin_form.inc file at line 124 and 126 You should use t() function for given the ability to translate this strings

Automated review :

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.
• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (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.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/tokshow_admin_form.inc:
 +124: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
 +126: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.

Status Messages:
 Coder found 1 projects, 1 files, 2 normal warnings, 0 warnings were flagged to be ignored

FILE: ...-7-pareview/sites/all/modules/pareview_temp/test_candidate/tokshow.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 5 | ERROR | It's only necessary to declare files[] if they declare a class or
   |       | interface.
--------------------------------------------------------------------------------


FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/tokshow.module
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 1 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  2 | ERROR   | Missing file doc comment
  3 | ERROR   | Missing function doc comment
 31 | ERROR   | Missing function doc comment
 35 | WARNING | A comma should follow the last multiline array item. Found:
    |         | TRUE
 40 | ERROR   | Missing function doc comment
 41 | ERROR   | Space after opening parenthesis of function call prohibited
 41 | ERROR   | Space before closing parenthesis of function call prohibited
 45 | ERROR   | Missing function doc comment
--------------------------------------------------------------------------------


FILE: ...w/sites/all/modules/pareview_temp/test_candidate/tokshow_admin_form.inc
--------------------------------------------------------------------------------
FOUND 38 ERROR(S) AND 5 WARNING(S) AFFECTING 35 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | Missing file doc comment
   3 | ERROR   | Missing function doc comment
  13 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
  20 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
  27 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
  34 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
  41 | WARNING | A comma should follow the last multiline array item. Found:
     |         | TRUE
  47 | ERROR   | Whitespace found at end of line
  55 | ERROR   | Whitespace found at end of line
  63 | ERROR   | Whitespace found at end of line
  71 | ERROR   | Whitespace found at end of line
  85 | ERROR   | Missing function doc comment
  92 | ERROR   | Missing function doc comment
  93 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  94 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  95 | ERROR   | Inline control structures are not allowed
  95 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  96 | ERROR   | Inline control structures are not allowed
  96 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  97 | ERROR   | Inline control structures are not allowed
  97 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  98 | ERROR   | Inline control structures are not allowed
  98 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  99 | ERROR   | Inline control structures are not allowed
  99 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
 101 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 124 | ERROR   | Whitespace found at end of line
 125 | ERROR   | else must start on a new line
 130 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 136 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 140 | ERROR   | Space after opening parenthesis of function call prohibited
 140 | ERROR   | Space before closing parenthesis of function call prohibited
 146 | ERROR   | Space after opening parenthesis of function call prohibited
 146 | ERROR   | Space before closing parenthesis of function call prohibited
 147 | ERROR   | Whitespace found at end of line
 152 | ERROR   | Space after opening parenthesis of function call prohibited
 152 | ERROR   | Space before closing parenthesis of function call prohibited
 153 | ERROR   | Whitespace found at end of line
 160 | ERROR   | You must use "/**" style comments for a function comment
 162 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 165 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
 171 | ERROR   | Missing function doc comment
 181 | ERROR   | elseif must start on a new line
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Hi,

Manual review:

1/ In tokshow_show_admin() You have :
drupal_goto('Location: http://api.opentok.com/hl/tokshow/' . variable_get('tokshow_embed_id') . '/admin');

This should be :

drupal_goto('http://api.opentok.com/hl/tokshow/' . variable_get('tokshow_embed_id') . '/admin');

2/ In your install file you should remove the line :
debug("HOOK UNINSTALL");

Hi
Had a quick look. Here is what I found:

• If there is no other modules that will put themselves into 'tokbox' package, then you should remove it from .info
• You should use sentence case, not title case, for end-user strings, not required though but coder would love it.
• Take some timeout to implement hook_requirements so you don't get errors like this

The specified file temporary://fileTXSCQ2 could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
  Notice: Trying to get property of non-object in tokshow_config_form_submit() (line 182 of /Applications/MAMP/htdocs/score/trunk/drupal7/sites/all/modules/opentok_tokshow/tokshow_admin_form.inc).

I think that's because of "tokshow" directory doesn't exists

Thanks

Please do not assign your own applications, only the current reviewer should do this.

Note that drupal.org useraccounts are per-person, if you this account is used by multiple persons you have to register separately.

Review of the 7.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.

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/tokshow_admin_form.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 156 | ERROR | else must start on a new line
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

tokshow.module

1. /**
    * @file
    * Main module file.
    */
   

   This should be a more descriptive text explaining what the module does in a few words.

2. The result from file_load() in tokshow_show_fan() should be checked.

tokshow_admin_form.inc

1. preg_match('/[^a-z0-9 ]/i', $artist_name)
   

   I think there are ^ and $ tokens as well quantifiers missing in this RegEx.? I think this only checks whether the first character in the string "$artist_name" is alphanumeric or a space character. I suggest something like this:

   preg_match('/^[^a-z0-9 ]+$/i', $artist_name)
   

2.   if ($fid = variable_get("tokshow_before_image_fid", FALSE)) {
       $file = file_load($fid);
   

   Again the result from file_load should be checked.

forgot to change status.

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