[D6] Watchdog Cleaner

Thanks for the module
You're working on the master branch, it's not really good. You should create something like 6.x-1.x.

See this manual: http://drupal.org/node/1127732

watchdog_cleaner_menu()

You don't need to use t() function in title and description

watchdog_admin_form(), missed function arguments. Proper syntax is

watchdog_admin_form($form, &$form_state);

function period($id) {

}

This function name does not respect Drupal's namespace system. It's recommended to name your functions like MODULE_function_name() or _MODULE_function_name()

This is what the Coder module says:

FILE: watchdog_cleaner.module
--------------------------------------------------------------------------------
FOUND 51 ERROR(S) AND 5 WARNING(S) AFFECTING 39 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
4 | WARNING | Format should be "* Implements hook_foo()." or "Implements
| | hook_foo_BAR_ID_bar() for xyz_bar()."
6 | ERROR | Return comment must be on the next line
12 | ERROR | Do not use t() in hook_menu()
13 | ERROR | Do not use t() in hook_menu()
23 | WARNING | Format should be "* Implements hook_foo()." or "Implements
| | hook_foo_BAR_ID_bar() for xyz_bar()."
25 | ERROR | Superfluous doc comment at position 1
25 | ERROR | Parameter comment must be on the next line at position 1
26 | ERROR | Last parameter comment requires a blank newline after it
26 | ERROR | Superfluous doc comment at position 2
26 | ERROR | Missing parameter type at position 2
26 | ERROR | Parameter comment must be on the next line at position 2
27 | WARNING | Line exceeds 80 characters; contains 87 characters
27 | ERROR | Return comment must be on the next line
48 | WARNING | A comma should follow the last multiline array item. Found: )
49 | ERROR | Array indentation error, expected 4 spaces but found 2
78 | ERROR | Missing function doc comment
82 | ERROR | Missing function doc comment
88 | ERROR | Function comment short description must end with a full stop
90 | ERROR | Missing parameter type at position 1
90 | ERROR | Parameter comment must be on the next line at position 1
91 | ERROR | Missing parameter type at position 2
91 | ERROR | Parameter comment must be on the next line at position 2
98 | ERROR | Function comment short description must end with a full stop
100 | ERROR | Missing parameter type at position 1
100 | ERROR | Parameter comment must be on the next line at position 1
101 | ERROR | Missing parameter type at position 2
101 | ERROR | Parameter comment must be on the next line at position 2
112 | WARNING | Format should be "* Implements hook_foo()." or "Implements
| | hook_foo_BAR_ID_bar() for xyz_bar()."
115 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
118 | ERROR | Whitespace found at end of line
124 | ERROR | More than 2 empty lines are not allowed
125 | ERROR | More than 2 empty lines are not allowed
126 | ERROR | More than 2 empty lines are not allowed
127 | ERROR | Missing function doc comment
127 | ERROR | Expected 1 space between the closing parenthesis and the
| | opening brace; found 0
129 | ERROR | Blank lines are not allowed after CASE statements
129 | ERROR | Comments may not appear after statements.
131 | ERROR | BREAK statements must be followed by a single blank line
132 | ERROR | Blank lines are not allowed after CASE statements
132 | ERROR | Comments may not appear after statements.
134 | ERROR | BREAK statements must be followed by a single blank line
135 | ERROR | Blank lines are not allowed after CASE statements
135 | ERROR | Comments may not appear after statements.
137 | ERROR | BREAK statements must be followed by a single blank line
138 | ERROR | Blank lines are not allowed after CASE statements
138 | ERROR | Comments may not appear after statements.
140 | ERROR | BREAK statements must be followed by a single blank line
141 | ERROR | Blank lines are not allowed after CASE statements
141 | ERROR | Comments may not appear after statements.
143 | ERROR | BREAK statements must be followed by a single blank line
144 | ERROR | Blank lines are not allowed after CASE statements
144 | ERROR | Comments may not appear after statements.
146 | ERROR | BREAK statements must be followed by a single blank line
147 | ERROR | Blank lines are not allowed after DEFAULT statements
147 | ERROR | Comments may not appear after statements.
--------------------------------------------------------------------------------

Hello farnabaz,

You still have some issues, that you can check from:

http://ventral.org/pareview/httpgitdrupalorgsandboxfarnabaz1825046git-6x-1x

Easiest way is to fix the presented problems and recheck again using pareview.

You still have a master branch availlable in the repository.
You don't have any documentation. README.txt is missing, see the guidelines for in-project documentation.
Not all your function are correctly prefixed. They should as much as possible to start with the module name.

