Logbook

You need to fix documentation throughout the module. It is very lacking.

Your hook documentation should say:

Implementation of hook_menu(). ...or... Implements hook_menu().
Not...
Implements hook_menu ...or... implements hook_menu

Most of your functions have no documentation at all and all your functions should be namespaced, even if they're private i do believe. Please review the coding and documentation standards and set this back to 'needs review' once documentation is updated.

Good luck!

Right on! I will try to get to this shortly.

Okay, so your documentation is indeed improved, but there are still some issues. Comments should always start with a capital letter and end with a period in sentence format. Actually, here is an example of documentation covering most of the bases (from one of your functions, along with pretend parameters and return value):

/**
 * Page callback for 'logbook/add'.
 *
 * Creates a link to be able to add a particular logbook. This documentation should be
 * no longer than 80 characters if possible, which was done well.
 *
 * @param $uid
 *   A user ID
 * @param $node
 *   A node object
 *
 * @return
 *   An associative array of logbook data
 */
function logbook_add($uid, $node) {
  // Return an array of data for insert into the database.
  return $data;
}

The example above makes careful uses of many standards. This may seem long and like a lot of unnecessary documentation, but remember that in cases like common hook implementations and functions with $form and similar parameters they don't generally need to be documented in @param, @return, etc. Just use discretion and document what might not be obvious. Some of these documentation standards I noticed were issues in your module, which I'll detail below.

Even hook implementations should be Implements hook_menu(). starting with a capital letter and a period at the end.

Inline comments that use // should start with a space, capital letter, and end with a period like // This item does this.

@return documentation should be like this as an example:

 * @return
 *   An associative array of form values
 */

So, we have @return on one line, and the following line, indented two extra spaces, has the information on the returned value.

Similarly, @param documentation is like:

 * @param $rid
 *   A role ID
 * @param $permission
 *   A string representing permission

For good examples on documentation standards, refer to core modules.

Aside from these documentation formatting issues, the module looks like it makes proper use of the API, uses placeholders and t(), implements hook_help(). All good.

I haven't tested the module yet, but noticed it has a dependency on CCK. Normally I see this as dependencies[] = content, does the dependency work properly using dependencies[] = CCK?

Lastly, you should add a README.txt file detailing instructions on installation and configuration of the module.

Thanks again for your contribution and I look forward to continuing the review!

Actually, it looks great! Nice job. I think your well on your way. I will try out the module soon.

Thanks again for all your hard work. This can be a long and sometimes confusing process.

Hi Jack,

You can read more about comment formatting conventions at http://drupal.org/node/1354.

I was going to give this module a try, but when I try to enable it says I don't have cck installed. (But I think I do...?). Are you sure 'cck' is a dependency (and not 'content')?

Anyway, after changing the dependency to 'content', things work okay. When I try and edit other CCK content types, I get an error message
"Notice: Undefined index: content_type in logbook_form_alter() (line 300 of /Users/bfroehle/tmp/logbook/logbook.module)."

It'd be nice to have a README or similar so I would know how to get started. (For example, just going to 'logbook/add' didn't present me with any options, so I had to figure out how to go to settings/logbook first.

More comments:

1. Use theme('checkbox', ...) instead of '<input checked="true" type="checkbox" name=logbook_add_permissions[] value="' . $drupal_type . ':' . $rid . '">';
2. I don't see why this logbook_revoke_permissions function needs to exist. You can see how user.module does it at user_admin_perm_submit()
3. Also, in your form submit handler, you should be using $form_state['values'] instead of $form['#post']
4. In logbook_add, you should be using l() instead of the manually constructed URL. Also, I can't help but wonder if a presentation like node/add using dl/dt/dd or an unordered list using theme('links', ...) would be more appropriate.

Overall things are looking pretty good. I love the concept, and the presentation is pretty great.

@bfroehle did you pull the latest commit? These documentation, dependency, and README issues were fixed as per #5 and I see them in the repository.

