System Status

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.

The automated review has passed on all code files, the README.txt file, however, contains single lines that are too long (limit is 80 characters per line).

Instructions in README instruct to go to /admin/config/system, yet your hook_menu implementation calls for admin/config/system/system_status.

When I pull up the list (system/status), I am not getting anything for contrib modules, only core. From what I see, the problem lies in the way you determine whether a module is 'core', 'contrib' or 'custom'. You are assuming the paths are */modules/custom/ and */modules/contrib/ instead of sites/*/modules and/or sites/*/modules/custom. It might also be a good idea to allow the users to specify their custom module directory (default to 'custom').

Another way to consider checking would be to look at: $module_info->info['package'] or $module_info->info['project'] which would result in 'Core' and 'drupal' respectively.

Hope that helps a bit at least.

Sorry, but vlad.pavlovic forgot to change the status.

Hi, Sam Hermans!

I've installed your module on a test site.

In the last paragraph of the instructions it says:
"Once you saved the settings you should be able to: GET /system/status"
though this statement is true only if the drupal installation is placed only in to the root.

Best regards.

hi @Sam Hermans,

I ticked "Enable custom modules" but there wasn't a text box for their regex until I hit save. you could mention this behaviour under the box with a '#description' property. another option is to make that text box appear - without the user having to save - simply using a '#states' property:

<?php

'#states' => array(
  'visible' => array(
    ':input[name="form_do_match_custom"]' => array('checked' => TRUE),
  ),
),

?>

also regex isn't very n00b-friendly; maybe you could use the more generic {^sites\/([A-z,\.,\-]+)\/modules\/*} instead.

and I can confirm the module works on a localhost :) good luck!

cheers,
Luciano

Hi Sam,

overall it looks pretty good, it works as described and I can see how it could be useful.

I've got a few suggestions, some of which are just recommendations and you can decide if you want to implement or not, and a few which I think are required. I'll differentiate them by using 'could do' and 'need to'

1. .gitignore
Need to remove this .gitignore file. You should ignore these files locally in ~/.gitignore

2. Use of the /system/status path
I think this could probably go under the /admin/reports/status route, possibly /admin/reports/status/system_status
It would be nice if this was also a clickable link on the /admin/reports/status screen, eg like the phpinfo() function.

3. You could consider moving the menu callbacks into separate include files

  $items['system/status'] = array(
    'page callback' => 'system_status_status_page',
    'access callback' => 'system_status_access_callback',
     'file' => 'system_status_status_page.inc'
  );

This makes a Drupal site more performant because it does not need to load so much code for every page, and also helps other developers looking at your code because related functions are stored in individual files.

4. Variable names used (eg in system_status_form() and system_status.install
Variables need to be prefixed with the module name to help avoid potential namespace issues.
eg system_status_public_allow_public instead of form_public_allow_public

5. Minor performance issue in system_status_access_callback()
You might see a slight performance increase by setting a variable to ip_address() rather than make repeated calls to that function.

6. Hardcoded IP check - if (system_status_match_cidr(ip_address(), "91.212.186.0/24")) {
Need to do a lookup of the domain name here, IP could conceivably change, also easier for other developers to check the code is correct. I get very concerned when I see a hardcoded IP in module code as it's one of the signature's of a hacked site.

7. Regex use
The path matching could be more usable if you used drupal_match_path() rather than a regex.
eg instead of

    preg_match($regex, $filename)
    drupal_match_path($filename, $patterns);

which works with patterns like
drupal_match_path($filename, 'sites/all/modules/*');
The patterns variable can be multi-line (so input via a textarea) if several different path types want to be matched, as used on eg block config screens.

8. JSON output
Need to set the correct content type header for the returned json. You can use
drupal_json_output(array("system_status" => $res)) instead of echo drupal_json_encode(array("system_status" => $res)); or you could set the headers manually if you prefer.
http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_j...

That all looks pretty good so far.

3

For 3 you could also move system_status_access_callback() and system_status_match_cidr() into system_status_status_page.inc if you wanted.
You could also put the admin functions into a system_status_admin.inc if you like.

<?php
  $items['admin/config/system/system_status'] = array(
    'title' => 'System Status',
    'description' => 'Configuration for System status module',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('system_status_form'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
  );

Optional if you want to do this, but I think it would be nice.

6

For 6. could you make a request to your server to get the range?

$response = drupal_http_request('http://www.drupalstatus.org/some_path/range.json');
$range_data = drupal_json_decode($response->data);

You could just use a plain text file with the data in, shouldn't be any need to write an api or anything.
I think fixing 6 is necessary, but I'm willing to be overruled by someone else if they don't think it's a concern.

7

I think for 7. you will eventually need to add checkboxes to allow users to select which are custom modules. Sometimes we put them in /sites/all/modules/custom but another pattern that I see more often is to name each module with a prefix for the client so for the BBC you might create a set of modules named bbc_some_module_name. I don't think 7 should be a blocker for this to be a full project though, more of a feature request really.

Installed and enabled with no effort.

I was a little bit surprised one could not enter a subnet for IP filtering. I'm not sure if it's related to the previous reviews you received.

Another point : the name of a couple of file seems not correct, if i'm not mistaken.
I would rename system_status_admin.inc to system_status.admin.inc and system_status_status_page.inc to system_status_status.page.inc to better follow the module_load_include() API.

Last one : the callback URL of your JSON output is in the admin/* area whereas it's not really an administrative UI because it's not even a regular page. Maybe system-status/report would be more appropriate. In the same topic, I'm a bit uncomfortable with the fact that your entry adds a regular menu item in the Admin section (in admin/reports) which lead to a machine-only non-HTML output. I suggest you declare your item as a MENU_CALLBACK.

I let more experienced reviewers decide it this leads to a new 'need works'.

Nice job, by the way.

Hi again Sam,

that all looks great to me, thanks for making the changes. If you feel you need to put the hard coded IP back in it would be nice to add a comment and a longer note in the README explaining. I'm thinking to make it easy for users you could add a help message asking users to input the IP range manually if gethostbyname doesn't return a result. Might be good to add this value as a define so that it doesn't get changed in the code and not the help message.

Again, this is more of a feature request than a requirement for full project, so I'm going to mark this as RTBC.

Changing priority to normal as well, not sure why it was set to minor?

Thanks,

Mark

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

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

  
  FILE: /home/klausi/pareview_temp/system_status.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   41 | WARNING | The administration menu callback should probably use
      |         | "administer site configuration" - which implies the user can
      |         | change something - rather than "access administration pages"
      |         | which is about viewing but not changing configurations.
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 2.50Mb
  

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.

So the permission problem is a security blocker, but otherwise this looks almost RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Thanks for your contribution, Sam Hermans!

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.