You have the following coding errors:
--------------------------------------------------------------------------------
FOUND 28 ERROR(S) AND 1 WARNING(S) AFFECTING 15 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
42 | WARNING | A comma should follow the last multiline array item. Found: )
63 | ERROR | Whitespace found at end of line
74 | ERROR | Function comment short description must end with a full stop
81 | ERROR | Function comment short description must end with a full stop
89 | ERROR | Function comment short description must end with a full stop
96 | ERROR | Function comment short description must end with a full stop
117 | ERROR | Function comment short description must start with a capital
| | letter
117 | ERROR | Function comment short description must end with a full stop
121 | ERROR | Blank lines are not allowed after CASE statements
121 | ERROR | BREAK statements must be followed by a single blank line
121 | ERROR | Closing brace must be on a line by itself
122 | ERROR | Blank lines are not allowed after CASE statements
122 | ERROR | BREAK statements must be followed by a single blank line
122 | ERROR | Closing brace must be on a line by itself
123 | ERROR | Blank lines are not allowed after CASE statements
123 | ERROR | BREAK statements must be followed by a single blank line
123 | ERROR | Closing brace must be on a line by itself
124 | ERROR | Blank lines are not allowed after CASE statements
124 | ERROR | BREAK statements must be followed by a single blank line
124 | ERROR | Closing brace must be on a line by itself
125 | ERROR | Blank lines are not allowed after CASE statements
125 | ERROR | BREAK statements must be followed by a single blank line
125 | ERROR | Closing brace must be on a line by itself
126 | ERROR | Blank lines are not allowed after CASE statements
126 | ERROR | BREAK statements must be followed by a single blank line
126 | ERROR | Closing brace must be on a line by itself
127 | ERROR | Blank lines are not allowed after DEFAULT statements
127 | ERROR | Closing brace must be on a line by itself
-------------------------------------------------------------------------------

Reviewing manually I did not found problems apart from english errors in:
42 t('1 day'), t('1 week'), t('1 month'), t('2 month'), t('4 month'), t('1 year')

A few things I've noticed:

1) You still have one error from ventral: http://ventral.org/pareview/httpgitdrupalorgsandboxfarnabaz1825046git

2) I might suggest that you make a watchdog entry after each watchdog clean happens (so leaving that as the first entry) just as record that watchdog was cleaned with your module.

3) I don't think "increasement" is a word, and I notice a few other grammatical and spelling issues. Misspelling on line 58 and I might suggest that line 44 be changed to:

'#description' => t('Clear logs older than the selected interval. Note that this means all logs older than the selected interval will be cleared with each cron run.'),

4) I might change line 37 to the following to make it more clear:

'#title' => t('Enable'),

As it is right now, it might imply that it's some other feature of the module - and not what the module is meant to provide.

5) On form save, you might want to do a drupal_set_message with confirmation of the settings, or a confirmation that X entries of watchdog were cleared when the clear button is set.

6) Interesting features that might be helpful for this module would be to allow only certain types of messages to be cleared. Or some log (in your own table) that says when watchdog was cleared and how many entries were cleared. Just some ideas.

7) You might want to consider giving this module its own permission rather than just the default site configuration permission.

Nice work!

Hi farnabaz,

Just a random thought since there is a lot of talk about avoiding duplicate module - Why is there a need for Watchdog Cleaner when logs can be cleared using Util Module http://drupal.org/project/util?

BTW the code looks clean and concise.

Hi,

1) for menu title add t() function
$items['admin/settings/wcleaner'] = array(
'title' => 'Watchdog Cleaner',

2) Remove the empty form validate function is not used
/**
* Validate watchdog_cleaner_admin form.
*/
function watchdog_cleaner_admin_form_validate($form, &$form_state) {

}

3) Line 125, Add t() function to the message
watchdog('watchdog_cleaner', 'All log entries older than selected interval has been cleared.');

4) Remove the empty hook_install from .install file is not in use

function watchdog_cleaner_install() {
}

Automated review found no errors, the module is clean.

I agree with all points of manual review by logan.H except for no 1 since I don't think there's a need for translated title.

As for the duplicate module raised by arjkap, http://drupal.org/project/util is buggy on Drupal 6 and not intuitive. I've just installed it with Drush quickly, couldn't get it running at all. Log reveals:

Referrer http://www.drupal6.dev/admin/settings/util
Message function_exists() expects parameter 1 to be string, array given in /Users/ain/NetBeansProjects/Drupal6/includes/form.inc on line 775.

It could of course be discussed if bugfixing should be superior to creating new modules, but I personally love the standalone module to clean up the logs so it's not a direct overlap of interests.

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 :-)

Hi farnabz,

Manual Review:

Thanks for sharing useful module to drupal community.This module works as per the expectation.Most of time of application DB size increased due to watchdog and cache.Anyway cache is important for the drupal application.

Only you have to remove the unwanted zip file in your module directory.

regards,
-Raj.

Remove the zip file from your repo and you will get an RTBC from me.

With regard to the Util module, this one does just 1 thing, although they do appear similar. As far as I can tell, the Util module does not use hook_cron. Together that's enough of a difference to make this one useful.

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

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