Drupalmonitor Connector

Welcome!

The actual project page is: http://drupal.org/sandbox/lukas.fischer/1398102
Please take a moment to make your project page follow tips for a great project page.

USE AT YOUR OWN RISK! really ? :D
If your module is that evil maybe you should leave it in sandbox ? (or remove this)
Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Drupal 7 installs and uninstall your schemas automatically

function drupalmonitor_connector_install() {
   drupal_install_schema('drupalmonitor_log');
}

You don't have to do that!

The correct commenting head is

/**
 * Returns memory usage
 */

Instead of

/**
 * Returns memory usage
 **/

(I know that's picky)

Always use TRUE and FALSE for boolean values not 1,0

Your using variables -> you got to remove them on hook_uninstall with variable_del()

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.

regards

Clarification

Correcting title.

Not yet, it'll take some time but we'll do a deeper review soon.
(As said you can speed this up by doing reviews your self)

project page updated

Hi

That's a nice idea !

1 ) drupalmonitor_connector.module

Is there any reason why you call 'drupalmonitor_connector_page_settings' instead of 'drupalmonitor_connector_settings_form' from the menu ?

$items['drupalmonitor'] = array(
  'title' => 'Drupal Monitor',
  'page callback' => 'drupal_get_form',
  'page arguments' => array('drupalmonitor_connector_settings_form'),
  'type' => MENU_CALLBACK,
);

2 ) drupalmonitor_connector.module

You can add more details in the permission hook (title and description)
example :

return array(
    'administer drupalmonitor' => array(
      'title' => t('title'),
      'description' => t('description'),
    ),

3 ) drupalmonitor_connector.module

t() should not contain HTML tags.

http://drupal.org/node/322731
or see below :

$message = t("If you don't want to receive such e-mails, you can change your settings at !url.", array('!url' => l(t('My account'), "user/$account->uid")));
$title = t("@name's blog", array('@name' => $account->name));
$message = t('%name-from sent %name-to an e-mail.', array('%name-from' => $user->name, '%name-to' => $account->name));

Please consider to get a review bonus so we will come back to your application sooner :

http://drupal.org/node/1011698
See also #1410826: [META] Review bonus

See #1410826: [META] Review bonus, just ask which step is unclear.

Don't forget to add the review bonus tag if you have done the reviews of other applications.

Your README.txt states:

-- REQUIREMENTS --

* The module does not make sense with an account on www.drupalmonitor.com.

Should be:

-- REQUIREMENTS --

* The module does not make sense without an account on www.drupalmonitor.com.

May be more friendly if it stated:

-- REQUIREMENTS --

* An active account and security hash with www.drupalmonitor.com is required for this module to work.

After enabling your module I get:

Notice: Undefined index: module in _field_info_prepare_instance_display() (line 354 of /home/digitalporch/public_html/modules/field/field.info.inc).

I haven't figured out what exactly is causing it, but the message does go away when I disable your module.

Please disregard the Undefined index post. I realized that it was related to commerce_price but appeared when enabling/disabling your module. Sorry for the false alarm. Great module/service!

README.txt

When installing the module I got slightly lost - what I would have liked to see is

