gaspaio [gaspaio]

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

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site; for modules, it should include also a comparison with the existing solutions.

• 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. The version line needs to be removed from the .info file.
2. 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.

3. Drupal variables are not initialized in hook_install(), or hook_enable().

4.   /*
      * delete DB references to our block from the database
      */
     db_query("DELETE FROM {blocks} WHERE module='%s'",'simple_subscription');
   
   

   That is already done by Drupal core code, as far as I can see.

5.       variable_set('simple_subscription_config',array(
           'form_header'   =>  check_markup(trim($edit['simple_subscription_form_header'])),
           'form_footer'   =>  check_markup(trim($edit['simple_subscription_form_footer'])),     
           'input_label'   =>  check_plain(trim($edit['simple_subscription_input_label'])),
           'input_size'    =>  $edit['simple_subscription_input_size'],
           'input_content' =>  check_plain(trim($edit['simple_subscription_input_content'])),
           'submit_value'  =>  substr(check_plain(trim($edit['simple_subscription_submit_value'])),0,20),
           'success_message'=> check_markup($edit['simple_subscription_success_message']),
           'redirect_path' =>  check_url(trim($edit['simple_subscription_redirect_path'])),
           ));
   
   

   check_markup() should be eventually called when the content is output, not when it is saved in a Drupal variable.

6.       '#description'  =>  t('This text will be displayed before the form'
                               .' elements (leave empty for none). You may use Html tags.'),
   
   

   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.

7. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.
8. There are some typos in the strings used in the user interface (e.g. standart, carefull, adress)

9.   if ($path) {
       $path = drupal_get_normal_path($path);
       drupal_goto($path);      
     }
     
   
     return true;  
   
   

   A form submission handler doesn't use drupal_goto(), nor returns any values.

See also what I reported in comment #3.

None of the Drupal core modules that define a block remove the references to their blocks from the blocks database table.
See poll_uninstall(), book_uninstall(), and forum_uninstall().

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.