[D6] Lognode

Doesn't watchdog does that already?

change a node and then check the recent log messages under reports.

Also a link to sandbox is missing

ok, i see.
With this module you can filter log by content type and also find out who modified it.

Any chance porting this to 7? ( i know its irrelevant with review)

about reviewing it now:

- Remove $Id: tags.they are not needed for git
- Fill up your README.txt file
- Wrap your code to 80 chars
- Run it through Coder module set to minor and correct the errors

The applicant has been contacted to ask if the application is abandoned.

Hi,

Great that you want to resume working on this module.

Seems that you are working in the master branch, you should not do that, you should work in 7.x-1.x branch, see http://drupal.org/node/1127732.

You have som coding standards issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxwtouomi1254706git

When you have adressed this issues you could mark the application as need review.

Good stuff. I re-ran the PAReview.sh test script for you and it looks like you have the error you mentioned and one other:

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


FILE: ...es/all/modules/pareview_temp/test_candidate/templates/help_text.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 4 | ERROR | Missing file doc comment
--------------------------------------------------------------------------------

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

To solve the "Missing file doc comment", you should add what is described here: http://drupal.org/node/1354#files.

That being said, most template files, including those in core, that I've seen, don't include a file doc comment at the top...so I think you can safely ignore that warning.

To solve the 'newline' error, simply add a return at the end of your README.txt.

.tpl.php files that you have in there aren't really template files. They are just static pages of HTML markup. I don't think this is the "drupal way" of including markup like that. Also, you're placing the help_text.tpl.php content in the description of a fieldset, which is a bit confusing to me. It should probably be its own set of markup. Something like this (around 194 of your .module file):

 $form['lognode'] = array(
    '#type' => 'fieldset',
    '#title' => t('Log events'),
    '#collapse' => TRUE,
    '#collapsible' => TRUE,
  );
$form['lognode']['lognode_types'] = array(
  '#theme' => '#item_list'
  '#items' => array(
    "<strong>" . t('create') . "</strong>:" . t('log a node after creation'),
    "<strong>" . t('update') . "</strong>:" . t('log a node after update'),
    // ... etc .. etc... etc...
  ),
  '#title' => 'Log Events',
);
$form['lognode']['enable_lognode'] = array(
      '#type' => 'checkbox',
      '#title' => t('Enable logs on this content type'),
);

I haven't tested this code exactly to make sure it works. There may be a bug or two, but that's the idea. See http://www.vkareh.net/blog/render-arrays

Hi wilmar81,

http://drupalcode.org/sandbox/wtouomi/1254706.git/blob/refs/heads/6.x-1....
During install, you probably shouldn't save an empty string into the variables table. I think it should be ok if you just set it when the user sets the first node to track.

This was mentioned on my review by patrickd, so I'm passing it on:
Wrap translatable test in t(), also in hook_permission (but not in hook_menu or watchdog())

http://drupalcode.org/sandbox/wtouomi/1254706.git/blob/refs/heads/6.x-1....
Missing argument doc

Also - I'm not sure if this is the right way to do the help - in a .tpl.php file... But from previous comments looks like you're changing that around :)

Manual review:

Name

In the info file you name the module Lognodes, but the module name is lognode - you should choose on of them, or it could be confusing.

Help

You should not have the help in a tpl-file, and I agree with #9 on the subject.

Install hooks in .install

lognode_enable should be in the .install file, when it is a installation hook.

No t() in hook_menu()

All menu items is run though t() by default, so you should not use it for the menu items

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

added the link to sandbox