1. Explicit instructions for setting up the hash + security implications
2. What to expect - initially it takes a while to register the site on http://www.drupalmonitor.com/
3. Troubleshooting steps? I disabled cloudflare (caching service) temporarily on my site to see if that was causing the site not to show up on http://www.drupalmonitor.com/
4. Indication of risks - I don't care if someone intercepts my personal sites stats but it will be critical to some people especially as it provides a list of outof date modules:
   • A clear run down of the risks involved - I know that this was addressed above ( http://drupal.org/node/1505236#comment-5794520 ) but I think it better if people make an informed choice
   • Would it possible to ensure that http://www.drupalmonitor.com/ runs over https?

I'd echo andyg5000's comment above about the requirements section!

drupalmonitor_connector.install

/**
 * Implements hook_update().
 */
function drupalmonitor_connector_update_6001() {
  drupal_install_schema('drupalmonitor_log');
}

Don't think you need this and refers to 6001 - drupal 6 hangover?

drupalmonitor_connector.module

Line 9-13: Any reason why not using
http://api.drupal.org/api/drupal/includes%21module.inc/function/module_l...

Line 57: Could there be more details for the stupid? I wanted to see an automatically generated hash here as 'hash' says to me something MD5 hashed. Perhaps rephrased as "Enter a string that will identify this site correctly to drupalmonitor.com - you will need to enter the identical string there." or something

Manual review:
1. Please don't use german names in SQL queries: "SELECT count(*) as anzahl FROM {drupalmonitor_log} WHERE dt > :time_offset" ... the same on other places

2. Use drupal_json_output() instead of "echo json_encode($info);"

3. I don't like the idea of using global vars, it would be better using static and using a function for accessing the variable on demand...

function drupalmonitor_connector_init() {
  global $_drupalmonitor_connector_script_start_time;
  $_drupalmonitor_connector_script_start_time = microtime(TRUE);
}

4. "if ($diff < 2147483648) {"
What is 2147483648?? A comment would be nice ...

5. Autotomated review:

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• 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: ...ll/modules/pareview_temp/test_candidate/drupalmonitor_connector.install
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 35 | ERROR | Concat operator must be surrounded by spaces
 35 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | The $text argument to l() should be enclosed within t() so that
    |       | it is translatable
 36 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | Concat operator must be surrounded by spaces
 36 | ERROR | The $text argument to l() should be enclosed within t() so that
    |       | it is translatable
 36 | ERROR | Concat operator must be surrounded by spaces
--------------------------------------------------------------------------------


FILE: ...all/modules/pareview_temp/test_candidate/drupalmonitor_connector.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 88 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

--
The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could consider evaluating my own application:
http://drupal.org/node/1302786

Just checked a bit the code, some small things:

Coder Review:

drupalmonitor_connector.module:48:75: error - Concat operator must be surrounded by spaces
drupalmonitor_connector.module:48:105: error - Concat operator must be surrounded by spaces
drupalmonitor_connector.module:88:1: error - Whitespace found at end of line

inside of drupalmonitor_connector_settings_form() you use a default value for the variable drupalmonitor_hash:

variable_get('drupalmonitor_hash', md5(microtime().'drupalmonitorrocks_salt_666'.time())),

why? this confuses people because there is already something there and this here:

  if ($hash_site == '' || $hash_site == $hash_request)

in drupalmonitor_connector_page_data doesn't make sense, this means that you can access the /drupalmonitor callback when no hash is set!

  echo json_encode($info);
  exit();

you should use this combination:

  echo drupal_json_output($info);
  drupal_exit();

which will add some checks for JSON problems in PHP and also invokes correctly the exit hooks of all modules.

    header("Pragma: no-cache");
    header("Expires: 0");

you should use: drupal_page_header()

  else {
    $info['drupalmonitor_status'] = "NO ACCESS";
  }

better would be also to return 403 HTTP instead of 200.

drupalmonitor_connector_settings_form()

Please use http://api.drupal.org/api/drupal/modules!system!system.module/function/s... for module settings form, it also make drupalmonitor_connector_settings_form_submit() obsolete.

drupalmonitor_connector_get_load_requestscount_300s()

  $query = "SELECT count(*) as anzahl FROM {drupalmonitor_log} WHERE dt > :time_offset";

Please use english words, not german.

/**
 * Implements hook_perm().
 */
function drupalmonitor_connector_permission() {

wrong documentation, it is now called hook_permission()

/**
 * take time on init
 */

As the Doxygen and comment formatting conventions suggest you should use this style for documentation:

/**
 * Documentation here.
 */

  $query = "SELECT count(*) FROM {users}";
  return db_query($query)->fetchField();

Why don't you use the db_select function? http://api.drupal.org/api/drupal/includes%21database%21database.inc/func...

function drupalmonitor_connector_status_listmodules()
    $infos = unserialize($record->info);
    unset($record->info);
    $record->info = $infos;

can be replaced with

   $record->info = unserialize($record->info);

drupalmonitor_connector_page_data()
// Load Cound Data.

Typo

I would suggest to move the *.inc files into a subfolder, makes the whole module strukture more clear.

Why do you have a specific setting to enable the logging? If it is not enabled you don't run anything at all. So why not just disable the module via the module form?

looks like we submitted at the same time

Removing : PAReview: review bonus
Setting status

well yes seems so, and apparently we found different issues, interesting :)

ah ;)

Hi,
Is there an open source version of the SERVER code that runs on drupalmonitor.com ? That would probably help external contribution.

Please don't change the category of this project application. Use the issue queue of the project instead.

i see that your reviews are generally very short, especially the following 3 ... check the quote below
http://drupal.org/node/1511646#comment-5874464
http://drupal.org/node/1452328#comment-5819028
http://drupal.org/node/1536854#comment-5913024

