google_form

Hi, I just had a look at your module. You should use the coder module to see if there are still problems regarding coding standards. I've seen one error, a typo I guess.

Anyway, are you having problems with GIT? Because you said the module was D6 compatible, but the info file in master states it's 7.x
I tried installing your module, but upon enabling I'm getting this error: DatabaseSchemaObjectExistsException: Table <em class="placeholder">google_form</em> already exists. in DatabaseSchema->createTable() (line 630 of /var/www/d7/includes/database/schema.inc).
Upon clicking the new menu entry, I'm getting this error: Fatal error: Call to undefined function db_fetch_array() in /var/www/d7/sites/all/modules/google_form/google_form.module on line 106

Make sure you create branches! You should create a 6.x-1.x branch for the Drupal 6 version, and a 7.x-1.x branch for the Drupal 7 version. Then you just checkout the branch you are going to work on. Right now you just have a master branch, which essentially does nothing, i.e. you can't create releases from the master branch on Drupal.org.

Ok, I've had another look, your module is working (after struggling with simple_html_dom's permissions). I suggest you warn users in your README that the permissions need to be changed on the library.

Furthermore, it's very strange that coder doesn't show any warnings, because I've taken a more thorough look and have found numerous indenting problems. Here's an example:

function _google_form_get_link($target, $postdata = array()) {
  $html= 'test';
  $request_retry = 3;

  if (!empty ($postdata)) {
    $postOut = '';
    foreach ($postdata as $key => $item) {
      $key = str_replace('_', '.', $key);
      $postOut = $postOut . $key . '=' . $item . '&';
      }

      $method = 'POST';
      $headers = array('Content-Type' => 'application/x-www-form-urlencoded');
  }
    else {
        $method = 'GET';
        $headers = array();
        $postOut = NULL;
        }

$html = drupal_http_request($target, $headers, $method, $postOut, $request_retry);
return $html->data;

}

which should be

function _google_form_get_link($target, $postdata = array()) {
  $html= 'test';
  $request_retry = 3;

  if (!empty ($postdata)) {
    $postOut = '';
    foreach ($postdata as $key => $item) {
      $key = str_replace('_', '.', $key);
      $postOut = $postOut . $key . '=' . $item . '&';
    }

    $method = 'POST';
    $headers = array('Content-Type' => 'application/x-www-form-urlencoded');
  }
  else {
    $method = 'GET';
    $headers = array();
    $postOut = NULL;
  }

  $html = drupal_http_request($target, $headers, $method, $postOut, $request_retry);
  return $html->data;

}

Sorry that the coder module can't be of any help on this one (very strange, maybe you should file a bug report).
Furthermore, the @file is missing from your module and info files. To more easily resolve all the indenting problems, you might wanna use an editor like Emacs. Take a look here how to configure Emacs for Drupal's coding standards (note that that isn't perfect either, though).

Yes, I have checked with coder as well, same problem, not using mac.

/me is looking at your code again...

<?php
      return $blocks;
      break;
?>

You don't need to use break here, since you are already using return. The parser will therefore never arrive at break in the first place. Also, you can ALWAYS omit break when you have arrived at the last case, because break simply jumps to the end of the switch block, or whatever control structure it's in.

There are still a few indenting issues.

<?php
  $items['admin/build/google_forms'] = array(
    'title' => 'Google forms'
    //etc....
    );
?>

This should be:

<?php
  $items['admin/build/google_forms'] = array(
    'title' => 'Google forms',
    //etc....
  );
?>

Format arrays. Note that every line key => value, should end with a comma, including the last one (for practical reasons). There are a few more, I will list them later on.

<?php
    foreach ($postdata as $key => $item) {
      $key = str_replace('_', '.', $key);
      $postOut = $postOut . $key . '=' . $item . '&';
    }
?>

If it weren't for the str_replace() you could do this with drupal_query_string_encode(). Also, you ought to sanitize the POST data in $postdata = $_POST;. It might contain special characters.

!empty ($results)There shouldn't be a space in this .

Some of your functions don't have documentation (it generally doesn't need to be a lot):

<?php
/**
 * This function does many great things!
 */
function my_amazing_function() {
  // Cool stuff
}
?>

The following arrays also don't conform to Drupal's coding standards:

• The one in the .install file
• The array you return in the google_form_delete() function
• The form in google_form_settingsform()

Comments in you .info file should look like ; Insert comments here. It's in the .ini format. My apologies though, there doesn't need to be a @file block in your .info file. My mistake sorry.

In some files, you still have CVS $Id$ tags.

CSS code should be formatted according to these rules. It's mostly the same rules as PHP, with a few specific notes. For example, this:

div.block-google_form hr { display: none; }
div.block-google_form div.ss-footer-links { display: none; }

