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.

We are waiting for reviewers to check the code and report what needs to be changed.

From http://drupal.org/cvs-application/requirements, "...You must provide a motivation message, which should be a few paragraphs instead of only a few sentences. It should include details about your contribution, its features, how it compares to already existing solutions, links to screenshots or a demo, etc...."

I've done a visual review of the code.

It looks pretty good overall -- clearly good attention to detail and use of Drupal's APIs and conventions. Its purpose is quite esoteric, but that's fine :)

A few things that need work though:

- Quite a big architectural problem this one: you populate your database table from the included data file in hook_install. How is a user of the module going to get updated data from later versions of the module, or from their own data file? You need to think about a more flexible way of doing this. I suggest you look at how Ubercart handles country information files: there's a UI for adding a custom one, or for reading in data from one that's in the module's own data folder. It looks like you have the same requirements.

- variable_set("show_date", TRUE); -- namespace your variables.

- also, there's no point in setting a variable in your hook_install. Instead, take the value TRUE as your default!

- $lang = $GLOBALS["language"]->language; -- I'm not sure of this, but should you just say 'global $language'?

- $date = getdate(); -- you might want to account for the user's timezone?

- echo format_date(time(), "small"); -- the standard in TPL files is print, not echo.

- also, it's a nice touch to indent the code in TPL, ie indent this line because it's inside an if block. It makes the logic of your TPL much clearer to follow, as at the moment it's hard to see where the ifs end and what gets printed unconditionally.

- <?php if ($date):?> -- think the convention is a space before the closing angle bracket -- check what core does.

- finally, a minor point: no blank line between function doc blocks and the function declaration. But bonus points for putting these in! :)

I'm not sure whether to set this to 'needs work' or RTBC. The architectural problem I mentioned is a big task and needs addressing, but on the other hand the best course might be to grant CVS access so the module author can make a dev release and potentially get the attention of other interested developers to help with this. I'll leave this decision to one of the admins :)

• 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. 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.     case 'list':
         $blocks[0]['info'] = "Name day";
         return $blocks;
   
   

   Strings used in the user interface should be translated.

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! :)

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

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