Going Down

Cool module. I can definitely see where this might be useful for users. A few thoughts as I reviewed its code and functionality:

1) In your configuration form's validate function, you are doing a bunch of checks to make sure users have entered required values. You should instead do this by adding '#required' => TRUE, to the form elements. This is the "Drupal Way" and it will include the theme's styling for required fields.

2) The site maintenance configuration page is throwing an error for your implementation of hook_form_FORM_ID_alter(). This hook only takes the first 2 arguments, not the form_id (in Drupal 6).

3) Also in drupal 6, variable_get() requires a 2nd parameter for the default. You're missing it on line 65.

4) Below is the PAReview.sh automatic review script's notes on your module. You should fix up these to meet Drupal's coding standards.

Great start...just some clean up to do :)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./going_down.module
  ./going_down.install
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 6 WARNING(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
 3 | WARNING | Line exceeds 80 characters; contains 119 characters
 4 | WARNING | Line exceeds 80 characters; contains 109 characters
 5 | WARNING | Line exceeds 80 characters; contains 118 characters
 6 | WARNING | Line exceeds 80 characters; contains 120 characters
 7 | WARNING | Line exceeds 80 characters; contains 120 characters
 8 | WARNING | Line exceeds 80 characters; contains 117 characters
 8 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/going_down.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  2 | ERROR | You must use "/**" style comments for a file comment
  6 | ERROR | You must use "/**" style comments for a function comment
 16 | ERROR | The final ?> should be omitted from all code files
--------------------------------------------------------------------------------


FILE: ...review/sites/all/modules/pareview_temp/test_candidate/going_down.module
--------------------------------------------------------------------------------
FOUND 27 ERROR(S) AND 4 WARNING(S) AFFECTING 23 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | You must use "/**" style comments for a file comment
   7 | ERROR   | You must use "/**" style comments for a function comment
  18 | ERROR   | Inline comments must start with a capital letter
  18 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  18 | ERROR   | Comments may not appear after statements.
  21 | ERROR   | Inline comments must start with a capital letter
  21 | ERROR   | Comments may not appear after statements.
  24 | ERROR   | No space found after comma in function call
  31 | ERROR   | else must start on a new line
  32 | ERROR   | Inline comments must start with a capital letter
  32 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  41 | ERROR   | You must use "/**" style comments for a function comment
  50 | WARNING | A comma should follow the last multiline array item. Found: )
 104 | WARNING | A comma should follow the last multiline array item. Found: )
 111 | WARNING | A comma should follow the last multiline array item. Found: )
 116 | ERROR   | Inline control structures are not allowed
 121 | WARNING | Avoid backslash escaping in translatable strings when
     |         | possible, use "" quotes instead
 125 | ERROR   | Inline control structures are not allowed
 134 | ERROR   | Missing function doc comment
 136 | ERROR   | Inline comments must start with a capital letter
 136 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 167 | ERROR   | You must use "/**" style comments for a function comment
 175 | ERROR   | Inline comments must start with a capital letter
 175 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 175 | ERROR   | Comments may not appear after statements.
 178 | ERROR   | Inline comments must start with a capital letter
 178 | ERROR   | Comments may not appear after statements.
 188 | ERROR   | You must use "/**" style comments for a function comment
 197 | ERROR   | Missing function doc comment
 202 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 207 | ERROR   | The final ?> should be omitted from all code files
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Added Git

Looks pretty good to me. Good stuff.

As for your question in number 1, hmmm, I'm not totally sure of the best way to do that. Some options:

1) leave it as it is now. Since they are now required, the user will have to enter the values in for those fields when they first turn on Going Down. Then when they turn it off, those values should still be there from your variable_get calls, so it probably won't be an issue. A user would have to delete the required fields and turn off giong_down for the error to throw, which is probably unlikely. It's not totally clean though.

2) In Drupal 7, I might use '#ajax' and and '#states' (http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...) to disable those fields with default unused values when the user disables going_down. Not sure if that's easy to accomplish in Drupal 6, or if that's what the best solution is.

3) Go back to your custom validation implementation, which has higher usability than #1 but lacks visual cues to the user that those fields are required.

Hmmmm...

Also the automated review comes back clean except for one error: The master branch still has files in it besides README.txt. See Step 5 here for instructions on how to remove them: http://drupal.org/node/1127732

Namely, these commands should do it:

