michaelpporter [michaelpporter]

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 using the proposed theme; for modules, it should include also a comparison with the existing solutions.

The module has obvious coding standard issues like indenting, see http://drupal.org/coding-standards please.

Security seems fine.

• 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. ; Information added by drupal.org packaging script on Jul-26-2010
   version = "6.x-1.1"
   core = "6.x"
   project = "simple_mobile_redirect"
   package = "Simple Mobile Redirect"
   datestamp = "1222796709"
   

   Those lines need to be removed from the .info file.

2. function simple_mobile_redirect_perm() {
     return array('Administer Simple Mobile Redirect configuration');
   }
   
   

   The permission doesn't follow the schema used for Drupal permissions. It should be administer simple mobile.

3. 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 constants should be written.

4. function _simple_mobile_redirect_clearcookie() {
   	setcookie("nomobi", "", time()-3600);
   	header('Location: /'); 
   }
   
   

   The code is not using the correct Drupal function that should be used to redirect users to a different location. Drupal can be installed on a sub-directory of the web root directory; redirecting the user to / would not redirect it to the home page of a Drupal-powered site.

5.   switch(true){ 
   
       case (preg_match('/ipad/i',$user_agent)); // find the word ipad in the user agent
         $is_mobile_browser = $ipad; 
         $platform = 'Apple iPad';
         if(substr($ipad,0,4)=='http'){ // Is there a URL to redirect iPads to?
           $simplemobileredirect = $ipad; 
         } 
       break; // break out and skip the rest
   
   

   I would not use switch() when the code is not checking a variable (or the value returned from a function) against different values; apart that, there is a PHP error in the code.
   Comments should be placed in a different line than the code.

6.   drupal_load('module', 'simple_mobile_redirect');
     $variables = array_keys(simple_mobile_redirect_variables());
   
   

   The function being called doesn't exist.

1. /**
    * Implementation of hook_perm().
    */
   function simple_mobile_redirect_perm() {
     return array('administer simple mobile');
   }
   function _simple_mobile_redirect_clearcookie() {
     setcookie("nomobi", "", time()-3600);
     $simplemobileredirect = variable_get('simple_mobile_redirect_mobileredirect', '/');
     if ($simplemobileredirect == '/'){
       $simplemobileredirect = '';
     }
     drupal_goto($simplemobileredirect);
   }
   

   Leave an empty line between a function and the other.

2.   switch(true){ 
       // find the word ipad in the user agent
       case (preg_match('/ipad/i',$user_agent)); 
   
   

   See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how constants should be written, and how the code should be formatted.
   The syntax used for case (preg_match('/ipad/i',$user_agent)); is not the syntax suggested by the coding standards.
   The code should be written as

     switch (TRUE) { 
       // find the word ipad in the user agent
       case (preg_match('/ipad/i',$user_agent)): 
   
   

The code is not using spaces where the coding standards suggest to use it. That is done in almost all the code, and that makes it difficult to read. Point #2 of my previous comment is still applicable.