@jordojuice: The author must have done them today. That's fantastic!

There are certainly ways to access $form_state['values'] with the form built correctly. I wrote this as a quick example of something that might work properly with a bit of theming (if I'm reading your code correctly). This is a simple list of checkboxes. (didn't test it, but I know this is how it goes, and the point is that it can be done this way.)

/**
 * Form function example.
 */
function mymodule_form() {
  foreach ($user_roles as $rid => $name) {
    // This is where the value of each checkbox is set ($roles)
    // so you could still set default values to checked or unchecked.
    $roles[$rid] = '';
    $form['role'][$rid] = array('#value' => $name);
    $form['field'][$rid] = array('#value' => $some_variable);
  }

  $form['roles'] = array(
    '#type' => 'checkboxes',
    '#options' => $roles,
  );

  return $form;
}

/**
 * Form theme example.
 */
function theme_mymodule_form($form) {
  $header = array(
    theme('table_select_header_cell'),
    array('data' => t('Permission')),
    array('data' => t('Something else')),
  );

   if (isset($form['role']) && is_array($form['role'])) {
    foreach (element_children($form['role']) as $key) {
      $rows[] = array(
        drupal_render($form['roles'][$key]),
        drupal_render($form['role'][$key]),
        drupal_render($form['field'][$key[),
      );
    }
  }
  else {
    $rows[] = array(array('data' => t('No roles available.'), 'colspan' => '3'));
  }

  $output = theme('table', $header, $rows);
  $output .= drupal_render($form);

  return $output;
}

/**
 * Submit handler for mymodule_form().
 */
function mymodule_form_submit($form, &$form_state) {
  $roles = array_filter($form_state['values']['roles']);
  foreach ($roles as $role) {
    // Since role IDs were used for the key, we have access to them here!
  }
}

Indeed it can be very finicky to work with! I understand. I spent a lot of time working with these types of forms in the module I just released, as well. And without everything fitting together perfectly it often will not work at all. So that should be okay with me. But maybe keep this method in mind for the future or for other applications. It does make good use of the Drupal API, which is mostly the reason i showed you that instead.

I'll have to take another look at this here soon.

I think we were waiting for some improvement to the permissions form handling, like the example posted in #14. I'm not comfortable approving the application until $form_values is used properly.

Some additional comments:

1. You should be using t() to translate bits of text into non-english languages. See hook_help() for inspiration of how to use t() in logbook_help().
2. Download the coder module and use it to perform a code review of your module. Fix any errors it finds.

* lines in README should not exceed 80 characters
* indentation: always use 2 spaces per level (wrong in logbook_extra_menus())
* don't use /** and */ for comments in function bodies, they are only used function doc blocks. Use // instead.

Hi jack_tux,

Some problems I found:

function logbook_uninstall() {
  drupal_uninstall_schema('logbook');
  db_query("DELETE FROM {variable} WHERE name LIKE 'logbook\_%'");
  menu_rebuild();
}

Everything in this function seems to be unnecessary. There's no schema_logbook() definition, so nothing to uninstall there. There are no calls to variable_set(), so nothing to delete there. And menu_rebuild() is already called from system_modules_submit(), which handles installing and uninstalling modules. logbook_install() also seems to be unnecessary, so you should probably just remove the whole file, unless I'm missing something.

'#collapsible' => True,

TRUE should be all-caps.

print theme("page", $page); should just be return $page; The page wrapper will be added automatically on a return.

$regex = "/(\s+?)(\w+)(\s?)(\w+)(\s?)(" . $perm . ")(\s?)(\w+)(,?)/";

It's not clear to me what's going on here, but there must be a better way to do this. Have you looked at hook_node_grants()?

• indentation errors in logbook_menu() and elsewhere. Did you run coder?
• "else {" blocks need to start on a new line

• "6.x-2.x-dev" is not a valid git tag, see http://drupal.org/node/1015226
• You have now 3 branches, I think one should be enough for a start. The master branch cannot be delted right now, but you can remove all contents (like here http://drupalcode.org/project/rules.git/tree/refs/heads/master ). And you should remove one other.
• indentation errors: the whole contents of the module file is indented 2 spaces at the base level, why?
• logbook_extra_menus(): why do you need this function? do you want to re-use it later? If not just move the contents to hook_menu().
• logbook_add_extra(): why do you need that function? Looks like you are re-implementing the URL alias system. Or you could just re-use "node add" page callback function in the menu entry.
• code style: "user_roles(false);" should be "user_roles(FALSE);" Coder did not detect this?
• "$form['#post']['logbook_revoke_permissions']": why are you looking into $form['#post']? Better use $form_state['values'], no? See also comment #14

Hello Jack_tux,

here is few issues :

A security issue for preventing SQL injection attacks : you don't sanitize datas thats are going to be used on sql query :
To Fix it :
line 202 : _logbook_revoke_permissions(check_plain($revoke));
line 211 : _logbook_add_permission($rid, check_plain($permission));

- in logbook_menu() : don't use t() fonction on the "title" key (reference ) but the untranslated title of the menu item.:

 $items['logbook/add'] = array(
  22     'title' => 'Logbook Entry',
  23     'page callback' => 'logbook_add',
  24     'access arguments' => array('add logbook'),
  25     'type' => MENU_NORMAL_ITEM,
  26   );

- line 70 and 385 : replace "strtolower" by "drupal_strtolower"

- line 295 : logbook_help() : separate content from presentation like here. Write :
return ("<p>" . t('Select which type of Logbook Entry you would like to make') . "</p>");

- factorize code :

         // Already has this permission so create a checkbox that is already checked.
 143           $row_perms[] = theme('checkbox', array(
 144             '#return_value' => $drupal_type . ':' . $rid,
 145             '#value' => TRUE,
 146             '#name' => 'logbook_add_permissions[]',
 147             "#parents" => $form[$logbook['type'] . '_permissions']
 148               ));
 149         }
 150         else {
 151           // Else just create a checkbox.
 152           $row_perms[] = theme('checkbox', array(
 153             '#return_value' => $drupal_type . ':' . $rid,
 154             '#name' => 'logbook_add_permissions[]',
 155             "#parents" => $form[$logbook['type'] . '_permissions']
 156               ));

become :

          // Already has this permission so create a checkbox that is already checked.
 143           $row_perms[] = theme('checkbox', array(
 144             '#return_value' => $drupal_type . ':' . $rid,
 145             '#value' =>(strpos($role_permissions[$rid], $drupal_type . ',') !== FALSE),
 146             '#name' => 'logbook_add_permissions[]',
 147             "#parents" => $form[$logbook['type'] . '_permissions'],
 148               ));

I add a coma on line 147 to.

Thanks

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/logbook.module:
   +5: [minor] @file description should be on the following line
   +62: [minor] There should be no trailing spaces
   +310: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

• "_logbook_revoke_permissions(check_plain($revoke));": why are you doing this exactly? You are using the %s database placeholders correctly in that function, which will protect you from SQL injection.
• same for _logbook_add_permission().
• logbook.module, line 366: "@param A form oject.": first, the variable name is missing, to which one refers that parameter description (obviously $form)? See http://drupal.org/node/1354#functions . Second, $form is an array, not an object. Also elesewhere.
• _logbook_revoke_permissions(): Wow Drupal 6 was really ugly, I find that hard to understand. Why can't you load the permission, use explode() to get an array, unset all permissions that match, implode the array and save it back?

make sure to enable PHP notices in your php.ini for development, I encountered this one on content type edit page: "notice: Undefined index: content_type in /home/klausi/workspace/drupal-6/sites/all/modules/logbook/logbook.module on line 322.".

and there is a huge list of notices if I just add a Logbook content type and then visit the Permissions page.

notice: Undefined index: #required in /home/klausi/workspace/drupal-6/includes/form.inc on line 2246.
notice: Undefined offset: 0 in /home/klausi/workspace/drupal-6/includes/form.inc on line 834.
notice: Undefined index: #id in /home/klausi/workspace/drupal-6/includes/form.inc on line 1889.
notice: Undefined index: #title in /home/klausi/workspace/drupal-6/includes/form.inc on line 1894.
notice: Undefined index: #required in /home/klausi/workspace/drupal-6/includes/form.inc on line 2246.

And there is a security vulnerability in your permission code. An attacker can easily modify form POST information (I did it with a firefox extension https://addons.mozilla.org/de/firefox/addon/tamper-data/ ) to change the submitted permission string. You must verify the incoming permission string because right now you are writing it as is to the database, which can lead to a permission escalation. I changed "create logbook_test content:2" to "administer nodes:2" in the request and the role got that permission. So everybody with the "manage logbook" permission can assign arbitrary permissions to their own role, which means they can take over your site.

That is exactly the reason why we suggested to use the form API properly with $form_state instead of accessing raw POST data.

jack_tux,

Just a heads up ... while it's not a hard-and-fast rule, it is common practice for reviewers to go to the bottom of the queue (i.e. the application which has gone the longest wait without an update) when picking up a new application.

With this in mind, bumping your own issue can actually result in you having to wait longer for reviews; as it brings your issue back to the 'top' of the queue instead of letting it sift down to the bottom (where many reviewers look first for new projects).

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 6.x-2.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: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 17 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...-7-pareview/sites/all/modules/pareview_temp/test_candidate/logbook.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 7 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/logbook.module
--------------------------------------------------------------------------------
FOUND 62 ERROR(S) AND 4 WARNING(S) AFFECTING 37 LINE(S)
--------------------------------------------------------------------------------
   4 | ERROR   | The second line in the file doc comment must be " * @file"
  15 | ERROR   | Data type of return value is missing
  29 | WARNING | Line exceeds 80 characters; contains 83 characters
  64 | ERROR   | Data type of return value is missing
  86 | ERROR   | Expected 1 space before variable type
  86 | ERROR   | Doc comment for var redirection does not match actual variable
     |         | name $logbook_type at position 1
  86 | ERROR   | Parameter comment must be on the next line at position 1
  97 | ERROR   | Data type of return value is missing
 102 | ERROR   | There must be no blank line following an inline comment
 135 | WARNING | Line exceeds 80 characters; contains 84 characters
 157 | ERROR   | There must be no blank line following an inline comment
 172 | ERROR   | Missing comment for @return statement
 187 | ERROR   | Last parameter comment requires a blank newline after it
 187 | ERROR   | Missing comment for param "$form" at position 1
 188 | ERROR   | Missing comment for @return statement
 207 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 217 | ERROR   | Expected 1 space before variable type
 217 | ERROR   | Doc comment for var form does not match actual variable name
     |         | $form at position 1
 217 | ERROR   | Parameter comment must be on the next line at position 1
 219 | ERROR   | Last parameter comment requires a blank newline after it
 219 | ERROR   | Expected 1 space before variable type
 219 | ERROR   | Doc comment for var form_state does not match actual variable
     |         | name $form_state at position 2
 219 | ERROR   | Parameter comment must be on the next line at position 2
 224 | WARNING | Line exceeds 80 characters; contains 81 characters
 227 | ERROR   | There must be no blank line following an inline comment
 252 | ERROR   | Missing comment for param "$haystack" at position 1
 253 | ERROR   | Last parameter comment requires a blank newline after it
 253 | ERROR   | Missing comment for param "$needle" at position 2
 254 | ERROR   | Missing comment for @return statement
 270 | ERROR   | Data type of return value is missing
 323 | ERROR   | Data type of return value is missing
 333 | ERROR   | Expected 1 space before variable type
 333 | ERROR   | Doc comment for var path does not match actual variable name
     |         | $path at position 1
 333 | ERROR   | Parameter comment must be on the next line at position 1
 335 | ERROR   | Expected 1 space before variable type
 335 | ERROR   | Doc comment for var arguments. does not match actual variable
     |         | name $arg at position 2
 335 | ERROR   | Missing comment for param "arguments." at position 2
 338 | ERROR   | Data type of return value is missing
 343 | ERROR   | Concatenating translatable strings is not allowed, use
     |         | placeholders instead and only one string literal
 369 | ERROR   | Expected 1 space before variable type
 369 | ERROR   | Doc comment for var form does not match actual variable name
     |         | &$form at position 1
 369 | ERROR   | Parameter comment must be on the next line at position 1
 371 | ERROR   | Expected 1 space before variable type
 371 | ERROR   | Doc comment for var form_state does not match actual variable
     |         | name &$form_state at position 2
 371 | ERROR   | Parameter comment must be on the next line at position 2
 373 | ERROR   | Expected 1 space before variable type
 373 | ERROR   | Doc comment for var form does not match actual variable name
     |         | $form_id at position 3
 373 | ERROR   | Parameter comment must be on the next line at position 3
 381 | WARNING | Line exceeds 80 characters; contains 107 characters
 406 | ERROR   | Data type of return value is missing
 423 | ERROR   | Expected 1 space before variable type
 423 | ERROR   | Doc comment for var form does not match actual variable name
     |         | $form at position 1
 423 | ERROR   | Parameter comment must be on the next line at position 1
 425 | ERROR   | Expected 1 space before variable type
 425 | ERROR   | Doc comment for var form_state does not match actual variable
     |         | name &$form_state at position 2
 425 | ERROR   | Parameter comment must be on the next line at position 2
 441 | ERROR   | Expected 1 space before variable type
 441 | ERROR   | Doc comment for var role does not match actual variable name
     |         | $rid at position 1
 441 | ERROR   | Parameter comment must be on the next line at position 1
 443 | ERROR   | Expected 1 space before variable type
 443 | ERROR   | Doc comment for var users does not match actual variable name
     |         | $permission at position 2
 443 | ERROR   | Parameter comment must be on the next line at position 2
 457 | ERROR   | Expected 1 space before variable type
 457 | ERROR   | Doc comment for var permmission does not match actual variable
     |         | name $perm at position 1
 457 | ERROR   | Parameter comment must be on the next line at position 1
 469 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

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

Manual review:
- Add translation function for the title & description, for example:

    'title' => 'Logbook Entry',
    'title' => 'Logbook Configuration',
    'description' => 'Configure system settings for Logbook.',
    'title' => 'Edit',
	...

- Change to use the @ placeholder for t function

      '#title' => t($logbook['name'] . ' Permissions'),

I'm afraid you may have got some misleading advice regarding use of t().

t() should not be used on the 'title' and 'description' parameters within any function implementing hook_menu(), as these will automatically be run through t() by the menu system. However, t() should be left in place for 'title' and 'description' on $form items.

- Change to use the @ placeholder for t function

Because $logbook['name'] is a dynamic value which will differ for every logbook created, it is impossible for a translator to anticipate every possible value and provide a translation for it. Because of this, we want to let the translation system know that $logbook['name'] is a placeholder, and only translate the 'permissions' portion of the string. This is done with the following syntax:

'#title' => t("@logbook Permissions", array("@logbook" => $logbook['name'])),

The '@' symbol tells t() to run the value through check_plain() for sanitization ... we could also have used '!' to insert the variable as-is (assuming you've sanitized the value elsewhere), or '%' to indicate the variable should be html-escaped and processed with theme('placeholders', ...).

A full description of this pattern can be found at http://api.drupal.org/api/drupal/includes!common.inc/function/t/6

logbook_settings_permissions:

Can you confirm whether $logbook['type'] has been sanitized before being included as your hidden $form item values? (Not saying that it definately does, but I suspect you may need a check_plain() here ...)

Redundant code

359 $page = ""; ... Even if $page contained anything, it would be wiped out on by line 360.

Consider the more specific hook_form_FORM_ID_alter() instead of hook_form_alter

Since you only have one form_id you are checking in 379 function logbook_form_alter(&$form, &$form_state, $form_id), hook_form_FORM_ID_alter() may be a better choice. Also, can you get your content_type from an arg() parameter instead of $_GET?

Missing space

414     $result = db_query("SELECT * FROM {node_type} WHERE type LIKE 'logbook_%'ORDER BY name");

Makes me uneasy ...

Not saying there is an issue here, but I'd feel more comfortable with a regex check that made 100% sure the permission being inserted matches your one of your $logbook_permissions templates before substituting out the 'type' component and inserting directly into the database with 451 db_query("UPDATE {permission} SET perm = CONCAT(perm,', ','%s') WHERE rid = %d", $permission, $rid); ... just to make sure that you aren't somehow passed something like 'administer nodes'. Maybe redundant, but then again, I'm the paranoid type. :)

Review of the 6.x-2.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: ...l-7-pareview/sites/all/modules/pareview_temp/test_candidate/logbook.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 13 | ERROR | Additional blank line found at the end of doc comment
--------------------------------------------------------------------------------


FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/logbook.module
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 135 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
 277 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
 329 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

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

Removing review bonus tag, you have not done any manual review as required in #1410826: [META] Review bonus, you just posted the output of an automated review tool.

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. Same with your readme, you may have a look at the common styles for readme's (see guidelines for in-project documentation.)

(mostly 7.x-1.x but should also apply to 6.x-1.x)

1. package = Other Not necessary, if you provide no package information it'll be "Other" anyway
2. You may add your config page as "configure = " to the .info (only applies for d7)
3. t($perm) - never do this, it won't work properly anyway as only t('strings') (NOT t($variables)) will be detected and can be translated by the translation system on localize.drupal.org.
4. 'false', '4': Have a boolean? - make it a boolean 'false' -> FALSE ;; Have an integer? use it as integer '4' -> 4
5. check_plain('item') there is no need to sanitize hardcoded strings - you have to filter all user input not static stuff (please have a look at writing secure code chapter in documentation)
6. Your using hook_form_alter() just for altering one form, that's unnecessary overhead - whenever possible use hook_form_FORM_ID_alter()
7. 490: $form['submission']['body_label']['#default_value'] = 'Description'; you should keep t('Description') translatable here
8. logbook_settings_form(): your defining $destination within foreach() - if $logbook_types is an empty array, this variable will never be defnined and it'll throw an notice later for line 395.
9. theme_logbook_settings_form(): theme_table() will not sanitize the $rows contents for you and the content type title ($key) will be printed out raw, USE check_plain() here!

After I installed your module and goto logbook/add it says "Select which type of Logbook Entry you would like to make" - but I haven't created any yet, you should tell me to create a logbook type first and a proper link.
If no logbook was created yet, I get an empty table - why don't you provide a proper message like "No logbook type created yet. Create one now (link)" using the '#empty' property of theme_table ?

I also get Warning: Invalid argument supplied for foreach() in element_children() (line 6282 of /home/patrickd/www/drupal-7-test/includes/common.inc). because of your theming function (theme_logbook_settings_form()) see dblog.

Your printing out the operations without any separation and it'll look like "EditManage FieldsDelete".
Use something like

'data' => array(
          '#theme' => 'links__node_operations', 
          '#links' => $operations, 
          '#attributes' => array('class' => array('links', 'inline')),
        ),

here instead. (see theme_table documentation)

The security issue mentioned in point 9. is actually not possible, because machine readable names can't contain any harmful stuff - but by your "validation" you make it possible. In _logbook_validate_new() you rewrite the module's machine name with 'logbook' + human readable name! use the machine readable name here properly, so this isn't a security issue anylonger.

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

Code Review