Stan.Ezersky [ezersky]

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 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. datestamp 					= "1267655587"
   project 						= "Clearland"
   datestamp 					= "1283905033"
   project status url 	= "http://templates.ezersky.ru"
   version 						= "6.x-1.0-beta"
   

   Those lines needs to be removed.

2. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
3. Files available from third-party sites should not be committed in Drupal.org repository. Files licensed under a license different from GPL License (even if it is a compatible license) should not be committed in Drupal.org repositories.
4. The files README_en.txt, and README.txt contain the same text; one of the files should be deleted.

5. <?php
   // $Id: template.php
   
   

   The CVS ID is not correct.

6. if (is_null(theme_get_setting('rounded_corners'))) {  // <-- change this line
     global $theme_key;
   
   

   The settings should be prefixed by the theme name, to avoid conflict with settings used by Drupal, or third-party modules.

7.     $output .= (theme_get_setting('user_notverified_display') == 1) ? ' ('. t('not verified') .')' : '';
   
   

   Why aren't the parentheses included in the string to translate?

8. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted (in particular the control statements).

9.       drupal_set_message(t('Theme registry rebuild completed. <a href="!link">Turn off</a> this feature for production websites.', array('!link' => url('admin/build/themes/settings/' . $GLOBALS['theme']))), 'warning');
   
   

   The placeholder to use for URLs doesn't start with !.

10.     '#description'      => t('Meta tags aren\'t used much by search engines anymore, but the meta description is important -- this is what will be shown as the description of your link in search engine results.  NOTE: For more advanced meta tag functionality, check out the Meta Tags (aka. Node Words) module.  These theme settings do not work in conjunction with this module and will not appear if you have it enabled.'),
    
    

    Avoid to escape a string delimiter inside a string, especially when the string is translated. To use a string delimiter inside a string it's enough to delimit the string with the other delimiter.

      $string = "That's all, folks!";
    

11.   if (module_exists('nodewords') == FALSE) {
    

    The code can simply be written as

      if (!module_exists('nodewords')) {
    

12.     $form['clearland_set']['seo']['meta']['#description'] = 'NOTICE: You currently have the "nodewords" module installed and enabled, so the meta tag theme settings have been disabled to prevent conflicts.  If you later wish to re-enable the meta tag theme settings, you must first disable the "nodewords" module.';
    
    

    Strings used in the user interface should be translated.

13. <?php if ($block->subject): ?>
      <h2><?php print $block->subject ?></h2>
    <?php endif;?>
    

    Always use the semicolon at the end of statements that are not control-statements.

14. <?php
    // $Id: page.tpl.php,v 1.18.2.1 2009/04/30 00:13:31 goba Exp $
    ?>
    
    

    The CVS ID of files that has never been committed in Drupal.org CVS should be // $Id$.

15. The directory views is empty.

Let me see if I understood correctly; first you applied for a CVS account, and then you found somebody who created the project page for you. In fact the project page has been created on September 7, when this CVS application has been created on September 6. Therefore, this is not an application to become co-maintainer.

The code that has been committed in CVS doesn't follow what reported in my previous comment; in particular, files available from third-party sites have been committed in Drupal.org CVS.

@Stan.Ezersky: If you follow what is done in other CVS applications, you would notice that when somebody reports something that needs to be changed, a new archive containing the changed code is added in a new comment in the issue opened for the CVS application; that is what you should do too.

The project page should be removed, as your CVS application has not been approved, yet.

See my previous comment. The archive containing the new code needs to be uploaded in a comment here.

1. Some of the points I previously reported are still valid.

2.   // Modify elements of the search form
     $vars['form']['search_theme_form']['#title'] = t('');
   
   

   An empty string is an empty string in every languages.

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 the indentation to use for the code; how the constant names should be written.

4. 		$user_links['account'] = array('title' => t('Hello, !username', array('!username' => $user->name)), 'href' => 'user', 'html' => TRUE);
   
   

   The username must be passed through check_plain(); therefore, the used placeholder is not the correct one.

5.     if (theme_get_setting('meta_keywords') !== '') {
         $keywords = '<meta name="keywords" content="'. theme_get_setting('meta_keywords') .'" />';
         $vars['head'] .= $keywords ."\n";
       } 
   
   

   The content of HTML attributes need to be passed through check_plain() too.
   There is a Drupal function to use in such cases (drupal_attributes()).

6.   $node_author = t('!username', array('!username' => theme('username', $node)));
   
   

   There is no need to use t() in this case; why?

7.     if (drupal_strlen($object->name) > 20) {
         $name = drupal_substr($object->name, 0, 15) .'...';
       }
   

   There is a Drupal function that has that purpose.

8.   // First-page link display condition
     $show_first = ( $i > 1 ) ? true : false ;
   
   

   What is the shorter way to write that line?

1. As I reported in comment #8, point #2, files available from third-party sites should not be committed in Drupal.org repository. Files licensed under a license different from GPL License (even if it is a compatible license) should not be committed in Drupal.org repositories.
2. Verify that the line endings used for the files is the Unix one; there are some files that are using a different one.

3.   <div class="content"><?php print $block->content ?></div>
   

   There should be a semicolon at the end of such PHP commands.

4.         <a href="<?php print $next_url; ?>" class="page-next vtip" title="<?php print $next_title; ?>"><img src="<?php print base_path().path_to_theme()?>/images/spacer.gif" alt="<?php print $next_title; ?>" title="<?php print $next_title; ?>" /></a>
   

   There is a Drupal theme function to use for images.

5.   <div class="total">
       <?php print t('Total votes: @votes', array('@votes' => $votes)); ?>
     </div>
   

   The code should use the function format_plural().

6. Remember to change status when you upload a new version of the theme, or nobody will know there is something to review.

Hi Stan,

I think the main issue with timing right now is there is a very high ratio of CVS applications vs. people reviewing them. I'm a very recent volunteer and started looking through this.

I'm not changing the status of this myself right now because mainly I wanted to add the above note as I started to look through this - and maybe add a couple of notes.

Earlier, one of the comments Kiam made pertained to passing the user name through check_plain. My understanding is that you don't need to literally add "check_plain" to the user name, but you can change the placeholder. For variables passed through t(), I believe there are 3 types of placeholders - !, @, and % - and each does something different. I think where you previously had !username can just be changed to @username actually. My understanding is that using "@" would send the text through check_plain. Kiam can correct me if I'm wrong here or misunderstood his comment - he know far more than I do.

In regards to his comment in #22 about files available from 3rd parties, my assumption is this was in reference to the Vtip.js file which still exists. He can correct me if the comment referred to something else. I think the basic rule and issue here is, among other things, trying to keep the Drupal repository from being "cluttered" - because libraries and scripts available elsewhere don't need to be hosted here. And it helps to avoid other issues too, I think. From what I have seen, other developers have done one of two things:

1) Included a reference to a script hosted elsewhere - such as on Google.
2) Included instructions on downloading and installing additional files and scripts.

Sincerely,

Matt

P.S. Here's a link to documentation on t() - http://api.drupal.org/api/drupal/includes--common.inc/function/t/6

1. 	else {
   		$user_links['account'] = array('title' => t('Hello, !username', array('!username' => check_plain($user->name))), 'href' => 'user', 'html' => TRUE);
   

   As reported by WebNewCastle, calling check_plain() is not necessary if it is used the correct t()-placeholder.

2. The function clearland_preprocess_search_theme_form() gets only the first parameter, as it happens to template_preprocess_page().

3.   if (!module_exists('nodewords')) {
       if (theme_get_setting('clearland_meta_keywords') !== '') {
         $keywords = '<meta name="keywords" content="'. check_plain(theme_get_setting('clearland_meta_keywords')) .'" />';
         $vars['head'] .= $keywords ."\n";
       } 
       if (theme_get_setting('clearland_meta_description') !== '') {
         $keywords = '<meta name="description" content="'. check_plain(theme_get_setting('clearland_meta_description')) .'" />';
         $vars['head'] .= $keywords ."\n";
       } 
     }
   

   Drupal has the function drupal_attributes(), which could be called in such cases.

4.   if ( $li_previous ) $items[] = array( 'class' => 'pager-previous', 'data' => $li_previous );
     if ( $show_first && $li_first ) $items[] = array( 'class' => 'pager-first', 'data' => $li_first );
   

   The IF-statements are not formatted as reported in http://drupal.org/coding-standards.

5. drupal_set_message(t('Theme registry rebuild completed. <a href="@link">Turn off</a> this feature for production websites.', array('!link' => url('admin/build/themes/settings/' . $GLOBALS['theme']))), 'warning');
   

   The placeholder used in the string is not the same passed to the function in the array.

6. '#description'      => t("Meta tags aren't used much by search engines anymore, but the meta description is important — this is what will be shown as the description of your link in search engine results.  NOTE: For more advanced meta tag functionality, check out the Meta Tags (aka. Node Words) module.  These theme settings do not work in conjunction with this module and will not appear if you have it enabled."),
   

   Using Linux, I notice an extraneous character in the string passed to t().

7. <img src="/<?php print $base_path.$directory ?>/images/edit.png" alt="<?php print t('Edit') ?>" title="<?php print t('Edit') ?>" />
   

   The semicolon is missing at the end of the PHP instructions.

8. <div class="poll">
     <?php print $results; ?>
     <div class="total">
       <?php print t('Total votes: @votes', array('@votes' => $votes)); ?>
     </div>
     <?php if (!empty($cancel_form)): ?>
       <?php print $cancel_form; ?>
     <?php endif; ?>
   </div>
   

   <div class="poll">
     <?php print $results; ?>
     <div class="total">
       <?php print t('Total votes: @votes', array('@votes' => $votes)); ?>
     </div>
     <?php if (!empty($cancel_form)): ?>
       <?php print $cancel_form; ?>
     <?php endif; ?>
   </div>

   I am not sure why the theme is including templates that are equal to the templates used from Drupal core modules; if they are the same, it should be better to not include such templates.

Status: needs work » fixed

Status fixed means that issue is resolved (you've got cvs account). Use "needs review" instead.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue as Won't fix.
Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.