Posted by patrickd on April 24, 2012 at 10:05pm

Thanks for your participation, but I'm afraid the comments you did are not very detailed and IMHO not really "reviews".
A good deep review can take up to an hour, this is not about just pointing out 2 things and set the issues to needs work.
Sorry, removing tag.
http://drupal.org/node/1540804#comment-5912808

> also short feedbacks are wished and "rewarded"

i think every feedback is generally good feedback, but for calling a feedback "review" is probably something different, but i won't argue on this ;)

new review link added

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time.

manual review:

1. drupalmonitor_connector_schema(): doc block is wrong, see http://drupal.org/node/1354#hookimpl
2. drupalmonitor_connector_enable(): doc block is wrong.
3. drupalmonitor_connector_enable(): "'!hash' => check_plain(variable_get('drupalmonitor_hash')),": use the "@" placeholder with t(), then it will do check_plain() for you. And then you will not need the filter_xss() in drupal_set_message().
4. drupalmonitor_connector_page_data(): You should use drupal_json_output() here.
5. drupalmonitor_connector_page_data(): why do you load all the include files before you check if the incoming request is even allowed to retrieve the data?
6. drupalmonitor_connector_page_data(): Also, no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';
7. drupalmonitor_connector_page_data(): you should return MENU_ACCESS_DENIED or use drupal_access_denied() if you want to deny the request.
8. drupalmonitor_connector_settings_form(): use system_settings_form() then you don't need the submit callback.
9. drupalmonitor_connector_page_data(): So if I accidentially set the drupalmonitor_hash to the empty string this page callback is basically unprotected and everyone can retrieve the site stats. You should only return data if the drupalmonitor_hash is not empty.
10. drupalmonitor_connector_get_load_requestspersecondcount_300s(): so you run the same DB query again just to do the division? Not efficient.
11. drupalmonitor_connector_get_load_slowrequests_300s(): same here, you run two very similar queries here. You could just count the results of one query.
12. drupalmonitor_connector_get_load_slowrequests_300s(): why do you need the foreach()? Why can't you use ->fetchAll() or similar?
13. drupalmonitor_connector_node_contenttypes(): you should use fetchAllKeyed() or similar, then you don't need the foreach(). Also elsewhere.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

• drupalmonitor_connector_page_data(): You should use drupal_json_output() here.
• drupalmonitor_connector_settings_form(): use system_settings_form() then you don't need the submit callback.
• drupalmonitor_connector_page_data(): So if I accidentially set the drupalmonitor_hash to the empty string this page callback is basically unprotected and everyone can retrieve the site stats. You should only return data if the drupalmonitor_hash is not empty.

mhh all these things Klausi mentioned I already mentioned in my review. but @lukas you told us:

I fixed all your mentioned points

Did you really fixxed all of them? Because I still see them??
If you don't actually fix the things, this process will take really long and produces a lot of double work, which is really rare in the application processes....

7. Actually I don't want to drupal_access_denied(); because when drupalmonitor.com is pinging the site, it should be able to read the structured output "NO ACCESS". Would this be a problem?

have you checked the api for drupal_deliver_page?
drupal_deliver_page($page_callback_result, $default_delivery_callback = NULL)

With $default_delivery_callback you should be able to return your own text and still using the normal drupal behavior for access denied

@lukas: take a look at http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal_...

latest version gives me this error when trying to activate the module.
our DB is MS SQL.

DOException: SQLSTATE[42000]: [Microsoft][SQL Server Native Client 10.0][SQL Server]Column, parameter, or variable #1: Cannot specify a column width on data type int.: CREATE TABLE [{drupalmonitor_log}] ( [id] int(15) NOT NULL IDENTITY, [q] nvarchar(250) NOT NULL, [dt] int(11) NOT NULL, [memory] int(15) NOT NULL, [execution_time] int(11) NOT NULL, CONSTRAINT {drupalmonitor_log}_pkey PRIMARY KEY CLUSTERED ([id]) ); Array ( ) in db_create_table() (regel 2688 van C:\inetpub\drupal-7.12\includes\database\database.inc).

leave application issues as tasks please

@lukas

maybe you should not use length on the "int" datatypes, read the doc:

http://drupal.org/node/146939

'length': The maximal length of a type 'char', 'varchar' or 'text' field. Ignored for other field types. Note, length is required for 'varchar's.

