Webform2vTiger

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:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module:
   +180: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +186: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +277: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 2 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
  

• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  webform2vtiger_fields.inc:8://Available fields
  

• ./webform2vtiger_fields.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

     // 'assigned_user_id'=>'Assign to user ID', // does not work - defaults to standarduser
  

• ./webform2vtiger.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // $vtiger_url = strstr("http://", variable_get('webform2vtiger_vtigercrm_url', NULL)) ? variable_get('webform2vtiger_vtigercrm_url', NULL) : 'http://' . variable_get('webform2vtiger_vtigercrm_url', NULL);
    // webform names client forms with the pattern web_form_client_form_$nid, use this knowledge to
      // all leads coming from this form should be treated as new on the vtiger CRM side
      // if the form requests the full name from the user, we need to let the lead capture
      // Since vtiger CRM by default only looks for an Opt-out, we need to make sure that we
        // add it to the submitted fieldset for consistency with when the user adds an
      // use the redirect/confirmation data from the original web form to tell vtiger CRM where
      // to redirect the user this is the same logic that the client_form_submit function
      // create an absolute URL for the vtiger CRM lead capture page to send the user back to
        // Collapse the email info on the page assuming that it is going to be a vtiger CRM form //
        // module_load_include would not work since it uses require_once, not include_once
      // if those required fields are not mapped - lead will not be created in vTiger
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./webform2vtiger_fields.inc:14:  'lastname' => 'Last Name', // required or submit will fail
  ./webform2vtiger_fields.inc:18:  'company' => 'Company', // required or submit will fail
  ./webform2vtiger_fields.inc:21:  'website' => 'Website', // should have no http:// at the beginning
  ./webform2vtiger_fields.inc:22:   // 'assigned_user_id'=>'Assign to user ID', // does not work - defaults to standarduser
  ./webform2vtiger_fields.inc:34:  'cf_539' => 'ID', // custom field
  ./webform2vtiger_fields.inc:41:  // 'designation' => 'Designation', // not working
  ./webform2vtiger.module:50:  $combine .= '&moduleName=Leads'; // we adding new lead
  ./webform2vtiger.module:83:        if ($key == 'none' && is_array($value) && !empty($value)) { // none is for group
  ./webform2vtiger.module:197:      // Collapse the email info on the page assuming that it is going to be a vtiger CRM form //
  ./webform2vtiger.module:206:      $tfield = $form_state['post']['advanced']['form_key']; // mapped to vTiger element
  ./webform2vtiger.module:207:      $dfield = $form_state['post']['form_key']; // name of form element in drupal
  ./webform2vtiger.module:274:          if (isset($required_fields[$c['tfield']])) { // make them bold
  

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

  README.txt:1:/* $Id$ */
  README.txt:2:# $Id$
  webform2vtiger.info:1:; $Id$
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./README.txt ./webform2vtiger.info ./webform2vtiger.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module:
   +194: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 normal warnings, 0 warnings were flagged to be ignored
  

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

  
  FILE: ...9/sites/all/modules/pareview_temp/test_candidate/webform2vtiger.install
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    9 | ERROR | Missing function doc comment
   13 | ERROR | Missing function doc comment
   24 | ERROR | You must use "/**" style comments for a function comment
   25 | ERROR | Multi-line assignments must have the equal sign on the second
      |       | line
  --------------------------------------------------------------------------------
  
  
  FILE: ...09/sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module
  --------------------------------------------------------------------------------
  FOUND 32 ERROR(S) AND 4 WARNING(S) AFFECTING 36 LINE(S)
  --------------------------------------------------------------------------------
     9 | ERROR   | Missing function doc comment
    20 | ERROR   | You must use "/**" style comments for a function comment
    36 | ERROR   | An operator statement must be followed by a single space
    40 | WARNING | Line exceeds 80 characters; contains 102 characters
    41 | WARNING | Line exceeds 80 characters; contains 100 characters
    63 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
    67 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
    73 | ERROR   | Expected 1 space after comma in function call; 2 found
    78 | ERROR   | You must use "/**" style comments for a function comment
    83 | ERROR   | Inline control structures are not allowed
    85 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
    88 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
    92 | ERROR   | Line indented incorrectly; expected 6 spaces, found 12
    96 | ERROR   | Closing brace indented incorrectly; expected 2 spaces, found 4
   106 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
   118 | ERROR   | You must use "/**" style comments for a function comment
   120 | ERROR   | Inline control structures are not allowed
   123 | ERROR   | Inline control structures are not allowed
   125 | ERROR   | Inline control structures are not allowed
   132 | ERROR   | Missing function doc comment
   168 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
   175 | ERROR   | Closing brace indented incorrectly; expected 8 spaces, found 4
   183 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
   186 | ERROR   | Line indented incorrectly; expected 6 spaces, found 8
   190 | ERROR   | Closing brace indented incorrectly; expected 4 spaces, found 6
   191 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
   192 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
   193 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
   194 | ERROR   | Concat operator must be surrounded by spaces
   195 | ERROR   | Closing brace indented incorrectly; expected 6 spaces, found 4
   240 | ERROR   | "include_once" is a statement not a function; no parentheses
       |         | are required
   286 | ERROR   | Line indented incorrectly; expected 8 spaces, found 10
   291 | ERROR   | Line indented incorrectly; expected 8 spaces, found 10
   326 | WARNING | A comma should follow the last multiline array item. Found:
       |         | MENU_NORMAL_ITEM
   332 | ERROR   | Missing function doc comment
   361 | WARNING | A comma should follow the last multiline array item. Found:
       |         | TRUE
  --------------------------------------------------------------------------------
  
  
  FILE: ...ites/all/modules/pareview_temp/test_candidate/webform2vtiger_fields.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 5 WARNING(S) AFFECTING 5 LINE(S)
  --------------------------------------------------------------------------------
    5 | WARNING | Line exceeds 80 characters; contains 85 characters
   28 | WARNING | Line exceeds 80 characters; contains 281 characters
   29 | WARNING | Line exceeds 80 characters; contains 188 characters
   30 | WARNING | Line exceeds 80 characters; contains 104 characters
   46 | WARNING | A comma should follow the last multiline array item. Found: //
      |         | 'designation' => 'Designation', - not working
  
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./webform2vtiger.install ./webform2vtiger.info ./webform2vtiger.module ./README.txt ./webform2vtiger_fields.inc ./INSTALL.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Addition: Have you already dived into http://drupal.org/coding-standards yet? I.e. more than a glance? There's really much you can learn even if you thought you were familiar with Drupal coding and even if you are doing it for already a good while. I really recommend it, not just in order to follow some rules. This is just what I noticed on myself, of course.

Automated review:

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

  FILE: ...ites/all/modules/pareview_temp/test_candidate/webform2vtiger_fields.inc
  --------------------------------------------------------------------------------
  FOUND 18 ERROR(S) AND 2 WARNING(S) AFFECTING 12 LINE(S)
  --------------------------------------------------------------------------------
    9 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   13 | ERROR   | Inline comments must start with a capital letter
   13 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   15 | ERROR   | Inline comments must start with a capital letter
   15 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   20 | ERROR   | Inline comments must start with a capital letter
   20 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   24 | ERROR   | Inline comments must start with a capital letter
   24 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   27 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   27 | ERROR   | There must be no blank line following an inline comment
   29 | ERROR   | Inline comments must start with a capital letter
   32 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   36 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   39 | WARNING | Line exceeds 80 characters; contains 82 characters
   39 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   48 | ERROR   | Inline comments must start with a capital letter
   48 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
   56 | WARNING | A comma should follow the last multiline array item. Found: //
      |         | 'designation' => 'Designation'  not working
  
   56 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
      |         | question marks
  --------------------------------------------------------------------------------
  

Manual review:

• Recommend moving the content of INSTALL.txt to README.txt, breaking at 80 characters and removing INSTALL.txt. See http://drupal.org/node/447604 -- there's just not enough there to warrant an extra file.
• webform2vtiger_schema and webform2vtiger_form_alter: array formatting errors; always use 2 spaces per level. See http://drupal.org/node/318#array
• Globally, use an indent of 2 spaces, with no tabs; see http://drupal.org/coding-standards#indenting
• You might consider moving the webform2vtiger_fields.inc array to a persistent variable instead of using an include.

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

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

  
  FILE: ...-7/sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module
  --------------------------------------------------------------------------------
  FOUND 15 ERROR(S) AND 6 WARNING(S) AFFECTING 18 LINE(S)
  --------------------------------------------------------------------------------
    52 | ERROR   | The first index in a multi-value array must be on a new line
    90 | ERROR   | Expected 0 spaces before closing bracket; 1 found
   209 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
       |         | question marks
   239 | ERROR   | Comments may not appear after statements.
   290 | ERROR   | Inline comments must start with a capital letter
   290 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
       |         | question marks
   294 | ERROR   | Inline comments must start with a capital letter
   294 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
       |         | question marks
   310 | WARNING | The closing paranthesis of an array should not be preceded by
       |         | a white space
   333 | ERROR   | More than 2 empty lines are not allowed
   334 | ERROR   | More than 2 empty lines are not allowed
   335 | ERROR   | More than 2 empty lines are not allowed
   336 | ERROR   | More than 2 empty lines are not allowed
   437 | WARNING | Line exceeds 80 characters; contains 81 characters
   445 | ERROR   | An operator statement must be followed by a single space
   445 | ERROR   | There must be a single space before an operator statement
   488 | ERROR   | The first index in a multi-value array must be on a new line
   516 | WARNING | Line exceeds 80 characters; contains 91 characters
   548 | WARNING | Line exceeds 80 characters; contains 111 characters
   569 | WARNING | Line exceeds 80 characters; contains 100 characters
   582 | WARNING | Line exceeds 80 characters; contains 85 characters
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• webform2vtiger_submit_form(): bad variable name: $form_id looks like a string, but you are using it as array?
• Calls to watchdog(): use "@" place holders instead of "!" place holders to check_plain() to variables.
• "if (!empty($form_state['post'])) {": why do you access $form_state['post'] here? Why not $form_state['values']?
• "$message = t("Found and removed old maping to vtiger field '$tfield' in these webform fields:");": do not use the $tfield variable like this in t(), use place holders instead. Also don't concatenate $message like this, use another place holder for the concatenated dfields.
• "'Administer > Site configuration > Webform to vtiger CRM '": All user facing text should run through t() for translation.

Even if you are keeping the ! placeholder, it should probably still be sanitized...

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

• 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. Go and review some other project applications, so we can get back to yours sooner.

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

sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module:
 +292: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
 +456: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.

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

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/webform2vtiger.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 290 | ERROR | Space before closing parenthesis of function call prohibited
--------------------------------------------------------------------------------

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

I have taken a look at the code in both places and tend to agree with you--there isn't any security risk in the way it is done and would be comfortable saying that coding standards wise, things look good.

I've given this a manual review and I haven't seen any issues standing out. Moving this to RTBC.

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

manual review:

• "define('SERVICE_URL', '/modules/Webforms/post.php');" all constants must be prefixed with your module's name to avoid name collisions.
• webform2vtiger_submit_form(): You should sanitize the external result data before putting it into watchdog(), as it should be treated as untrusted input. If you need to preserve the HTML use filter_xss() or filter_xss_admin().
• 'Map field to vtiger CRM': all user facing text should run through t() for translation, please check all your strings.
• webform2vtiger_form_alter(): I don't understand what you are doing in there. You check for $form_state['post'] so you want to process submitted data, right? That should be done in a submit callback or a validation callback.
• webform2vtiger_fieldtype_check(): again, do not access raw post data, check $form_state['values'] instead.
• "'access arguments' => array('access administration pages'),": this permission is too generic, create your own.

don't RTBC your own issues ;-)

Fixed repo command.

Please don't post the output of automated review tools like pareview.sh inline in a comment, as it just clutters the issue. Better add it as .txt attachment instead.

manual review:

• "'#options' => variable_get('webform2vtiger_available_fields', array()),": #options on checkboxes is not sanitized by the form API, so this is vulnerable to XSS exploits. See http://drupal.org/node/28984
• webform2vtiger_vtigercrm_url_validate(): why do you check for the user permission here? This function is only called during validation of the form that is protected by the permissions anyway?
• your module file is quite long. I suggest to move page callbacks to a dedicated *.admin.inc or *.pages.inc file.
• webform2vtiger_form_alter(): If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as '#type' => 'value'.

Otherwise this looks nearly ready to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please leave the tag for statistical purposes.

Added new section - "Reviews of other projects"

You're getting there, Robertas! :)

1. You should delete the master branch on your repository. You can do this with the command git push origin :master. (Note the colon.)
2. In webform2vtiger.module:

    // Suffix the URL where data needs to be sent to in vtiger.
   define('WEBFORM2VTIGER_SERVICE_URL', '/modules/Webforms/post.php');

   Constants should be documented using the docblock format, just like functions. Just change the comments to the /** */ style.

3. Any time you implement a hook, the docblock comment should read "Implements hook_whatever()." c.f. webform2vtiger.module:webform2vtiger_help(), webform2vtiger.module:webform2vtiger_form_alter(), webform2vtiger.install:*, etc.
4. The file docblock for the .install file should use this template:

    /**
   * @file
   * Install, update, and uninstall functions for the XXX module.
   */ 

5. In webform2vtiger.admin.inc:

    /**
    * Menu callback for admin/settings/webform2vtiger.
    * Settings for accessing vtiger.
    */
   function webform2vtiger_admin_settings() { 

   The short description and the expanded description in a docblock should have a blank line between them. See Documenting hook_menu() callbacks for an example. I think I saw this in a few other places.

6. In webform2vtiger.admin.inc:

        '#description' => '<p>' . t('Allows you to configure your connection to your vtiger CRM.<br/>
                            When providing your vtiger url, do not use a / at the end.') . '</p>', 

   Strings with line breaks and irregular whitespace in them are hard to override. You should either put them all on one line or run each line through t() separately.

7. You should be documenting your function parameters (@param) and return values (@return) when they have them. A read through Doxygen and comment formatting conventions would be helpful in general.
8. In webform2vtiger.admin.inc:

   form_error($element, t('Provided vtiger URL (%url) is not working - no site at this address.',
     array('%url' => $element['#value']))); 

   should be

   form_error($element, t('Provided vtiger URL (%url) is not working - no site at this address.', array(
     '%url' => $element['#value'],
   ))); 

   There should never be unbalanced indentation. Any structure that you indent you must eventually outdent to the same depth. And you should break the lines around opening and closing parentheses. There are a number of other places this needs to be fixed.

9. In webform2vtiger.admin.inc:

              if ($lead_field_array['mandatory']
               && isset($form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']])
               && $form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']] == '0') { 

   Control structure conditions should not be wrapped into multiple lines.

@travis: I could not find your #9 in the coding standards, of course it is recommended for long condition lines to split them up for better readability.

@klausi: It's under Line length and wrapping. "Conditions should not be wrapped into multiple lines." Here's an example of how this standard might be observed with the code above while maintaining readability:

Instead of this:

if ($lead_field_array['mandatory']
  && isset($form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']])
  && $form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']] == '0') {

this:

$name = (!empty($form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']]) = $form_state['values']['webform2vtiger_required_fields'][$lead_field_array['name']] : NULL;

if ($lead_field_array['mandatory'] && $name == '0') {

Sorry for the delay. Make sure to review more project applications and get a new review bonus to get this done faster.

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

• ./webform2vtiger.admin.inc: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.

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.

But otherwise this looks RTBC to me.

No objections for more than a week, so ...

Thanks for your contribution, Robertas! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

I am giving credits to the users who participated in this issue.