tstoeckler [tstoeckler]

Overall, this module almost looks too small to deserve an own project.

However, considering that this is a trivial use-case and there may be demand for it, this looks ok - although there are some localization issues with strings in t().

The block rendering function clock_content() should be a theme function and get the display settings already prepared as settings array.

Additionally, I expected some jQuery/JavaScript that updates the clock on the page (optionally / configurable). Probably a good target for future development.

Another possible enhancement in the future would be to support clock "widgets", i.e. JavaScript/Canvas/Flash-driven graphical elements (instead of text).

I'd also like to see an open communication and collaboration with the wordclock module maintainer(s).

Furthermore, I'd be interested to see a kind of "mini-widget" that could be exposed to Administration menu (admin_menu), so one could display a small clock in the menu.

Also, there are plenty of coding style and coding standards issues. Please run this code through Coder and alternatively the coder_format script.

1.             'short' => t('Short date format') . ' (' . $short_date_format_formatted . ')',
               'medium' => t('Medium date format') . ' (' . $medium_date_format_formatted . ')',
               'long' => t('Long date format') . ' (' . $long_date_format_formatted . ')',
               'custom' => t('Custom date format'),
   

   Use a placeholder.

2.   if ($clock_timezone_type == 'site') {
       $clock_timezone = variable_get('date_default_timezone_name', 'UTC');
     }
     if ($clock_timezone_type == 'user') {
       $clock_timezone = NULL;
     }
     if ($clock_timezone_type == 'custom') {
       $clock_timezone = variable_get('clock_custom_timezone', 'UTC');
     }
   

   This is a minor optimization, but the code should be rewritten, as one value excludes the other.
   It would also be better to use integers for that setting, as the setting values are part of a set.

What reported by sun about the theme function is still true.

@tstoeckler: I know that you are an active member of the Drupal community... can I trust you and we simply move forward here or do you want to be on the safe side of life and go with us through this process to make sure your CVS stuff has a value for users in the end? :)

@tstoeckler: The applicant is not surely the person who can change the status to reviewed & tested by the community.

I don't see anything wrong in the code, except I don't understand why it loads data from the other module database table, when it could use date_api_date_formats(); there are date formats that are not found in the database.

function clock_theme() {
  return array(
    'clock' => array(
      'arguments' => array(
        'clock_timezone' => 'UTC',
        'clock_date_format_type' => 'short',
        'clock_custom_date_format' => 'g:i a',
      ),
...
function theme_clock($timezone = 'UTC', $format = 'g:i a') {

Those theme function arguments should match. The keys specified in the registry should map to the function argument variable names. Furthermore, I think (!) you do not need to specify default values, because theme() will already assign them according to the theme registry. (However, it's possible that this is a D7-only feature, so please test...)

  switch ($clock_timezone_type) {
    // Site time zone
    case '1':
      $clock_timezone = variable_get('date_default_timezone_name', 'UTC');
    // User time zone
    case '2':
      $clock_timezone = NULL;
    // Custom time zone
    case '3':
      $clock_timezone = variable_get('clock_custom_timezone', 'UTC');
  }

Those cases are missing break; statements - which means that the last ('3') will always be in effect.

  $output .= '<span class="clock">' . $clock_formatted . '</span>';

You likely want to convert this into a DIV.

function clock_block($op = 'list', $delta = 0, $edit = array()) {
    case 'view':
      $block['subject'] = 'Clock';
      $block['content'] = clock_content();
      return $block;
  }
}

/*
 * Return the content of the clock block.
 */
function clock_content() {
  return theme_clock($clock_timezone, $clock_date_format_type, $clock_custom_date_format);
}

You can directly invoke the theme function from hook_block().

However, the rest of the code looks just fine, and we are not really here to teach best practice coding, just prevent horrible code from hitting CVS contributions. Thus, I'll move forward and mark this fixed.

I am giving credits to the users who participated in this issue.