git checkout master
rm -R ./*
echo See major version branches.> README.txt
git add -A
git commit -am "Removed files from deprecated master branch."

I vote for go back to custom validation, and show users the validation rules through fields description.

@tinker I did not get this error. pareview only returns this:

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/going_down.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
12 | ERROR | Missing parameter type at position 1
160 | ERROR | Last parameter comment requires a blank newline after it
--------------------------------------------------------------------------------

So it seems you're good on the branching part. Also I checked the repository viewer, and all seems good.

I'm very sorry for the delay,
as there are currently hundreds of application this is all runnning slow.. :/
We do really need more hands in the application queue and highly recommend to get a review bonus so we will(/can) come back to your application sooner.

Here's my review:

Licensing: No problems found

Module duplication: No module I could find on Drupal.org does what Going Down does.

Overall understanding of Drupal API's: Looks good to me

Security: The unfiltered $style and $message variables that get passed to the templates in the footer hook would be a security risk if the only user that can set them wasn't the admin. That said, it would definitely be Best Practice to check_plain() or check_markup() user entered values, even if the user is the admin.

Code documentation: Functions are documented well enough, and the code is clear and easy to read.

Automated review:

FILE: ...review/sites/all/modules/pareview_temp/test_candidate/going_down.module
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
17 | ERROR | Additional blank line found at the end of doc comment
162 | ERROR | Last parameter comment requires a blank newline after it
164 | ERROR | Additional blank line found at the end of doc comment
223 | ERROR | Additional blank line found at the end of doc comment
240 | ERROR | Additional blank line found at the end of doc comment
--------------------------------------------------------------------------------

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I went through this project application review checklist, and I think the checkpoints are mostly fulfilled, but I found some issues. Here are my suggestions.

User provided value is passed to t()

In the method _going_down_get_message() a user provided value is passed to t():

$message = check_plain(variable_get('going_down_message', '@site is going offline for maintenance: @time'));
$message = t($message, array(
  '@site' => check_plain(variable_get('site_name', 'Drupal')),
  '@time' => $time_str));

This is problematic for many reasons. Please read the section Go the easy way with t() - wreck your ship in Gábor's blogpost for a more detailed explanation.

Variable naming

There are a few variables in the method _going_down_get_js() which are not named according to the coding standards, such as $messagegoingdown and $messageoffline. These should be corrected with underscores indicating the separated words. Also, the object that you pass to JavaScript as setting should follow the naming conventions described in the JavaScript coding standards, so the keys needs to be lowerCamelCased.

Not used format_date()

strftime() is used in many places, but there is a Drupal API method to use for the same purpose: format_date().

JavaScript API

The file going_down.js doesn't follow the already mentioned JavaScript coding standards. Please review it and correct the naming of your variables accordingly. Also, please take a look at the Drupal JavaScript API. You can for instance eliminate the $(document).ready usage, since Drupal has its own way to handle the same thing through Drupal.behaviors.

Punctuation in error messages

It's a very minor thing, but the error messages provided in the method going_down_system_site_maintenence_form_validate() are missing punctuation.

Nice progress you made. I have only two remarks.

• I think in the method _going_down_get_html() the default value of the `going_down_message` variable shouldn't be translated with t() either. I would suggest you to use format_string() it the project were for Drupal 7, but in Drupal 6 you could use sprintf() to get the values replaced (just don't forget about escaping).
• In your JS file you create a function (Drupal.settings.goingDown.remaining). You should never add a function to the Drupal.settings object, because it's purpose is to store settings which are globally accessible by any method that is added to Drupal.behaviors. Also, there is no need to wrap all your functionality in a method here, so I think you can just omit that.

Only the default '@site is going offline for maintenance: @time' is going through t(). If the user changes it then their text does not go through t(). According to Gábor's blog post this is acceptable because the default English text never changes and can always be referenced. This is how drupal_site_offline() does it.

Valid point. :)

I have moved Drupal.settings.goingDown.remaining() to Drupal.goingDownUpdate() - I need this function because it is triggered by setTimeout("Drupal.goingDownUpdate()", 1000) so that the text gets updated every second.

I haven't noticed the setTimout call, sorry. In this case, I would recommend you to declare your method outside of Drupal.behaviors.goingDown. Also, it's better to create an object for the module inside Drupal, and that can hold all the methods related to the module. E.g.: Drupal.goingDown would be the object, and your method would be Drupal.goingDown.update().

With that said, I'm setting it to RTBC.

Add one more review.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

Removed automated reviews.

Review of the 6.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/going_down.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   223 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
   226 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 5.00Mb
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• "check_plain(variable_get('going_down_message', t('@site is going offline for maintenance: @time', array('@site' => $site))));": if the default value is used then there might be double escaping, which is bad. I would split that up and only run check_plain() if the variable actually exists.

But that are not blockers, so ...

Thanks for your contribution, tinker!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.