toby.batch [tobybatch]

You need to upload here the module you want to add in the CVS repository.

Why do we need to tag issues with "CVS module review" when we're in an issue queue specifically for CVS applications? Feels the same if I were to tag all issues in the XML sitemap issue queue with "XML sitemap"

It serves to distinguish a module review from a theme review; as AjK already used the tags you see, I just kept using the same tags.

It is possible to rename the tags in the taxonomy settings page, but who uses the tags must know that "module review", and "theme review" are now used.
"module review" is perfectly fine with me; maybe AjK had a reason to use "CVS module review". I will check if there is already a "module review" used for another reason, and I will report here.

I don't find any taxonomy term "module review"; I guess we could use that, and "theme review".

Ok cool. I was just wondering why. Didn't see that AjK was already using it. I'll use module review and theme review if I get to process any new applications. Thanks Kiam!

As AjK uses CVS module review, it is better to use all the same tag, otherwise there would be just confusion.

Ok; now we can return to handle this report. I forgot to change the status of this report. :-)

See the Drupal coding standards to understand how a module code should be written.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.

I reported the link to the Drupal coding standards on my previous comment.

If you want to verify if your code is compliants to the standards used by Drupal, there is http://drupal.org/project/coder. Bear in mind it is not able to tell you if you are using the correct name for the functions, in example.

Methods defined from an object that is defined from a PHP extension needs indeed to be called as they are defined from the class. false should be written as FALSE, anyway.

1. I am not sure of the usefulness of the template files, which could be replaces from a theme function, especially for their simple output.

2.     '#description' => t('The name used to identify this trac. Any name will do it\'s only used by humans adminstrating this module.'),
   

   Avoid escaping the quotes inside a translatable string.

3.   drupal_set_message(t('The server '. $server['name'] .' has been created.'));
   

   The first argument of t() needs to be a literal string, not a concatenation of strings, nor a constant.

4.       l(check_plain($server['url']), $server['url']),
   

   The text used for the link should be something more descriptive.

5. if (!DEFINED("_DEBUG")) {
     DEFINE("_DEBUG", FALSE);
   }
   

   Debug code should be removed.

6. function tracrss_perm() {
     return array('access tracrss', 'access administration pages', 'config own tracrss');
   }
   

   A module needs to declare only its own permissions; one of those permissions is already declared by a Drupal core module.

7.   $path  = substr($base_url, 0, strpos($base_url, "/query?"));
     $path  = str_replace(":", "_", $path);
     $path  = str_replace("/", "_", $path);
     $path  = htmlentities($path);
   

   The module should use Drupal Unicode functions.

8.   drupal_set_message(t("Cache update for $uid"));
   

   I have already reported about this kind of error; which one is it?

9.     // build the title
       // edit link
       $title = l(t('[edit]'), "tracrss/edit/$id");
       $title .= " ";
       // text of the link
       $title .= t
       (
         "Tickets for %userlist from trac server %tracname",
         array
         (
           '%userlist' => implode(",", $userlist),
           '%tracname' => $server['name'],
         )
       );
       $title .= " (". l($server['url'], $url) .")";
   

   The code is creating a translatable string by concatenating strings; it is possible to use a better way.

10.   global $user;
      $uid = $user->uid;
      $config = tracrss_userconfig_get($uid);
    

    It would better to write $config = tracrss_userconfig_get($user->uid).

I stumbled upon this issue by mistake. But I thought I'd clarify that KiamLaLuno most likely means you should use functions like drupal_substr that are multibyte safe.

1.       l(check_plain($server['url']), $server['url']),
   

   The first argument of l() is already passed to check_plain() by l(), as it is evident in the function code.

   function l($text, $path, $options = array()) {
     global $language;
   
     // Merge in defaults.
     $options += array(
         'attributes' => array(),
         'html' => FALSE,
       );
   
     // Append active class.
     if (($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) &&
         (empty($options['language']) || $options['language']->language == $language->language)) {
       if (isset($options['attributes']['class'])) {
         $options['attributes']['class'] .= ' active';
       }
       else {
         $options['attributes']['class'] = 'active';
       }
     }
   
     // Remove all HTML and PHP tags from a tooltip. For best performance, we act only
     // if a quick strpos() pre-check gave a suspicion (because strip_tags() is expensive).
     if (isset($options['attributes']['title']) && strpos($options['attributes']['title'], '<') !== FALSE) {
       $options['attributes']['title'] = strip_tags($options['attributes']['title']);
     }
   
     return '<a href="'. check_url(url($path, $options)) .'"'. drupal_attributes($options['attributes']) .'>'. ($options['html'] ? $text : check_plain($text)) .'</a>';
   }
   

