Bookable

The waiting time for a project application review is currently approaching six weeks. Please consider shortening your wait time by contributing to the code review process. All it takes is basic module writing skills, plus it is a great way to add to your knowledge of Drupal.Please visit http://groups.drupal.org/code-review for details on how to participate.

I've taken a brief look through your code to find any obvious issues. Here's a few comments:

• Reading your module description, I do not understand what it is supposed to do. Could you expand the description in your sandbox page and in your README?
• In your README.txt, all lines should be wrapped at 80 characters. Check the module documentation guidelines for more info.
• $Id$ lines in files are not necessary now that drupal.org uses Git, no matter what the Coder module tells you.
• Your bookable_uninstall() implementation should not delete your module's entry from the system table. See the hook_uninstall documentation.
• Use the Coder module to fix some style issues, like spacing in if(is_numeric()) (space between if and condition).
• In bookable_booking_node_form_validate, do not substitute variables into strings directly; use t()'s automatic variable substitution and sanitization. Use t() on each error string independently, rather than on the concatenated group; as stated in the t() documentation, "Because t() is designed for handling code-based strings, in almost all cases, the actual string and not a variable must be passed through t()."
• Your hook_menu uses bookable.admin.inc but that file does not exist in your repository. Your module will not function properly without it.
• check_overlap() should be named bookable_check_overlap() to prevent another module from using the same function name, which would cause an error. Also, on line 181 of that function you can simply do $results[] = $row to avoid having to keep track of $i -- $results[] automatically advances to the next array element each time it is used.

Once you've fixed these issues, please set the status of this issue back to "needs review" and someone will take another look through.

Still a few issues.

• As mentioned previously, README.txt should be wrapped at 80 characters.
• The $Id$ line in bookable.info doesn't need to be there with git.
• bookable_booking_node_form_validate's error reporting seems to be faulty, for a few reasons. First, error strings are concatenated into $errors, but $errors is not declared before use -- $error is, and it's never used. You can just call form_set_error for each error message, instead of concatenating; Drupal can handle multiple error messages. (That would save you from manually linebreaking the messages like you do currently.) Finally, I think is_numeric($nid) and !isset($nid) are mutually exclusive conditions, so the second if may as well be an else if for clarity. (It tripped me up for a moment.)
• As mentioned previously: check_overlap() should be named bookable_check_overlap() to prevent another module from using the same function name, which would cause an error. Likewise with install_content_copy_import_from_file().

Please fix all these issues, then set the status back to "needs review." If you need any help or advice, feel free to ask questions here.

Ah, I see; the problem is multiple errors on one form component. In that case, I suppose you can't do it any other way than concatenating. Sorry for the misleading advice.

You should create a 6.x-dev branch using these instructions:

http://drupal.org/node/1066342

...since the module release system uses branches to distinguish between major module versions.

$i is unused in bookable_check_overlap.

Otherwise I don't see any major issues. I no longer have the time to a deeper review, unfortunately.

* Git release branch missing, see http://drupal.org/node/1015226
* do not include an @file doc block in the info file, you can make comments there by starting a line with ";"
* "//Install the content file" There should be a space after "//". Comments should end with a "."
* bookable_form_alter(): doc block has an additional empty line, remove it. See http://drupal.org/node/1354#functions
* "@param $from_date string" should be "@param string $from_date". Description in the next line should be indented with 2 spaces.
* I don't see a hook_permission(), so how do you control who can make a node booking?

Looks like everything in #10 is addressed, except the question about permissions, which I think I can answer. This module appears to just add booking data to existing node types, so CCK manages permissions.

Please take a moment to make your project page follow tips for a great project page.

In particular, please add a comparison of this solution to other similar solutions. In a quick search I found
* http://drupal.org/project/bookings
* http://drupal.org/project/resource_booking
* http://drupal.org/project/merci
* http://drupal.org/project/simple_reservation

It's OK to create a module similar to others, but that makes it hard for people to figure out when to use your module or some other module. Providing a comparison will help them decide.

Hi sigismo,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.

Please note what greggles said in #18 and give your project page more description.