Stutzer [stutzer]

Hello, and thanks for applying for a CVS account.

As per instructions, the archive containing the module needs to be attached in a comment here; without that, this CVS application cannot continue.

• 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.

1. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

2. /**
    * Remove all variables crated by this module
    */
   function _handyalias_remove_variables() {
     $result = db_query("SELECT v.name FROM {variable} v WHERE v.name LIKE 'handyalias%';");
     while ( $item = db_fetch_object($result) ) variable_del($item->name);
   }
   

   Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.

3. require('handyalias.lib.inc');
   
   

   There is a Drupal function to use to load PHP files.
   It usually not a good idea to unconditionally include code contained in PHP files. If the files contain form handlers, theme functions, or menu callbacks, Drupal core code allows to third-party modules to define which files must be loaded in the implementations of hook_menu(), and hook_theme(); in these cases, Drupal loads the file only when necessary.

4. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how constant names should be written; how the code should be formatted.

5.    '#description' => t('Here you can set vocab alias, enable Handy Alias feature for terms of this vocabulary and select content types this vocabulary can be used for as an URL-alias constructor.'),
   
   

   It should be vocabulary.

6.     $form['identification']['alias_markup'] = array(
        '#type' => 'markup',
        '#value' =>
            '<div class="messages error">'
          .   t('Unable to use Handy alias functionality with this vocabulary as it have incompatible configuration. We don`t know how could it happend.<br/>You shouldn`t use "Tags", "Multiply terms" and "Multiply hierarchy" in vocabulary')
          . '</div>',
        '#weight' => 0,
       );
   
   

   Why isn't the code using drupal_set_message()?

7. function _handyalias_alias_validate(&$element, &$form_state) {
     if ( !empty($element['#value']) ) {
       if ( !_handyalias_valid_alias($element['#value']) )
         form_error($element, t('Invalid alias. Only latin characters, digits, underscore, dash, dot and plus are alowed'));
     }
   }
   
   

   It should be allowed.

8.       return db_result( db_query("SELECT `alias` FROM {handy_alias} WHERE `oid` = '%d' AND `type` = '%d'", $oid , $type) );
   
   

   The character ` is not used from all the database engines for which Drupal is compatible.

I will review the code tomorrow.

1.   if ( is_array($vocab_settings_all) && !empty($vocab_settings_all) ) {
       foreach ( $vocab_settings_all as $vocab_id => $ctypes ) {
         if ( is_array($ctypes) && !empty($ctypes) && $vid != $vocab_id ) {
           foreach ( $ctypes as $ctype ) {
             $reserved_ctypes[] = $ctype;
           }
         }
       }
     }
   
   

   should be

     if (is_array($vocab_settings_all) && !empty($vocab_settings_all)) {
       foreach ($vocab_settings_all as $vocab_id => $ctypes) {
         if (is_array($ctypes) && !empty($ctypes) && $vid != $vocab_id) {
           foreach ($ctypes as $ctype) {
             $reserved_ctypes[] = $ctype;
           }
         }
       }
     }
   
   

   As per coding standards, there should not be any spaces after an open parenthesis.

2.     drupal_set_message(
         t(
           'You just has changed vocabulary alias. To update existing aliases up to new alias use "<a href="!url">Bulk update</a>" page of pathauto module',
           array( '!url' => url('admin/build/path/update_bulk') )
         ),
         'warning'
       );
   
   

   The placeholder for URLs must start with @.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.