Kevin Quillen [gh0st25]

Hello, and thanks for applying for a CVS account.

May you describe a bit more the proposed module, and make a comparison with existing solutions?

Thank you for your reply.

I will review the code tomorrow morning.

• 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. /**
    * Implementation of hook_form
    */
   
   function user_alert_settings_form() {
   			
   	$form = array();
   
   	$form['user_alert_label'] = array(
   		'#title' => 'User alert label',
   		'#type' => 'textfield',
   		'#description' => t('The label next to every user alert message.'),
   		'#default_value' =>	variable_get('user_alert_label', 'User Alert'),
   	);
   	
   	$form['user_alert_timelimit'] = array(
   		'#title' => 'User alert timelimit',
   		'#type' => 'select',
   	    '#options' => array(
   			0 => 'Never', 
   			3600 => '1 hour', 
   			10800 => '3 hours', 
   			21600 => '6 hours',
   			32400 => '9 hours',
   			43200 => '12 hours',
   			86400 => '24 hours'
   			),
   		'#description' => t('You can automatically unpublish alerts after a given time period. "Never" will leave them up forever, until the user closes it.'),
   		'#default_value' =>	variable_get('user_alert_timelimit', 0),
   	);
   	
   	$form['submit']['save'] = array(
   		'#type' => 'submit',
   		'#name' => 'save',
   		'#value' => t('Save'),
   	);
   	
   	$form['#submit'][] = 'user_alert_settings_submit';
   
   	return $form;	
   }
   
   /**
    * Implementation of hook_submit
    */
   
   function user_alert_settings_submit(&$form, $form_state) {
   	
   	variable_set('user_alert_label', $form_state['values']['user_alert_label']);
   	variable_set('user_alert_timelimit', $form_state['values']['user_alert_timelimit']);
   	
   	drupal_set_message(t('User alert settings have been updated.'), 'status', FALSE);
   
   	return;
   }
   

   Neither the functions are implementations of a hook.
   The coding standards report there should not be any empty lines between the documentation comment and the function.
   The return statement in the last function is not required.

2. package = Other
   

   The default package is Other; there is no need to declare it.

3. Drupal variables are not initialized from hook_install().

4. 	$result = db_query('SELECT nid FROM {node} WHERE type = "%s"', 'user_alert');
   	
   	while($row = db_fetch_object($result)) {
   		node_delete($row->nid);
   	}
   

   None of the Drupal core modules removes nodes of the content type they define.

5. /**
    * Implementation of hook_perm()
    */
   
   function user_alert_perm() {
   	return array('administer user alert', 'create user alert', 'edit own user alert', 'edit any user alert', 'delete own user alert', 'delete any user alert');
   }
   

   AFAIK, the permissions to edit any node of a content type, to edit own nodes of a content type, to delete any nodes of a content type, and to delete own nodes of a content type are already defined from the node module. See the permissions defined in book_perm(), which doesn't define the permission edit own books, in example.

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

7.   $node = $vars['node'];
     $vars['alert_label'] = t(variable_get('user_alert_label', 'User Alert'));
     $vars['body'] = t($node->body);
   
   

   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.

I can see a reason why book.module, in example, doesn't delete any book pages, when it is removed.
With few exceptions, Drupal core code is usually the reference code to use to write your own code; the exceptions are, in example, the name to give to the functions, and the use of functions with a name starting 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.