ClickDesk Live Help - Live Chat Support Module

You got some coding standart issues (http://ventral.org/pareview/httpgitdrupalorgsandboxhenry01363692git), you can use http://ventral.org/pareview and try to fix them.
If you got any questions on that, please ask!

At least this should be done as soon as possible:

• 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.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/clickdesk.install
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
3 | ERROR | Missing function doc comment
8 | ERROR | Missing function doc comment
8 | ERROR | Whitespace found at end of line
9 | ERROR | No space found after comma in function call
12 | ERROR | Whitespace found at end of line
15 | ERROR | Missing function doc comment
15 | ERROR | Whitespace found at end of line
17 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/clickdesk.module
--------------------------------------------------------------------------------
FOUND 16 ERROR(S) AND 2 WARNING(S) AFFECTING 15 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
2 | ERROR | "require_once" is a statement not a function; no parentheses
| | are required
18 | WARNING | Line exceeds 80 characters; contains 85 characters
23 | WARNING | Format should be * Implements hook_foo().
36 | ERROR | Expected 1 space between comma and "'clickdesk_widget_text'";
| | 0 found
47 | ERROR | Whitespace found at end of line
50 | ERROR | Missing function doc comment
55 | ERROR | Missing function doc comment
58 | ERROR | More than 2 empty lines are not allowed
59 | ERROR | More than 2 empty lines are not allowed
93 | ERROR | An operator statement must be followed by a single space
97 | ERROR | Concat operator must be surrounded by spaces
97 | ERROR | Concat operator must be surrounded by spaces
106 | ERROR | Whitespace found at end of line
113 | ERROR | More than 2 empty lines are not allowed
114 | ERROR | More than 2 empty lines are not allowed
115 | ERROR | More than 2 empty lines are not allowed
115 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/clickdesk_widget.inc
--------------------------------------------------------------------------------
FOUND 35 ERROR(S) AND 1 WARNING(S) AFFECTING 24 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
2 | ERROR | Missing file doc comment
3 | ERROR | Whitespace found at end of line
7 | ERROR | Missing function doc comment
17 | ERROR | More than 2 empty lines are not allowed
20 | ERROR | Whitespace found at end of line
21 | ERROR | Whitespace found at end of line
22 | ERROR | Missing function doc comment
23 | ERROR | Whitespace found at end of line
24 | ERROR | Whitespace found at end of line
25 | ERROR | Concat operator must be surrounded by spaces
25 | ERROR | Whitespace found at end of line
27 | ERROR | Whitespace found at end of line
29 | ERROR | An operator statement must be followed by a single space
29 | ERROR | There must be a single space before an operator statement
29 | ERROR | Concat operator must be surrounded by spaces
29 | ERROR | Concat operator must be surrounded by spaces
29 | ERROR | Concat operator must be surrounded by spaces
33 | ERROR | Variable "cdURL" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
33 | ERROR | There must be a single space before an operator statement
33 | ERROR | Concat operator must be surrounded by spaces
35 | ERROR | Expected "if (...) {\n"; found "if(...)\n {\n"
35 | ERROR | Line indented incorrectly; expected 2 spaces, found 3
37 | ERROR | No space found after comma in function call
37 | ERROR | Whitespace found at end of line
46 | ERROR | Whitespace found at end of line
55 | ERROR | Concat operator must be surrounded by spaces
55 | ERROR | Variable "cdURL" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
55 | ERROR | Concat operator must be surrounded by spaces
56 | WARNING | A comma should follow the last multiline array item. Found:
| | '
58 | ERROR | Whitespace found at end of line
62 | ERROR | Missing function doc comment
63 | ERROR | Opening brace should be on the same line as the declaration
68 | ERROR | Missing function doc comment
69 | ERROR | No space found after comma in function call
69 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
Remove "version" from the info file, it will be added by drupal.org packaging automatically.
Remove "project" from the info file, it will be added by drupal.org packaging automatically.
Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically.
Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting

You have some errors in clickdesk.install and clickdesk.module files found with PAReview.sh online

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/clickdesk.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
9 | ERROR | The open comment tag must be the only content on the line
16 | ERROR | Do not use t() or st() in installation phase hooks, use $t =
| | get_t() to retrieve the appropriate localization function name
21 | ERROR | The open comment tag must be the only content on the line
--------------------------------------------------------------------------------

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/clickdesk.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
8 | ERROR | The open comment tag must be the only content on the line
33 | ERROR | The open comment tag must be the only content on the line
--------------------------------------------------------------------------------

please don't set needs work on minor coding style issues, so in-depht reviews wont be blocked

sorry for the delay,
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Hi,
There are still some minor code style issues. See below:
Review of the 7.x-1.x branch:

• 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.

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/clickdesk.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  9 | ERROR | The open comment tag must be the only content on the line
 16 | ERROR | Do not use t() or st() in installation phase hooks, use $t =
    |       | get_t() to retrieve the appropriate localization function name
 21 | ERROR | The open comment tag must be the only content on the line
--------------------------------------------------------------------------------


FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/clickdesk.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  8 | ERROR | The open comment tag must be the only content on the line
 33 | ERROR | The open comment tag must be the only content on the line
--------------------------------------------------------------------------------

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

Looking into the code, after cloning your module, the folder name is clickdesk_live_help___live_chat_support_module, while the module name in the .info file is ClickDesk. The 2 should have the same name.

In the clickdesk_widget.inc file:
You are using this function

/**
 * _clickdesk_elements
 *
 * This function is returning a form element array
 *
 * @return mixed
 *   always return as false
 */
function _clickdesk_elements() {

   return FALSE;

}

Do you need it for something?

Function clickdesk_widget_form():

if (isset ($_GET['cdwidgetid'])) {

    variable_set('clickdesk_widget', $_GET['cdwidgetid']);

  }

  $form['clickdesk_textfield'] = array(
    '#type' => 'textfield',
    '#title' => filter_xss(t('Widget Id')),
    '#default_value' => variable_get('clickdesk_widget', ''),
    '#description' => filter_xss(t('Widget Id')),
  );

You filter against XSS the t("Widget Id") text, but not the $_GET['cdwidgetid'] which comes directly from GET. Just for info, a better idea to avoid using GET is to use the drupal menu system. Something like this:

  $items['node/%node'] = array(
    'title' => 'View',
    'page callback' => 'node_page_view',
    'page arguments' => array(1),
    'access callback' => 'node_access',
    'access arguments' => array('view', 1),
    'type' => MENU_CALLBACK,
  );

More info about this here: Drupal Menu System.

Function clickdesk_widget_form_submit():

variable_set('clickdesk_widget', filter_xss($_POST["clickdesk_textfield"]));

Not good. See Drupal Form API
The ideea behind drupal is to store in the database the raw value the user entered in the form. You filter it at display. Furthermore insted of POST you should use $form_state['values']

In clickdesk_widget.inc you are using filter_xss() for filtering plain text.
filter_xss() is usually used for filtering HTML text, for plain text you should use check_plain() function.
http://drupal.stackexchange.com/questions/10280/is-check-plain-enough

Drupal code sniffer found issues in the two branches. Please refer to the coding standards at http://drupal.org/node/318

See http://ventral.org/pareview/httpgitdrupalorgsandboxhenry01363692git and http://ventral.org/pareview/httpgitdrupalorgsandboxhenry01363692git-6x-1x

Few minor conding style issues are no valid reason for setting needs work.

Manual review:

on clickdesk_menu():

<?php
if (!user_access('administer')) {
 
     return FALSE;
 
  }
?>

is absolutely not necessary.

You should check your clickdesk_elements() as well. There is a method conception problem.

However, this is not a blocker to me, and I can RTBC you.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch:

git checkout 7.x-1.x
git branch -D master
git push origin :master

./clickdesk.install: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.
Please fix this, either by removing the byte order by hex-editors or by saving it with correct format settings.

/**
 * clickdesk_install
 * Install
 */

Correct would be:

/**
 * Implements hook_install().
 *
 * Install.
 */

----------------------

/**
 *  clickdesk_menu
 * @return mixed
 *   a menu list
 */

Correct would be:

/**
 * Implements hook_menu().
 *
 * @return mixed
 *   A menu list.
 */

- begin comments with capital letter
- only one space between * and short description
- there must be a empty line between function short and long description.

• It's not a best practice and not necessary to set variables on installation.
• The "name" in your .info should be humand readable - not the .module filename
• If there are no other modules using the package "clickdesk", choose another one which applies best and is used by similar modules
• In hook_menu():
  

    if (!user_access('administer')) {
  
      return FALSE;
  
    }
  

  This makes absolutely no sense. Menu configuration is cached and hook is NOT called everytime the site is requested.

  'access callback' => TRUE,
  If you really want to check access here then either use a realy access callback or use a defined permission (read hook_menu() documentation!)

• /**
   * clickdesk_elements
   * @return true/false
   *   checking if access by admin or not
   */
  function clickdesk_elements() {
  
    return FALSE;
  
  }

  your function description definitely not tells that this function allways returns FALSE. What is this good for??

• t(' clickdesk'); - make sure there are no spaces in the beginning or ending of t()
• t(''); - t() is for translatable strings - why do you want to translate "" ?

/**
 * @file
 * @category clickdesk
 * @package  clickdesk
 * @author
 * @license
 * @link
 */

so many @'s and nothing to say ? - if you don't use it - remove it

function clickdesk_widget_form() {

  if (!user_access('administer')) {

    return FALSE;

  }

Allways check access in menu callback!

check_plain(t('Widget Id')),
It makes absolutely no sense to check_plain() static text, which does not contain any user input. Ignore automated reports here - this is a false positive....

Here's much more to say and this code is far away from beeing ready to be published.
For me this looks like you rarely have a clue how to create a secure and correct drupal module and I'd highly recommend you to buy a good drupal development book and start from the beginning.

My code-smell-o-meter is alarming, setting needs work, sorry!

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