lesgreen [lesgreen]

Hi lesgreen,

Thanks for the interest in contributing to the community!

In order to proceed reviewing this application, please add the modules as attachments so we can review them

You must submit a finished, working module or theme for review along with your application. Upload an archive containing the code to review in the issue automatically created for you by the application. Our review will check for adherence to security best practices, usage of Drupal APIs, and coding standards compliance.

Make sure to have read and understood these two pages:

• http://drupal.org/node/59
• http://drupal.org/node/539608

• This is a partial review.
• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. Files available from third-party sites should not be committed in Drupal.org repository. Files licensed under a license different from GPL License (even if it is a compatible license) should not be committed in Drupal.org repositories.
2. The version line needs to be removed from the .info file.

3. 	if ($path == 'admin/help#gpimagerotate') {
   		$txt = 'This module uses th jQuery imBannerRotate plugin to rotate images - random, carousel, and portfolio.';
   		return '<p>' . t($txt) . '</p>';
   	}
   

   The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable), or t(function()); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

4. function gpimagerotate_perm() {
   	return array(
   		'create gpimagerotate node',
   		'edit gpimagerotate node',
   		'delete gpimagerotate node',
   	);
   }
   
   

   A module implementing a content type should also implement the permissions edit own <content type>, and delete own <content type>.

5. /**
    * Implementation of hook_access().
    */
   function gpimagerotate_access($op, $node, $account) {
   	switch ($op) {
   		case 'create':
   			return user_access('create gpimagerotate node', $account);
   		case 'edit':
   			return user_access('edit gpimagerotate node', $account);
   		case 'delete':
   			return user_access('delete gpimagerotate node', $account);	 		
   	}
   }
   
   

   Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

   As reported in Documenting hook implementations:

   If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

   /**
    * Implements hook_help().
    */
   function blog_help($section) {
     // ...
   }
   

   This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

   In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

   /**
    * Implements hook_form_FORM_ID_alter() for node_type_form().
    */
   function mymodule_form_node_type_form_alter(&$form, &$form_state) {
     // ...
   }
   

   This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

6. 	$items['admin/content/gpimagerotate/add'] = array(
   	    'title' => 'Add',
   	    'page callback' => 'drupal_get_form',
   	    'page arguments' => array('gpimagerotate_admin_edit', 3),
   	    'access arguments' => array('add gpImageRotate images to your site'),
   	    'type' => MENU_LOCAL_TASK,
       	'parent' => 'admin/content/gpimagerotate',
   	    'file' => 'gpimagerotate.admin.inc',
   	);
   
   

   The module doesn't define the permission add gpImageRotate images to your site.
   See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted (and the indentation to use for the code).

7. 	$items = array();
   	$result = db_query(
   		'SELECT image_title, image_desc, image_path, image_link_url, image_link_target from {gpimagerotate}'
   	);
   
   

   Reserved SQL keywords are written in uppercase.

8. The presence of commented out code make me think the module is not complete; one of the requirements is that the module should be complete, in order to be used for a CVS application.
9. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.

As far as the question about including jquery.BannerRotater.js (or any other file), there are only these options:

1. Is it already licensed under the GPL? If so, you can up it to drupal.org's CVS no problem.
2. Is it licensed under a different license than GPL? You can't upload the file. You should point users to where they need to grab it and where to place it in their site's directory. A lot of module's require this extra step because of this. I suggest if this is the case, to include a check for this file, and if it's not there, inform the user appropriately, usability++ and it will save you tons of questions and hassle. Ask the author if they would mind licensing under the GPL, a lot of times they won't mind.
3. You have no idea what license is it in, or not licensed yet? Contact the author, let them know that you are creating a Drupal module integrating their great plugin, and ask them if they would mind if it got licensed under the GPL that Drupal is under. Most of the time they won't mind, and then with permission from the author you can upload it to drupal.org's CVS, and therefore license it under the GPL with the rest of the code.

This is basically the way to go about it, contacting the author usually helps, as people who share their work freely are usually happy to find out someone is integrating their work into Drupal, and like the GPL also.

If you can't get through this, providing easy instructions on how to get it, and considering in your code the very real possibility of the user not even reading the instructions is also a good idea.

Further info on licensing can be found here: http://drupal.org/licensing/faq

Are you referring to the use of "Implements" as opposed to "Implementation of"? If so, I looked at some other modules (Webform, Panels, CCK) and the use of "Implementation of" is also used.

Old modules use what the coding standards suggested at the time the project was created; new modules need to follow what the updated Drupal coding standards.

About the files available on third-party sites, the general rule is that files available from third-party sites should not be committed in Drupal.org repository; files licensed under a license different from GPL License (even if it is a compatible license) should not be committed in Drupal.org repositories.