slanger [slanger]

did you forget to attach an archive of you module?

You might want to read http://drupal.org/node/539608
Especially the bit about "Here is a list of common motivation messages that, more often than not, get declined/rejected. If your motivation message resembles one of these then please, think before applying.

[....]
* I am planning to create modules / themes. Who applies for a CVS account should have already a module / theme. The purpose of a CVS application is to review code, and verify the degree of understanding about how a Drupal module is written; without a module there would not be a review, and there is not CVS application. For the same motivation reported in the previous point, future CVS applications reporting only this motivation will be declined. "

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

I offered up an environment for slanger to test this module and am pleased with the results. I hope this module is tested and approved soon as it will quickly become a must have for public libraries.

I would love to use this for our library. What do we need to do to get this approved as a module?

I am reviewing the module and should have some feedback for your shortly.

OK, I've reviewed your module and have some comments. First off let me say that the actual functionality of your module is quite nice and I look forward to using it. However there are a few problems that need to be addressed:

evanced_events_importer.module
You have two includes at the top of this file that are problematic:

• The './'. concatenation at the beginning is unnecessary.
• Because the files are being unconditionally included, you should use require_once() instead of include_once().
• Most importantly these includes shouldn't be here at all. Because they are no encapsulated in a function, they are called on every page load; an unnecessary performance hit. You should only require files when you actually need them.
  • In this case you don't need to require evanced_events_importer.admin.inc explicitly at all. The menu system will do it for you for paths that have a 'file' => '' declaration. Because of that you can remove that require and add an additional 'file' => 'evanced_events_importer.admin.inc' to your 'admin/settings/evanced-events-importer/xmlmapper' menu item.
  • evanced_events_importer.xml2node.inc is only needed when cron runs. That require should migrate into evanced_events_importer_cron.

evanced_events_importer.install
Your message in evanced_events_importer_install should be using st() instead of t().

Code Density
Your code has some density issues which make it hard to read/understand:

• You are too verbose in your comments and you have too many of them. For instance there really isn't a need to add a comment to the end of every brace to say what it is closing. You should only need a comment to document non-obvious code.
• You tend to do too much on a single line. For example the drupal_set_message() call in your .install file is incredibly long and contains no less than seven functions calls. Breaking those complex calls down will make your code easier to understand (and maintain).