'size': The data size: 'tiny', 'small', 'medium', 'normal', 'big'. This is a hint about the largest value the field will store and determines which of the database engine specific datatypes will be used (e.g. on MySQL, TINYINT vs. INT vs. BIGINT). 'normal', the default, selects the base type (e.g. on MySQL, INT, VARCHAR, BLOB, etc.). Data Types and Sizes are explained here.

so you should use size not length

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

I installed the connector module. Disclaimer: I don't have the correct security hash as I can't log into my a/c on drupalmonitor.com, but that's a different story.

Every time the /drupalmonitor page is loaded it leaves the following error:

Warning: Cannot modify header information - headers already sent by (output started at includes/bootstrap.inc:1364) in drupalmonitor_connector_noaccess() (line 239 of drupalmonitor_connector/drupalmonitor_connector.module).

This appears to happen because the cached output has already been triggered and the connector module wants to add the HTTP 403 header.

I suggest changing the hook_menu access arguments to use a custom function, rather than doing all of the logic in drupalmonitor_connector_page_data().

Note - my last comment should not preclude the module from consideration, it'd be perfectly fine as a separate issue for the module's issue queue. Once I can log into my drupalmonitor.com account and can get the module connected properly I'll provide a review.

@lukas: Yes, I did. FYI I have page caching enabled, perhaps that's part of the issue?

Lucas,

Very cool module/service! I had no problems installing or setting up on my shared hosting account. Rerun the automated testing and uncovered no problems. I also love that you put a link to the issue queue in the README file. Nice touch! Overall, README is very well written. Totally off-topic, but I wonder if we are distantly related...

I've checked through several of the previously raised issues and confirmed that you have implemented change/fixes for #37, #40, #43, and #46. I didn't see other reviewers mention that, so I just wanted confirm.

Major Issue (only 1)

CrawlerI haven't seen any data from my test site show up on the Drupalmonitor.com dashboard page. I've entered my security hash into Drupalmonitor.com, and I've provided a variety of traffic to my test site and refreshed the Drupalmonitor.com dashboard several times over the period of an hour. Is there a way to trigger the crawler? This is my only reservation about accepting this module -- I can't verify it worked with Drupalmonitor.com.

When I visit http://example.com/drupalmonitor?hash= url, I do get a valid JSON response which appears to be correct (though I didn't check in depth), so I'm guessing the above will work once the crawler scrapes my site.

If you can help me address this issue, then I'll review again.

Minor Issues

Possible Security Improvement Set a minimum length or test the strength of the security hash. Since this is a required field on the /drupalmonitor url and configuration page, I don't have a problem with letting this go or be an issue on the issue queue.

Quote Usage Consistency This is a super minor detail, and I'm probably guilty of these inconsistencies myself. However there's mixed use of single/double quote on the same line in drupalmonitor_connector.module lines 56, 66, 162, 226, 240, and possibly in other files. Though I don't really care about this issue, the coding standards on use of quotes state:

Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.

Changing status...

Hi Lucas,

I'm moving my minor issues into your sandbox issue queue for further work. Those should not be considered a barrier to full project application IMHO. You've written some beautiful code. Also, given the quality of your code and your active user base, I believe that Drupal is missing out by not having this module on d.o. The code looks good and demonstrates a thorough knowledge of d.o coding standards and apis. Congrats!

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

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

  FILE: /home/klausi/pareview_temp/drupalmonitor_connector.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   7 | ERROR | All constants defined by a module must be prefixed with the
     |       | module's name, expected
     |       | "DRUPALMONITOR_CONNECTOR_VERSION" but found
     |       | "DRUPALMONITOR_VERSION"
  --------------------------------------------------------------------------------
  

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. "variable_get('drupalmonitor_hash')": naming problem again: either you rename your module to simply "drupalmonitor" or you should change the variable name to "drupalmonitor_connector_hash".
2. drupalmonitor_connector_settings_form_validate(): doc block is wrong, this is not a hook. See http://drupal.org/node/1354#forms
3. drupalmonitor_connector_settings_form_validate(): this function is not necessary if you simply set the form element to #required. See http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
4. drupalmonitor_connector_enable(): filter_xss() is not necessary here since no user provided input is involved. Make sure to read http://drupal.org/node/28984 again.
5. drupalmonitor_connector_boot(): doc block is wrong, this is a hook implementation. See http://drupal.org/node/1354#hookimpl
6. "module_invoke_all('drupalmonitor');": naming problem again, and hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php

But that are not application blockers either, so ...

Thanks for your contribution, lukas.fischer!

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.