rcastera [rcastera]

Thanks for applying!

However, how is this module different from already existing community projects? For examples, see

http://drupal.org/project/modules?text=campaign
http://drupal.org/project/modules?text=tracking%20code
...

1. version = 1.00
   shouldn't be contained in the .info file.

2. /**
    * @package		Analytics
    * @uses        Install the schema for origin tracking
    * @link		http://www.sankyinc.com/
    * @author		Richard Castera http://www.richardcastera.com
    * @date		06/08/2010
    * @copyright 	Sanky Communications 2010  Copyright.
    * @version		1.00
    * @access      Public
    * @license     GPL
    **/
   

   We don't do such file headers in Drupal. See http://drupal.org/node/1354 for Drupal's Doxygen and documentation standards.

   @package is defined via .info file already.
   @uses is completely used wrong here.
   @link also wrongly used here.
   @version and @date are needless, auto-generated and injected into .info file already.
   @license is superfluous, all code hosted and distributed on drupal.org is GPLv2.
   @access wrongly used here.
   We don't do @copyright and @author in Drupal.

3. /**
    * @uses        Implementation of hook_install()
    * @param	    None. 
    * @return      None.
    * @comments    Installs the schema for campaign_origins.
    **/ 
   

   Also wrong function headers. @uses and @comments are bogus. @param and @return is not documented for module hook implementations. Also note that your multi-line comment syntax (at the end) is bogus. More details in above mentioned documentation standards handbook page.

4. function campaign_origins_install() {
     variable_set('campaign_origin_expiration', 7);
   

   You need to decide whether your namespace is campaign_origins (plural) or campaign_origin (singular). As of now, most of the module is plural. That means there shouldn't be a variable (or anything else) in the singular namespace.

   Note that you should not set variables in hook_install(). Your code should use default values for not configured variables instead.

5. You should run this module through Coder module, http://drupal.org/project/coder

6. /** Include dependent files. **/
   if(file_exists(drupal_get_path('module', 'campaign_origins') . '/class.campaign_origins.inc')): 
     require('class.campaign_origins.inc');
   endif;
   

   There should not be code executed in the global scope. drupal_get_path() might not be available yet. You can move this into hook_init(). Make sure you use http://api.drupal.org/api/function/module_load_include/6 instead.

7. function campaign_origins_init() {	
     if(class_exists('CampaignOrigins')){
       $tracking = new CampaignOrigins();
       $tracking->setExpiration(variable_get('campaign_origin_expiration', 7));
       $tracking->track();
       unset($tracking);
     }
   }
   

   Looking at this and looking at the class, implementing this entire functionality in OOP is 99% overhead. The entire module could be stripped down to a few lines of code by removing all of the needless overhead.

8. campaign_origins_add_origin() is not invoked from anywhere. I'm not sure how this module is supposed to work.

9. function campaign_origins_admin_settings_form() {
     $form['campaign_origins_settings'] = array(
       '#type'=>'fieldset',
       '#title'=>t('Enter a number representing the number of days the cookie will expire. Example: 7')
     );
   

   You should only use fieldsets where it's technically necessary to group form elements.

10.   $form['sanky_twitter_credentials']['submit'] = array(
        '#type'=>'submit',
        '#value'=>t('Submit')
      );
    

    Strange first key here.

11.   if(($form_state['values']['cookie_days'] == '') || (!is_numeric($form_state['values']['cookie_days']))) {
    

    Not sure whether this sufficiently validates that the entered value is an integer.

12. function campaign_origins_admin_settings_form_submit($form, &$form_state) {
      variable_set('campaign_origin_expiration', $form_state['values']['cookie_days']);
      drupal_set_message(t('Cookie Expiration updated!'), 'status');
    }
    

    You wouldn't need that form submit handler, if you'd use http://api.drupal.org/api/function/system_settings_form/6 - there are many examples in core and contrib for how to use it.

Remember to change status, when you upload a new version of the code.

• 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. Menu descriptions and titles, as well as schema descriptions, are not passed to t(). Schema descriptions should then more descriptive than id.
2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

3.     if(arg(0) == 'node' && is_numeric(arg(1))) {
         $node = node_load(arg(1));
       }
   
   

   The code should probably use menu_get_object().

4. The file README should be renamed README.txt.

I am closing this application due to lack of replies.

Please re-apply for a CVS account; this report has been left without a reply for two months, and it has been declined. Changing the status of this report doesn't change the status of the application.