Coding Standards
You are not completely in compliance with Drupal coding standards (http://drupal.org/coding-standards). In particular you don't have spacing around your periods when concatenating and your comments don't follow Doxygen formatting conventions (http://drupal.org/node/1354). I'd recommend setting up a Drupal 7 instance, downloading Coder (http://drupal.org/project/coder) and doing a code review. It will give you exact line numbers for most of the code problems (it won't identify your incorrect comment format however).

Improvements
These are not issues that necessarily need to be addressed immediately, but that would make your module easier to use:

• You might consider adding a second "evanced presets" module that defines a evanced events node type with all of the basic cck fields already configured and that sets the suggested exml <=> node mappings for that node type. That would allow people that just want to implement the basic features to do so more quickly.
• Having to manually create all the taxonomy terms in Drupal and keeping them synchronized with Evanced is a pain. It would be much nicer if users only needed to define the vocabularies and mappings and the module then treated any terms found in the exml as tags, adding them to their respective vocabulary as needed.

Looks good. I'd say it's time to give Slanger a CVS account and put this useful module out in the wild.

1. function evanced_events_importer_install() {
     // create the evanced_events_importer tables.
     drupal_install_schema('evanced_events_importer');
     drupal_install_schema('evanced_events_importer_xmlmapper');
     drupal_set_message(t('After '. l(t('enabling permissions'), 'admin/user/permissions') .' for this module, you should then ') . l(t('configure the Evanced Events Importer module'), 'admin/settings/evanced-events-importer') . t(' before you run cron.'), 'status');
   } // end-function evanced_events_importer_install
   
   The parameter of drupal_install_schema() is the name of the module, not the name of the database table; the same is true for drupal_uninstall_schema().
   

2. ; Information added by drupal.org packaging script on 2010-05-18
   core = "6.x"
   version = "6.x-1.0"
   project = "evanced_events_importer"
   datestamp = "1242609387"
   

   Those lines need to be removed, or they will confuse the update manager, once the packaging script adds its own lines.

3. The code requires a PHP5 extension, but the .info file doesn't declare this dependency. The module should then check if this dependency is present, if the extension is not always present in a PHP5 site.

4. // include configuration settings for this module.
   include_once('./'. drupal_get_path('module', 'evanced_events_importer') .'/evanced_events_importer.admin.inc');
   
   // include code that parses the EXML feed and either
   // adds, edits or deletes event nodes in drupal.
   include_once('./'. drupal_get_path('module', 'evanced_events_importer') .'/evanced_events_importer.xml2node.inc');
   

   Instead of using include_once(), the code should use the Drupal function thought for these cases; if the include files are included unconditionally, then it is better to merge the files into the module file.

5. /**
    * Implementation of hook_perm().
    */
   function evanced_events_importer_perm() {
     return array('configure Evanced Events Importer');
   } // end-function evanced_events_importer_perm
   

   The comment at the end of the function is not contemplated in the Drupal coding standards.

6. See http://drupal.org/coding-standards to understand how code should be written. In particular, see how the code should be formatted.

7.     if (trim($field_type) != '') { 
         // append the sql query.
         $sql .= ' WHERE node_field_type = \'%s\''; 
         // execute the sql query.
         $result = db_query($sql, trim($field_type));
       } 
   

         $sql .= ' WHERE node_field_type = \'%s\''; 
   

   Don't escape the string delimiter inside a SQL query; it is enough to change the string delimiter, in such cases ($sql .= " WHERE node_field_type = '%s'"; ).

8.     $form['instructions_additional']['suggested_settings'] = array(
         '#type' => 'markup',
         '#value' => t('<div><p>Typically, most event content types contain the same basic fields. With this in mind, the example below shows a common mapping configuration:</p></div>
   <h3>Mapped elements:</h3>
   <table class="sticky-enabled">
   <thead>
   <tr><th>EXML element</th> <th></th> <th>Content type field</th> </tr>
   </thead>
   <tbody>
   <tr class="odd"><td>title</td><td>=</td><td><i>Node title</i></td></tr>
   <tr class="even"><td>description</td><td>=</td><td><i>Node body</i></td></tr>
   <tr class="odd"><td>date1</td><td>=</td><td>Start & End Date</td></tr>
   <tr class="even"><td>time</td><td>=</td><td>Start Time</td></tr>
   <tr class="odd"><td>endtime</td><td>=</td><td>End Time</td></tr>
   <tr class="even"><td>id</td><td>=</td><td><i>Evanced ID field</i></td></tr>
   <tr class="odd"><td>library<br /> location</td><td>=</td><td>Taxonomy: <i>Location</i></td></tr>
   <tr class="even"><td>agegroup1<br /> agegroup2<br /> agegroup3</td><td>=</td><td>Taxonomy: <i>Age group</i></td></tr>
   <tr class="odd"><td>eventtype1<br /> eventtype2<br /> eventtype3</td><td>=</td><td>Taxonomy: <i>Event type</i></td></tr>
   </tbody>
   </table> '),
       );
   

   The code is passing too much HTML code to t(); the code needs also to be re-written to use a theme function.

9.     // determine if the 'organic groups' module is installed.  
       if (db_result(db_query("SHOW TABLES LIKE '%s'", trim($og_table_name)))) {
   

   The query is probably database-dependent; the correct way to verify if a module is installed is not that (try with module_exists()).
   I am not sure why the code is checking of OG is enabled, if it doesn't use any of the features of the module.

10.       // append the taxonomy data to the array. 
          $fields_array[$counter]['field_label'] = 'Taxonomy: '. $data->name;
          $fields_array[$counter]['field_name']  = 'taxonomy'. $data->vid;
          $fields_array[$counter]['field_name_alias'] = 'taxonomy'. $data->vid;
          $fields_array[$counter]['field_type']  = 'taxonomy';
    

    The strings should be probably translated.

11. The strings used in the user interface needs to be in sentence case (This is an example of sentence case.). The comments should be in sentence case too.
12. The JavaScript code should be rewritten to use Drupal behaviors.

13.     $sql = "SELECT  t.tid ". 
               "  FROM {term_data} t ". 
               " WHERE  t.vid = %d ". 
               "   AND  LOWER(t.name) = LOWER('%s') ". 
               " UNION ". 
               "SELECT  t.tid ". 
               "  FROM {term_synonym} t ". 
               " WHERE  LOWER(t.name) = LOWER('%s') "; 
    

    Is there any reason to not write the SQL query in a single string?

14.     // remove the last occurrence of a comma in the string.
        $xml_nodes_string = preg_replace('/,$/', '', trim($xml_nodes_string), 1);
    

    Instead of using a regular expression, it would be better to use rtrim().

15.       // remove the first occurrence of the comma in the string.
          $evanced_ids_string = preg_replace('/,/', '', $evanced_ids_string, 1);
    

    Same case as the previous, with the difference that the code should use a different function.

Since there doesn't appear to be any other feedback I think it's fair to flag this as reviewed again.

That is not exact: there have not been reviews since the code has been re-uploaded.

• The points reported in this review are not in order or 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.
• See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be indented.

1.   $form['instructions'] = array(
       '#type' => 'markup',
       '#value' => t('<div id="eei_instructions"> <p>This module imports an EXML feed from ' . l(t('Evanced Events'), 'http://evancedsolutions.com/our-solutions/events') . ', which it uses to create and update event nodes in Drupal. Once it is properly configured, event data will be imported every time cron is run.</p> <p>To learn more about the EXML feed, please contact your Evanced representative and/or refer to the Events Manual.</p> </div>'),
     );
   
   

   t() should use a placeholder; in that case, l() is never used together t(); see the exampled reported in the documentation for t() where similar code is marked as bad code.

2.     $form['instructions_div_close'] = array(
         '#type' => 'markup',
         '#value' => t('</div><!-- end-div eei_instructions -->'),
       );
   

   Strings like that are not passed to t(); an HTML tag cannot be translated, and the content of HTML comments is not translated either.

3.     $form['instructions_additional'] = array(
         '#type' => 'fieldset',
         '#title' => t('Suggested Settings'),
         '#collapsible' => TRUE,
         '#collapsed' => TRUE,
       );
   
       $form['instructions_additional']['suggested_settings'] = array(
         '#type' => 'markup',
         '#value' => t('<div><p>Typically, most event content types contain the same basic fields. With this in mind, the example below shows a common mapping configuration:</p></div> <p><b>Mapped elements:</b></p>'),
       );
   
       $form['instructions_additional']['suggested_settings_example'] = array(
         '#type' => 'markup',
         '#value' => _evanced_events_importer_xmlmapper_example_table(),
       );
   
   

   The first markup element should be used as #description of the fieldset element, and the second one should use a theme function.

4. In general, the form builder evanced_events_importer_admin_settings_xmlmapper() is using the markup element almost for each form element; in such case, a theme function should be used for the form builder.

5.     $fields_array[$counter]['field_label'] = t('Taxonomy: ' . $data->name);
       $fields_array[$counter]['field_name']  = t('taxonomy' . $data->vid);
       $fields_array[$counter]['field_name_alias'] = t('taxonomy' . $data->vid);
       $fields_array[$counter]['field_type']  = t('taxonomy');
   
   

   Use t()-placeholders.

6.     if (db_result(db_query("SHOW TABLES LIKE '%s'", "content_" . trim($fieldname_date)))) {
   
   

   The query is probably specific for a database engine, and it's not compatible with the others.

7.     $sql = "   SELECT  ct.nid, n.created, ct." . trim($fieldname_evanced_id) . "_value " .
              "     FROM  {content_type_" . trim($main_config_settings[0]->content_type_name) . "} ct, " .
                         $date_table_name . " {node} n " .
              "    WHERE  ct.nid  = n.nid " .
              "      " . $date_table_join .
              "      AND  n.type  = '%s' " .
              "      AND (ct." . trim($fieldname_evanced_id) . "_value != '' " .
              "      AND  ct." . trim($fieldname_evanced_id) . "_value IS NOT NULL) " .
              "      AND (ct." . trim($fieldname_evanced_id) . "_value NOT IN (%s)) " .
              "      AND (" . $date_table_alias . "." . trim($fieldname_date) . "_value BETWEEN '%s' AND '%s') ";
   
   

   The query can be built concatenating less literal strings.
   The SQL operator != is not standard, and should be replaced by <>.
   Is the variable $fieldname_evanced_id secure against SQL injection?

8. function evanced_events_importer_install() {
     // Create all the evanced_events_importer tables.
     drupal_install_schema('evanced_events_importer');
     $confirmation_msg = 'After ' . l(st('enabling permissions'), 'admin/user/permissions') .
                         ' for this module, you should then ' .
                         l(st('configure the Evanced Events Importer module'), 'admin/settings/evanced-events-importer') .
                         ' before you run cron.';
     drupal_set_message(st($confirmation_msg), 'status');
   }
   
   

   t() is available from hook_install(); it is used from php_install().

9. Avoid to escape the string delimiter inside a string, especially if the string is translated. If you need to use the string delimiter inside a string, then use the other delimiter to delimit the string.
10. The code is not using Drupal variables for its own settings. This is not what a Drupal module should do, especially when it implements a settings page.

1.   $confirmation_msg = 'After ' . l('enabling permissions', 'admin/user/permissions') .
                         ' for this module, you should then ' .
                         l('configure the Evanced Events Importer module', 'admin/settings/evanced-events-importer') .
                         ' before you run cron.';
     drupal_set_message(t($confirmation_msg), 'status');
   }
   

   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); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

2.   $sql = "SELECT * FROM {%s}";
   
   

   A string placeholder cannot be used for a database table.

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