2.     $msg = 'The server '. $server['name'] .' has been saved.';
       drupal_set_message(t($msg));
   

     $msg = 'The server '. $server['name'] .' has been created.';
     drupal_set_message(t($msg));
   

   The first argument of t() needs to be a literal string, not a variable; differently, the value passed to the function will not be translated.

3.     case $op == "list":
         // Generate listing of blocks from this module, for the admin/block page
   

   That is not the correct syntax.

4.   else if (isset($user) && strlen($user) > 0) {
       $userlist[] = "owner=". $users;
     }
   

   The code is not using a Drupal Unicode function.

5.   if ($userlist == null || empty($userlist)) {
       $userlist = array();
     }
   

   See the Drupal coding standards to understand how a module code should be written. The coding standards say to write a constant all in uppercase characters.

6. function tracrss_tickets() {
     // Set page title
     drupal_set_title("Trac RSS");
   

   The string passed to the function should be translatable.

7.     $title = l(t('[edit]'), "tracrss/edit/$id");
       $title .= " ";
       // text of the link
       $title .= t
       (
         "Tickets for %userlist from trac server %tracname",
         array
         (
           '%userlist' => implode(",", $userlist),
           '%tracname' => $server['name'],
         )
       );
       $title .= " (". l($server['url'], $url) .")";
   

   Rather than concatenating translatable strings, it would be better to use the t() placeholders, and use a single string.

8.   $form_state['#redirect'] = array('tracrss');
   

   $form_state['#redirect'] used as it is used by the code is set to a string.

Remember to change the status, when you upload new code, or who reviews the code doesn't know there is new code to review.

    $title = t
    (
      "%editlink Tickets for %userlist from trac server %tracname",
      array
      (
        '%editlink' => l(t('[edit]'), "tracrss/edit/$id"),
        '%userlist' => implode(",", $userlist),
        '%tracname' => $server['name'],
      )
    );
    $title .= " (". l($server['url'], $url) .")";

l() is not used together t(); that is reported in the documentation page for t(), which reports the following text:

Here is an example of incorrect usage of t():

$output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));

Here is an example of t() used correctly:

$output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';

Also, there is still a concatenation of two strings, which could still be avoided.

1.   $msg = 'Cache updated';
     drupal_set_message(t($msg));
   

   As I reported, that code is not correct; the script that extracts the translatable strings just parses the code, and it looks for any strings that are arguments of t(); in the case you write code like t($msg), that script just reports you an error because it doesn't find a literal string as argument for t() (literal string means that a variable is not accepted).
   In this case, the solution is just to write drupal_set_message(t('Cache updated')).

2. function tracrss_cron() {
     $servers = variable_get('tracrss_servers', array());
     $result = db_query_range('SELECT users.uid FROM {users} users');
     while ($row = db_fetch_object($result)) {
       $uid = $row->uid;
       # print $uid;
       $user        = user_load($uid);
       $user_config = tracrss_userconfig_get($uid);
       $servers     = variable_get('tracrss_servers', array());
       foreach ($servers as $id => $server) {
         $userlist = tracrrs_get_userlist_from_config($user_config, $id, $user->name);
         $url = tracrss_make_url($server['url'], $userlist, FALSE);
         if ($user_config[$id]['enabled'] != "disabled") {
           tracrss_fetch_tickets($server, $userlist, $uid);
         }
       }
     }
     return "Messages";
   }
   

   hook_cron() is not supposed to return anything.

    $title = t
    (
      "%editlink Tickets for %userlist from trac server %tracname",
      array
      (
        '%editlink' => l(t('[edit]'), "tracrss/edit/$id"),
        '%userlist' => implode(",", $userlist),
        '%tracname' => $server['name'],
      )
    );
    $title .= " (". l($server['url'], $url) .")";

l() is not used together t(); that is reported in the documentation page for t(), which reports the following text:

Here is an example of incorrect usage of t():

$output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));

Here is an example of t() used correctly:

$output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';