div.block-google_form div.errorbox-bad { border-right: 2px solid #E81818; border-bottom: 2px solid #E81818; padding: 0px 0px 5px 0px; }

should be:

div.block-google_form hr {
  display: none;
}
div.block-google_form div.ss-footer-links {
  display: none;
}

div.block-google_form div.errorbox-bad {
  border-right: 2px solid #E81818;
  border-bottom: 2px solid #E81818;
  padding: 0px 0px 5px 0px;
}

That's about it, I think once you've resolved these issues, there's probably not a lot left to do.

Just a quick reply, lots of indenting done correct and the css is good.
But have another look at the .install file and the google_formdelete return statement. The latter is strictly not an array, but a function call with an "array" of arguments. Function calls generally don't like a trailing comma (hence the error), the built-in array() function being the only exception I know of.

You are right that coder is giving a false positive on CVS tags.

Note that with commenting, I only mean a doxygen style comment directly above your functions, described here. You don't need to document every bit of code, a general description of your functions will suffice in most cases.

As for sanitizing post content, I made a wrong presumption I guess. I see it is simply being "reposted" to google, which is the only sane way to go about it, come to think of it :).
In other cases, depending on the situation, you could use check_plain, filter_xss, any of the other *filter_xss* functions, and maybe others that I don't know of yet.

I should apologize once more, I myself am quite new to code reviewing.

Like I said, input sanitizing not needed in your case. My mistake.

Good job on correcting the indenting issues. However, 3 issues still remain.

1. <?php
     return confirm_form($form,
     t('Are you sure you want to delete the `%title` google form association?', 
       array('%title' => $result['title'])
       ),
     'admin/build/google_forms',
     t('This action cannot be undone.'),
     t('Delete'),  t('Cancel')
     );
   ?>

   should be

   <?php
     return confirm_form(
       $form,
       t('Are you sure you want to delete the `%title` google form association?', array('%title' => $result['title'])),
       'admin/build/google_forms',
       t('This action cannot be undone.'),
       t('Delete'),
       t('Cancel')
     );
   ?>

   No trailing comma in this case, because it will lead to a syntax error.

2. Have another good look at the form you return in google_settings_form(). There is a single bracket that isn't indented enough. Otherwise it's good

3. <?php
   function google_form_menu() {
     $items['admin/build/google_forms'] = array(
       'title' => 'Google forms',
       // More attributes
       'file' => 'google_form.admin.inc'
     );
   
     // More menu items
   }
   ?>

   Don't forget the trailing comma for multi-line arrays.

Then there are two small new issues:

• <?php
    $form['gfid'] = array('#type' => 'value', '#value' => $result['gfid'],);
  ?>

  A trailing comma is only needed for multi-line arrays. In this case you can leave it out.

• <?php
  /**
   * This function parses the output of _google_form_get_link to either parse a
   * form and remove extra html (and the iframe) or parse the thank you page for
   * display in the block, this is the function that requires libraries API module
   * and the simple_html_dom library.
   */
  ?>

  That description is too long:

  <?php
  /**
   * Short description goes here. It shouldn't exceed a single line of 80 chars
   *
   * After a blank line, the description may continue here, further describing
   * what in the world this function exactly does.
   */
  function this_function_is_documented() {
  }
  ?>

  As for the other descriptions, that exceed 2 lines, I would try to just make the description a bit more concise.

Good job, yes 4 eyes generally see more than 2 :)
Looks like only this is remaining:

<?php
  return confirm_form($form,
    t('Are you sure you want to delete the `%title` google form association?', array('%title' => $result['title'])),
    'admin/build/google_forms',
    t('This action cannot be undone.'),
    t('Delete'),  t('Cancel')
  );
?>

There is no documentation specific to long function calls, but for consistency we should treat it as an array, except for the trailing comma. Therefore, every argument should be on it's own line, like so:

<?php
  return confirm_form(
    $form,
    t('Are you sure you want to delete the `%title` google form association?', array('%title' => $result['title'])),
    'admin/build/google_forms',
    t('This action cannot be undone.'),
    t('Delete'),
    t('Cancel')
  );
?>

But like I said, this isn't explicitly laid out in the documentation.

I justed went ahead to install the module again, and coder seems to be doing it's job a bit better now. Still strange that it bailed out like that. Anyway, now it reports trailing spaces in 3 places. You can have a look yourself and correct that. I trust you can fix this minor issue with the next commit, so I'll put this at RTBC.

I'll have a look on IRC to see if anyone with the proper access rights can approve and fix your application. If no result, I advise you to go on IRC #drupal-contribute to ask around for yourself.

You keep missing this one ;)

<?php
    t('Delete'),  t('Cancel') ?>

Should be:

<?php
    t('Delete'),
    t('Cancel')
?>

And there's this (reported as a trailing space):

<?php
 /**
 * Lists the google forms associated with the drupal site.
 */
?>

Should be:

<?php
/**
 * Lists the google forms associated with the drupal site.
 */
?>

Guess this can be marked fixed then! Good job and welcome to the developer community :)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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