sajt [sajt]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module/theme; for modules it should include also a comparison with the existing solutions, while for themes a screenshot is also required.

See comment #2.

I putted the code to the github: http://github.com/sajt/nameday I do not really understand why I can not get CVS access.

A lot of portal site like to show the actual name day, in some country (you can see a list in the Wikipedia). There was a lot of solution for it, but no drupal modules. (I did not find).

This module is very easy of use. Install it as usual and select the block from the admin/build/block list. The name day is depend on the current language (and naturally on the current date) and now only hungarian name days is working.

I welcome the new languages files for other countries. The file description is in the datas/README.txt. This file is included when You install the module.

I think my project a little bit better than this: http://drupal.org/project/chuck

• 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. <?php
function nameday_install() {
  drupal_install_schema("nameday");
  variable_set("show_date", TRUE);
  $dir = drupal_get_path("module", "nameday") . "/datas/";
  if ($dh = opendir($dir)) {
    while (($file = readdir($dh)) !== FALSE) {
      $pathinfo = (pathinfo($file));
      if ($pathinfo["extension"] == "namedays") {
        $lang = $pathinfo["filename"];
        $langfile = file($dir . $file);
        foreach ($langfile as $row) {
          $currentrow = split(";", $row);
          if (count($currentrow) == 3) {
            $currentrow[3] = "";
          }
          db_query("INSERT INTO  {nameday} (month, day, language, name, holiday) VALUES (%d, %d, '%s', '%s', '%s')", $currentrow[0], $currentrow[1], $lang, $currentrow[2], $currentrow[3]);
        }
      }
    }
    closedir($dh);
  }
}
?>

   Why isn't the code using file_scan_dir()?

2. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
3. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how Drupal variables, global variables, constants, and functions defined from the module should be named.
4. <?php
    case 'list':
      $blocks[0]['info'] = "Name day";
      return $blocks;
?>

   Strings used in the user interface should be translated.

I finished the changes. Only Joachim's UI suggestion is missing for the data-table manipulation, I must to think about the correct interface.

I hope every other errors is fixed. I added a little token support, what is pre-alpha and maybe very useless.

anybody?

A few minor points for consistency within Drupal:

- the 'datas' directory should be 'data'. ('data' is already a plural...)
- the standard filename for admin pages is MODULE.admin.inc rather than nameday.settings.inc.

The more complex logic in your TPL should really be moved up to a preprocessor function:

- print format_date(time(), variable_get('nameday_date_format', 'small'), variable_get('nameday_date_format_custom', ''));

This should be just a print $nameday_date; do the formatting work in the preprocessor.

- if ($nameday['holiday'] != '' && variable_get('nameday_show_holiday', TRUE))

This should be just if ($holiday):

> I must to think about the correct interface.

What I'd suggest is something like a list of the language files the system has found, along with operations depending on whether the system also has data for that language in the DB:

language status operations
- English new [load data button]
- French loaded [refresh data button]

Good idea adding token support btw! :)