[D7] Site Blackout

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2310391

Project 2: https://www.drupal.org/node/2065565

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxRajabNatshah2209183git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

I just noticed few issues in my testing of this module.

1. I think you should also add a module configure link so that a user can quickly reach at module configuration page from modules list.

2. Inside the function _site_blackout_default_css(), i think you should avoid to write inline CSS and you can include this through an external file. See sample code below.

$filename = drupal_get_path('module', 'site_blackout') . "/site_blackout_admin_form.css";

$handle = fopen($filename, "r");
$contents = fread($handle, filesize($filename));
fclose($handle);

return $contents;

Right now coder module is throwing below warning due to inline CSS written in module file.
warning "Line 475: Use of Perl/shell style comments (sun) is discouraged. (Drupal Docs) [comment_comment_shell]
#site-blackout-close-button {" generated by coder module.

3. You can also delete variables set by this module after module uninstallation.

Hi there,

I tried out your module, it works very nicely. Just a few things I have noticed:

a.) There are a few typos, in README.txt e.g. eidt, contnet, backout, swich. Also in the .module file I found some e.g. Exclud URLs. Maybe you go trough you files again (the translated content to the user) and correct these.
b.) In the exluded URLs tab you have to put every URL in the seperate line. If I have a lot of URLs on some path e.g. /path/url1, /path/url2 etc. I have to write every URL down, if I'm not mistaken. A Wildcard support would be a nice twist here, like /path/* or something.
c.) I noticed I had the permissions only to admin, logged out and I still could change the settings if I went to the URL directly:
/admin/config/system/site_blackout. I think you have to remove the lines "'access callback' => TRUE,'" at the $items in hook_menu. This would be needed if you want your own function to validate access, so for TRUE there would be a string for you function name. But in your case just remove them.

> have a super fast loaded blackout content

Could you explain what this means? I've no idea what this module does!

What's a blackout?

Ah right. Thanks for the screenshots!

So in functionality, it's the same as core's built-in maintenance mode -- the site is inaccessible, apart from a simple front page.

Have you looked into whether you can work with the maintenance mode, rather than invent a whole new functionality?

@joachim This doesn't replicate Maintenance mode.
It's a theme-related module to append an overlay that is handled by a cookie. This does not block the website. It blocks the UI and gives the option for the user to close it, and not show it again.

Many websites use an overlay for visitors, see: http://stream.aljazeera.com/.

The Al Jazeera one is implemented using this module.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Couple of spelling errors in the admin UI

Exclud URLs, spirate lines

"Allow to close after" setting is not being set

You are using the JS setting Drupal.settings.site_blackout_allow_to_close_after, but you are not setting that value in the JavaScript. This breaks the "allow to close after" functionality.

I fixed it locally by adding these lines to _site_blackout_front_content in site_blackout.module.

$close_after = variable_get('site_blackout_allow_to_close_after', 5000);
drupal_add_js(array('site_blackout_allow_to_close_after' => $close_after), 'setting');

"Allow to close after" phrasing is confusing

This option at /admin/config/system/site_blackout/close implies the user won't be able to close the blackout window at all until the set time has passed. The user may actually close the blackout window at any time by clicking the close button; this setting only delays closing the window by clicking anywhere other than the close button. This option needs a description field to explain that behavior.

Other than these issues, this module functions as described.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

please don't RTBC your own applications, see the workflow: https://www.drupal.org/node/532400

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I used an iframe for the content of the blackout page. Now my site appears all white and cannot undo the settings

There are few security issues I have noticed:

/**
 * Implements hook_menu().
 */
