joomlerrostov [joomlerrostov]

Thanks for applying to a CVS account joomlerrostov:

You must submit a finished, working module or theme for review along with your application. Upload an archive containing the code to review in the issue automatically created for you by the application. Our review will check for adherence to security best practices, usage of Drupal APIs, and coding standards compliance.

Also, please make sure to have read and understood these two pages to make the process go faster:
http://drupal.org/node/59
http://drupal.org/node/539608

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. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see hoe Drupal variables, global variables, constants, and functions defined from the theme should be named; how the code should be formatted; how constant names should be written.

3. if (get_drupal_version() == 5) {
     require_once("drupal5_methods.php");
   }
   else {
     require_once("drupal6_methods.php");
   }
   

   Is the theme for Drupal 5?

4. function arb_light_blogger_img_assist_page($content, $attributes = NULL) {
     $title = drupal_get_title();
     $output = '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">'."\n";
     $output .= '<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">'."\n";
     $output .= "<head>\n";
     $output .= '<title>'. $title ."</title>\n";
     
     // Note on CSS files from Benjamin Shell:
     // Stylesheets are a problem with image assist. Image assist works great as a
     // TinyMCE plugin, so I want it to LOOK like a TinyMCE plugin. However, it's
     // not always a TinyMCE plugin, so then it should like a themed Drupal page.
     // Advanced users will be able to customize everything, even TinyMCE, so I'm
     // more concerned about everyone else. TinyMCE looks great out-of-the-box so I
     // want image assist to look great as well. My solution to this problem is as
     // follows:
     // If this image assist window was loaded from TinyMCE, then include the
     // TinyMCE popups_css file (configurable with the initialization string on the
     // page that loaded TinyMCE). Otherwise, load drupal.css and the theme's
     // styles. This still leaves out sites that allow users to use the TinyMCE
     // plugin AND the Add Image link (visibility of this link is now a setting).
     // However, on my site I turned off the text link since I use TinyMCE. I think
     // it would confuse users to have an Add Images link AND a button on the
     // TinyMCE toolbar.
     // 
     // Note that in both cases the img_assist.css file is loaded last. This
     // provides a way to make style changes to img_assist independently of how it
     // was loaded.
     $output .= drupal_get_html_head();
     $output .= drupal_get_js();
     $output .= "\n<script type=\"text/javascript\"><!-- \n";
     $output .= "  if (parent.tinyMCE) {\n";
     $output .= "    document.write('<link href=\"' + parent.tinyMCE.getParam(\"popups_css\") + '\" rel=\"stylesheet\" type=\"text/css\">');\n";
     $output .= "  } else {\n";
     foreach (drupal_add_css() as $media => $type) {
       $paths = array_merge($type['module'], $type['theme']);
       foreach (array_keys($paths) as $path) {
         // Don't import img_assist.css twice.
         if (!strstr($path, 'img_assist.css')) {
           $output .= "  document.write('<style type=\"text/css\" media=\"{$media}\">@import \"". base_path() . $path ."\";<\/style>');\n";
         }
       }
     }
     $output .= "  }\n";
     $output .= "--></script>\n";
     // Ensure that img_assist.js is imported last.
     $path = drupal_get_path('module', 'img_assist') .'/img_assist.css';
     $output .= "<style type=\"text/css\" media=\"all\">@import \"". base_path() . $path ."\";</style>\n";
     
     $output .= '<link rel="stylesheet" href="'.get_full_path_to_theme().'/style.css" type="text/css" />'."\n";
     $output .= '<!--[if IE 6]><link rel="stylesheet" href="'.get_full_path_to_theme().'/style.ie6.css" type="text/css" /><![endif]-->'."\n";
     $output .= '<!--[if IE 7]><link rel="stylesheet" href="'.get_full_path_to_theme().'/style.ie7.css" type="text/css" /><![endif]-->'."\n";
     
     $output .= "</head>\n";
     $output .= '<body'. drupal_attributes($attributes) .">\n";
     
     $output .= theme_status_messages();
     
     $output .= "\n";
     $output .= $content;
     $output .= "\n";
     $output .= '</body>';
     $output .= '</html>';
     return $output;
   }
   
   

   The function is outputting a way too much HTML.

5. The file script.js is not using jQuery.

6.   <?php echo $styles ?>
     <?php echo $scripts ?>
   
   

   There should be a semicolon at the end of the PHP statements.

This theme was generated using the program Artisteer.

In my experience, the program generates code way outside of drupal standards and best practices. In my opinion we should not allow such code to be committed, as it is unmaintainable, extremely hard to customize even with just CSS, etc.

Please read the following links as this is very important information about CVS applications.

• Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications.
• Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:
  

  This project is not covered by Drupal’s security advisory policy.