[D7] Webex Xml Api

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

1) Add sandbox project link in Issue summary.

2) Found below error when I installed your module.

Notice: Undefined index: #node in webex_form_alter() (line 131 of D:\xampp\htdocs\responsivewebsolutions\sites\all\modules\webex_xml_api\webex.module).
Notice: Trying to get property of non-object in webex_form_alter() (line 131 of D:\xampp\htdocs\responsivewebsolutions\sites\all\modules\webex_xml_api\webex.module).

3) configure link is missing in *.info file
You should add configure = admin/config/user-interface/webexapi

4) Incorrect hook comments throughout the file. Change it from line no. 9, 20, 127. Correct hook comment is: Implements hook_HOOK().

Add Attendees button showing on all forms.

Need to address these issues first.

Manual Review

1. In D7 a module no longer should explicitly install or uninstall its database schema in hook_install() or hook_uninstall(). Reference

/**
 * Implements hook_install().
 */
function webex_report_install() {
  drupal_install_schema('webex_meeting');
}

/**
 * Implements hook_uninstall().
 */
function webex_report_uninstall() {
  drupal_uninstall_schema('webex_meeting');
}

2. It is also good to delete varables on uninstall

function webex_report_uninstall() {
  drupal_uninstall_schema('webex_api_url'); etc
}

3. Instead of using db select in your below funtion, you can use built in funtion to get the content types node_type_get_types()

/**
 * WebEx Xml Api: Listing the Content Type in the configuration page.
 */
function webex_getcontenttype() {
  $query = db_select('node_type', 'nt');
  $query->fields('nt', array('name', 'type'));
  $result = $query->execute();
  $options = array();
  while ($record = $result->fetchAssoc()) {
    $options[$record['type']] = $record['name'];
  }
  return $options;
}

Hi,

i)Add success message using hook_install () when your module has installed.

Removing review bonus tag (also, links on reviews from the issue summary), you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Assigning to myself for next review, which will hopefully by EOD today.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. One minor issue, need to be fix.

Review of the 7.x-1.x branch (commit 779f7d9):

FILE: /var/www/drupal-7-pareview/pareview_temp/webex.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
140 | WARNING | Do not concatenate strings to translatable strings, they
| | should be part of the t() argument and you should use
| | placeholders
---------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follow the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meet the security requirements.
From security prospective, It seems usages of user inputs in curl request without any sanitization not a issue blocker as you are not rendering it somewhere.

Coding style & Drupal API usage

1. (+) webex_install(): Why do you call check_plain() here, its just a static text? Remove it.
2. You should also really have a hook_help() with some basic info about the module.
3. You should move your setting form to a separate admin.inc file that is useful to keep all administration stuff like menu callbacks and forms etc. It's also useful for performance as well because all the functions from .module files get loaded at every initialization.
4. webex_form_alter(): arg() is evil, and should almost always be avoided.
5. webex_api_admin_settings_form() : IMO, you should have email validation in form.
6. webex_makewebexcall(): cURL is not a better option. Use the Drupal API whenever you can. I would recommend you to use drupal_http_request() to hit the external URL.
7. "'#title' => t('Attendees') . ' ' . $i,": do not concatenate variables into translatable strings, use placeholder with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I am not seeing any blocking issues. Assigning to mpdonadio for a second look if he has time.

Automated Review

Review of the 7.x-1.x branch (commit dcab3ec):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

Manual Review

Normally would prefer all module dependencies, like date, to go in the .info instead of relying on dependency chaining.
You don't have too many variables, just have a bunch of variable_del().

webex_schema() should have foreign keys, eg for nid, and probably indexes for nid and other columns you may JOIN on.

webex_uninstall(), loops to delete variables are frowned upon, as you may accidentally delete another module's variables.

webex_permission(), the permission 'access administration menu' is way too generic. It should be something like 'administer webex' or similar.

webex_form_alter(), you should use the hook_form_FORM_ID_alter() method to specifically target the node forms. Your logic may end being fragile and running on other forms.

No obvious security problems. webex_api_call() used unsanitized user input, but it doesn't goto output, rather it is used in an XML webservice (so it is OK).

webex_makewebexcall(), why cURL and not drupal_http_request()? Comment needed.

(*) In webex_makewebexcall(), why

curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 0);
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, 1);

This is a security problem and is the blocking issue in this application. This code only makes sure the certificate is valid, and disabled the check that is it for the correct host. CURLOPT_SSL_VERIFYHOST should almost always be 2.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

manual review:

1. webex_form_alter(): why is the node edit form excluded here? Please add a comment.
2. webex_api_call(): you should access $form_state['values'] here to get the submitted values.
3. webex_api_call(): where do you validate/escape the title and email addresses etc? Looks like you will get invalid XML pretty quick if I use a title such as test</confName>? You should use some from of escaping to make sure that the XML is always valid.

But that are not critical application blockers ands since this has been RTBC before ...

Thanks for your contribution, vigneshpr87!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.