function site_blackout_menu() {

  // Site blackout setting page.
  $items['admin/config/system/site_blackout'] = array(
    'title' => 'Site Blackout settings', // Use t() to put text
    'description' => 'To manage black out status and content.',

// Use t() tu put the text.

Find the same in all other places too.
2. Your module name is site_blackout and you are also using _site_blackout in function name. update it with prefix site_blackout or use sommthing else if starts with _ (underscore).

@RavindraSingh: that are not security issues?

And you should NOT use t() in hook_menu(), so the code is correct. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

Underscores are used in Drupal to specify "private" functions, that are not part of the module's public API, so that is fine as well.

As mentioned in #20, it isn't obvious what a blackout is - the module page should explain this.

@RajabNatshah, it would help if you update your description to explain that the module creates a full-page overlay when enabled. Without that context, the "blackout" functionality isn't obvious to everyone.

I was unable to reproduce the iframe issue in #36. iframe elements in the blackout page content functioned normally.

I recommend not showing the blackout overlay on any admin pages. I can't think of any reason you'd want to disable the site for an authenticated admin user.

Enabling excluded URLs works for the admin pages, so that's not a problem. Thanks for pointing that out.

There is a problem with the blackout close delay functionality in your JavaScript. On line 33 of site-blackout.js, you are passing an undefined variable as the time argument to setTimeout().

Please take a look at comment #27 for a suggested fix.

Much better, the delay on closing the overlay works correctly now.

I spotted a security issue on my second pass. In your menu hook, you're setting 'access callback' => TRUE on all the admin links. This allowed me to change the blackout settings as an anonymous user. You can remove that line from each menu item and the 'access arguments' property you already have will restrict access.

You should fix this as soon as you can, as it's possible for any user to change the blackout settings on your demo site right now.

We usually add the security tag when we find access bypass issues like this. Please don't remove it, we keep it for statistics and to show examples of security problems.

hi, Nice module, i have tested the module, it works fine, few comments are:
1. it is recommended to add PHP version and package name in your .info file.
2. Hook_install is missing its always good to use it please check it here
3. FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. at some places its missing.

Does this module differ significantly from the Splashify module?

Following the Project Application Checklist:

1.1 Project page and repository look good.
- https://www.drupal.org/sandbox/rajabnatshah/2209183
- http://cgit.drupalcode.org/sandbox-RajabNatshah-2209183/tree/

1.2 Project appears to serve a distinct enough purpose to not duplicate the Splashify module or any others.

2.1 Repository contains a suitable amount of code for review.

3.1 All identified security issues have been fixed.

4.1 Repository does not contain a LICENSE.txt file.

4.2 Repository does not appear to contain any third party, non-GPL code.

5.1 Project page contains a detailed description of the module functionality.

5.2 Repository contains a suitably detailed README.txt file.

5.3 Code is well commented and readable.

6.1 An automated review identifies some minor code style issues, but nothing that should be a blocker. Results here.

The module has passed Project Application Checklist and is marked as "Reviewed & tested by the community".

The main and most important difference from Spalishify, is that this module is a light-weight standalone module that requires no libraries or dependencies.

Hi RajabNatshah
Code seems to quite ok. But I noticed one thing while reviewing module.

Suggestion:
• It would be good, if you place .tpl files in template folder

@RajabNatshah - Congratulations! Your module is now RTBC!

Have you conducted reviews of other projects? If yes, make sure to add them to the issue summary, and then add the "PAReview: review bonus" tag to the issue to speed up approval of your module! More info on that here: https://www.drupal.org/node/1975228

Thanks so much for your contribution. This looks like a useful module.

Removing review bonus tag, you have only listed one review of other projects in the issue summary. Make sure to read https://www.drupal.org/node/1975228 again.

Review of the 7.x-1.x branch (commit a58d7cf):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• ESLint has found some issues with your code (please check the JavaScript coding standards).
  

  /home/klausi/pareview_temp/js/site-blackout-preview.js: line 9, col 3, Error - Strings must use singlequote. (quotes)
  /home/klausi/pareview_temp/js/site-blackout.js: line 9, col 3, Error - Strings must use singlequote. (quotes)
  /home/klausi/pareview_temp/js/site-blackout.js: line 36, col 11, Error - Strings must use singlequote. (quotes)
  /home/klausi/pareview_temp/js/site-blackout.js: line 37, col 13, Error - Expected indentation of 10 space characters but found 12. (indent)
  /home/klausi/pareview_temp/js/site-blackout.js: line 38, col 13, Error - Expected indentation of 10 space characters but found 12. (indent)
  /home/klausi/pareview_temp/js/site-blackout.js: line 38, col 25, Error - Strings must use singlequote. (quotes)
  /home/klausi/pareview_temp/js/site-blackout.js: line 39, col 13, Error - Expected indentation of 10 space characters but found 12. (indent)
  /home/klausi/pareview_temp/js/site-blackout.js: line 45, col 13, Error - Expected indentation of 10 space characters but found 12. (indent)
  /home/klausi/pareview_temp/js/site-blackout.js: line 48, col 11, Error - Expected indentation of 8 space characters but found 10. (indent)
  
  9 problems
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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:

1. _site_blackout_front_content(): do not use drupal_add_js(), use #attached on the $page render array. See https://api.drupal.org/api/drupal/developer--topics--forms_api_reference... . Same in _site_blackout_preview_page().
2. _site_blackout_front_content(): do not call theme here, just add a nested array with #theme to the $page render array. See https://www.drupal.org/node/930760
3. _site_blackout_close_settings_admin_submit(): doc block is wrong, this is not a hook. This is a form submit handler, see https://www.drupal.org/coding-standards/docs#forms . Please check all your doc blocks.
4. _site_blackout_close_settings_admin_submit(): why do you need to flush all caches here? So that the JS or CSS is picked up? Or something else? Please add a comment. Same for all other admin forms.
5. _site_blackout_preview_page(): why are you not applying the text format with check_markup() here? This means this is vulnerable to XSS exploits because for example the specified text format "filtered_html" is not applied which would filter the content. If I enter <script>alert('blackout');</script> as blackout content I will get a nasty javascript popup. Make sure to apply the configured text format and read https://www.drupal.org/node/28984 again.

The last point is a security blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.