Cron Debug 1.2 for Drupal 7.x

Sandbox located here:
http://drupal.org/sandbox/vertikal.dk/1072626

Cron debug can run cron hooks while registering success and time elapsed for each. You can see which hooks will be run in which sequence, select which hooks to run and see their duration in the results as well as in the log.

If a cron process times out, hangs or fails, you can see which ones finished successfully and which one did not finish properly by looking in the log when returning to Drupal's reports. All cron debug log entries are registered as "cron debug" and can be filtered separately. If the cron run failed, the usurper will be the top/last entry in the list of cron debug entries in the log.

Notice that your regular cron jobs will run as usual if you have set them up. You might want to disable cron while debugging with cron debug. This module runs the cron jobs for each module separately from the usual cron run done in common.inc and invoked by cron.php.
Running cron.php will not register any debug code! You will have to run the cron debug routine to get this registration.

Also notice that
* some modules have local settings, which enable and disable cron runs for that module. In such cases it will appear here as if the hook ran even though the function in the module might return without having done anything. In order to debug cron jobs in such modules, you will have to tamper with the module's own settings.
* some custom modules may call external functions and not return properly to Drupal in which case they may run as planned, but not register as finished in the log. Disable those jobs in order to have a smooth cron run, and (hopefully) a successful return to Drupal.

Installation
Cron Debug can be installed like any other Drupal module -- place it in the modules directory for your site and enable it on the 'admin/build/modules' page.

The module also exists in a D6-version, which will be added once I learn how to maintain several versions of a module.

Why do I want to contribute a cron debug module to Drupal?
Well, because there is none, and the discussions about finding errors in cron runs are numerous and long, and solutions seem to employ all kinds of hacking and guesstimating. This module will put a bit of science into that equation and also deliver some hard evidence of slow routines or failures.
Like here http://drupal.org/cron and here http://drupal.org/node/123269 and many other places.

Who am I and what do I do?
I'm a professional Drupal developer in Denmark making my living developing in Drupal.
I develop full sites as well as custom modules and themes.
I contribute to the Drupal project with documentation and translation and also try to be active in the community by participating in events such as DrupalCons and local Drupal camps or days.

I have done presentations and talks at several such events including DrupalCon Copenhagen.
http://www.archive.org/details/AddAMobileVersionToYourDrupalSite

My work references are such as:
http://journalisten.dk/ - main site of the Danish Journalist's union.
http://pressensuddannelsesfond.dk/ - application handling for the Danish press fund.
http://specialmedierne.dk/ - web site for organization for Danish special media, magazines etc.
...plus many others

Hope that this makes the ball roll...

And for the sake of confusion I add to this post the latest versions of the module with a few additions and edits - amongst those a hook_uninstall().

Martin

CommentFileSizeAuthor
#14 1072660-14.patch20.58 KBbfroehle
#9 cron_debug-Use-tableselect.patch4.16 KBbfroehle

Comments

vertikal.dk’s picture

vertikal.dk commented

The last sentence about the newly added files is a leftover from my CVS-application posting (from where I copied the text). No files are attached here. The latest D7-version of the module is found in the sandbox.

Martin

bfroehle’s picture

bfroehle commented
Status: Active » Needs work

Hi Martin, I haven't reviewed many (any) applications, but I'll give yours a shot.

1. Missing t() in the implementation of hook_help().

2. Docblock recommendations in D7 are "Implements ..." instead of "Implementation of ...".

3. In 'page arguments' => array('cron_debug_form', $void),, what is the $void for?

4. Improper indentation in cron_debug_uninstall.

5. You can probably short circuit

if (variable_get('cron_debug_semaphore', FALSE)) {
    // Get the semaphore
    $result = variable_get('cron_debug_semaphore', 'unknown');

as

if ($result = variable_get('cron_debug_semaphore', FALSE))

.
Why aren't you using drupal's exisiting locking mechanisms?

6. 

    $result = db_query("SELECT info FROM {system} WHERE name = :name AND type IN (:type)", array(
      ':name' => $module,
      ':type' => array('module'),
    ));

should probably be replaced by a call to the (cached & alterable) system_rebuild_module_data().

7. The comments in cron_debug_form_submit() are overly pedantic.

vertikal.dk’s picture

vertikal.dk commented

Brad,

Thanks for the excellent feedback. This is my first attempt at contributing, so let's help each other!

I haven't done anything to the code yet, but let me just run through the items:

1) t() in help. Will be fixed

2) Will change "Implementation of..." to "Implements..."

3) $void argument is a leftover from an earlier stage. Will be removed.

4) Duh! Running coder routinely, but just not before last update. My bad.

5) Just checked and yes, I can use the shortcut. I though assigning a value is always returned TRUE... heh, more than 30 years of programming and still learning.

6) The function system_rebuild_module_data() returns a wealth of information and requires a bit more code to give me what little I need. But if it's "good latin" as we say here, it might be a good idea. I can see that most functions needing module information uses it, so I will try using it too.

7) Well, I personally love the step-by-step details, but I can cut down and cluster some of the lines to compact it a bit. Will do. You are after all the one who reviews me.

I will create a new version with the above editions and commit it later.

Martin

bfroehle’s picture

bfroehle commented

Great, set the status to "needs review" when you've made your changes. ;)

vertikal.dk’s picture

vertikal.dk commented
Status: Needs work » Needs review

Brad,

Made a few smaller changes a one bigger one (new feature, actually).

Fixed in the current version
--------------------------
Added t() in hook_help
Changed "Implementation of..." to "Implements..."
Removed surplus argument $void from page_arguments
Using function system_rebuild_module_data() rather than direct db access to get module info
Sanitized some code comments
Fixed some formatting issues
Removed some HTML from code and added it to a user editable string through t() at the same time making the module list and results more user controlled
Edited and formatted a few messages as well as help text to be more useful
Updated README.txt

Added in the current version
---------------------------
An option to run individual cron functions in full view enabling you to find programming and syntax errors in the single function.

Coder will warn you about line 266, but the string elements have been sanitized in spite of what coder says. Other solutions than concatenating the strings seem to either strip the formatting or render the HTML as HTML-characters rather than code.

Will now slowly close down for today and find my bed. It's past midnight here, and in spite of D7 being a hoot, a little sleep now and then isn't bad either.

Martin

bfroehle’s picture

bfroehle commented
Status: Needs review » Needs work

Hi Martin, Wow! Quick response. A few more things:

1. Don't hard code in URLs. In particular, not all sites are installed in the root directory of the web server so a link like /admin/.. won't work. Instead you can use the url() function and the string replacement feature of t(). See dblog_help() for some sample usage.

2. You can theme your Cancel button! See confirm_form() for sample code. Ditto with the reset button.

3. If you want to quickly get a list of selected modules, instead of

  foreach ($form_state['values']['cron'] as $key => $module) {
    if ($module != '0') {

you can build a list of enabled modules with

$modules = array_filter($form_state['values']['cron']);

or even

foreach (array_filter($form_state['values']['cron']) as $module)

which is a pretty common construction in Drupal. In general, array_filter() is a pretty handy PHP routine.

4. If you are adventurous, you could implement 

    $run_link = l(t('Run @function in full view.', array('@function' => $module . '_cron()')), 'admin/reports/cron_debug/run/' . $module);
    // Create the options for the checkboxes
    $crons[$module] = t('!num) <strong>!name</strong> - !note !run_link', array(
        '!num' => ++$num,
        '!name' => $modules_data[$module]->info['name'],
        '!note' => $note,
        '!run_link' => $run_link,
        ));

using the theme system. Also think if the use of '!' is appropriate. Substitutions with '@' might be more appropriate for the name.

vertikal.dk’s picture

vertikal.dk commented
Status: Needs work » Needs review

Brad,

And quick response from you too! Excellent feedback again.

1) The url's are now implemented using url()

2) Both the Cancel and Reset links (buttons) are now created using the form system's link type

3) Your construct is smart, but I currently use the else {} to add FALSE to the results returned to the form. This might not be needed, and then your method is simpler and more elegant. I will check it and change if appropriate.
I looked into it, and the FALSE value is used to set the cron hook as "not run" in the results. I will keep things as they are for now, but consider a different approach using your method.

4) I can't see the difference between what I do now and what you suggest...

I will commit a new version with the above changes and a few more:

a) Rephrased the term "in full view" to be "individually", which is more adequate since the form can also run cron hooks "in full view"

b) Changed form element weights for more clarity and better form construction

c) Changed the word semaphore to flag to avoid confusion with the Drupal semaphore system, which is mainly used to lock processes as described here http://api.drupal.org/api/drupal/includes--lock.inc/group/lock/7

d) Edited README.txt and added a CHANGELOG.txt -- don't know whether the latter is fully kosher, but it allows me to register my changes somewhere

Can you help with Git? I'm quite green there...
The Git instructions on the sandbox page say to push changes like this:

git diff origin/master
git push origin master

When I fire up the diff line I get a cacophony of beeps and have to shut down Git Bash altogether to exit the wealth of code lines that appear. What does the diff step do other than show me the differences (or try to)?

Maybe I should just be patient and wait for the Git-class tonight:
http://groups.drupal.org/node/130014

Martin

bfroehle’s picture

bfroehle commented
Assigned: Unassigned » bfroehle
bfroehle’s picture

bfroehle commented
Assigned: bfroehle » Unassigned
Status: Needs review » Needs work
StatusFileSize
new cron_debug-Use-tableselect.patch4.16 KB

Note the indentation, specifically on the closing ); line:

$form['buttons']=array(
  '#weight' => 30,
);

I've attached a patch which uses a 'tableselect' form item instead of a 'select' form item. Try it out and see what you think.

Overall it looks like you're almost there. :)

vertikal.dk’s picture

vertikal.dk commented

Brad,

Odd that you suggest the tableselect type! I was thinking about creating a table for the checkboxes. I have done that in D6 modules before, but there's a bit of work connected with that. I hadn't noticed the new select type in D7. Excellent! I will apply the patch and see how it works, but I'm sure it's close to what I wanted.

I'll also check the indentations... again.

Will commit a new version later.

Martin

vertikal.dk’s picture

vertikal.dk commented

Brad,

Tableselect is the way to go! Works like a charm and looks good too.

I noticed two things in your patch: Cancel is gone and Reset is permanent.

I like Cancel and put it there because "my users" (clients etc.) often express doubt about how to cancel a Drupal form. Most of us know that we can just leave the form, click a link or whatnot, but to many users a proper Cancel button or link is a "safe escape". But it might be un-Drupal to put it there...

Reset doesn't make much sense before the routine has been run and the form has changed from the default since it just restores the default form by reloading the whole thing from scratch.

What's your opinion on these two links?

Martin

bfroehle’s picture

bfroehle commented

Hi Martin,

It's your decision. I didn't want to rethink the logic for "Reset" in light of the other changes, so I just replaced Cancel with it. Drupal generally only provides 'Cancel' buttons on warning pages for destructive operations "Are you really sure you want to delete user X?"

vertikal.dk’s picture

vertikal.dk commented
Status: Needs work » Needs review

Brad,

1) Implemented the full tableselect glory! Thanks for the patch.

2) Removed the Cancel link for good and made the Reset link dependent on the submit status.

3) Fiddled with the status column of the table/form to have it empty when the form is in its default state and only say "not run" when the form has been submitted and a hook hasn't been run.

4) Added yet another url() function to avoid hard coded url's anywhere.

5) Set all cron hooks to be run as default when entering the form at first or resetting it. This is the most probable scenario when running it for the first time.

Question... is it OK to have formatting such as strong in the code as found in line 126 (your code). Shouldn't it rather be in a t()'d string for the admin to edit or not formatted at all leaving it to CSS to do that?

Martin

bfroehle’s picture

bfroehle commented
StatusFileSize
new 1072660-14.patch20.58 KB

PHP Docblocks.

/**
 * Check the flag and remove it
 * @return string A note describing the result
 */

Should instead be (something like):

/**
 * Returns the result of the most recent Cron Debug run.
 *
 * Reads the 'cron_debug_flag' variable which is either:
 * - unset if the previous cron debug run was successful.
 * - the name of the module cron was last running, if the cron run was
 *   unsuccessful.
 *
 * The 'cron_debug_flag' is then reset.
 *
 * This information is formatted into a nice string and returned. If there
 * were any errors on the previous cron run, a message is displayed to the
 * user.
 *
 * @return
 *   A string describing the result of the previous cron debug run. If cron
 *   failed in a particular module, that is noted.
 */

Notes: break at 80 lines, no trailing spaces, periods at the end of sentences.

Other changes:
1. changed the URL to admin/config/system/cron/debug (see the additional menu item that needed to be created for this to work). I think this is better -- reports generally are only for reporting. This also puts cron debug in with the other cron stuff which is better for the end user.

2. reformatted hook_help(), and fixed some <p /> to instead be </p>.

3. Changed cron_debug_run() to output different messages based upon whether a module doesn't exist or doesn't implement cron.

4. Added a configure link in cron_debug.info which displays on the module page.

5. url substitution in t() is generally done with '@url', not ':url'. Probably doesn't matter at all.

6. hook_install(), hook_uninstall(), and hook_update_N() all belong in the .install file, not the .module file. This isn't very obvious in the docs -- perhaps you want to file an issue about that?

7. $Id$ is no longer needed in the Git era. Coder is wrong about those warnings.

See the attached patch! :)

bfroehle’s picture

bfroehle commented
Status: Needs review » Needs work 
+++ b/cron_debug.infoundefined
@@ -1,5 +1,5 @@
+configure = admin/config/system/cron/debug
\ No newline at end of file

Ooops, forgot a newline at the end of the file.

+++ b/cron_debug.moduleundefined
@@ -107,7 +101,20 @@ function cron_debug_form($form, &$form_state) {
+      drupal_set_message(t('Cron presumably failed while running the cron hook for module @module.'), array('@module' => $module));

There's an error in this line as well.. The array substitution should be for t().

Powered by Dreditor.

vertikal.dk’s picture

vertikal.dk commented

Brad,

Will patch and check your changes.

Smiled when I saw your suggestion for a dockblock for a single function remembering your remark on my pedantic comments in the first version! ;-)

Would you suggest similar detailed dockblocks for all custom (non-hook) functions or even all functions?

May finish this tonight, but will most likely look into it tomorrow.

Martin

vertikal.dk’s picture

vertikal.dk commented
Status: Needs work » Needs review

Brad,

Patched with your latest changes--thanks for the work on help, .install and cron_debug_run(). Excellent changes.

I removed the cron hook completely from the module. It was there for debugging the module and of no use to the average user in my opinion.

The configure link in .info -> admin/modules works fine.

Wondering why the link to the page/form itself disappeared on the help page? It was there it the last incarnation of the module. Now there's just a link to permissions.
Other help pages have links to all functions as well as permissions like admin/help/dblog or admin/help/path

Will commit the latest version in a minute.

Martin

bfroehle’s picture

bfroehle commented
Status: Needs review » Needs work

Three final things:

1. You can fix the admin page help link by changing the menu item to be of type MENU_LOCAL_TASK | MENU_NORMAL_ITEM.

(The actual code that generates the links is system_get_module_admin_tasks() -- you can read through it to see how the list is generated.)

The comment module is a nice example of this -- notice admin/help/comment links to a tab admin/content/comment.

You probably want to change the title of the link to something like 'Debug cron' so that it appears nicely in both the tab and the help link.

2. Break the lines in README.txt at 80 characters.

3. Remove the $Id$ line at the top of cron_debug.info.

vertikal.dk’s picture

vertikal.dk commented
Status: Needs work » Needs review

Brad,

1) Fixed the menu to include MENU_NORMAL_ITEM. Had already renamed the menu item, so great idea! ;-)

2) Fixed README

3) Removed all id tags as recommended here http://groups.drupal.org/node/100999

Martin

bfroehle’s picture

bfroehle commented
Status: Needs review » Reviewed & tested by the community

Martin:

I think you've contributed a useful module. The coding style matches Drupal, and your functions include documentation blocks. Your code uses safe practices (check_plain, t, etc) when it comes to presenting user provided information. You've demonstrated knowledge of the Form API, and implement many standard hooks (hook_permission, hook_help, hook_menu, etc).

You have responded quickly to inquiries and generally want to contribute.

I'm going ahead and marking this as Reviewed and Tested By the Community.

-Brad

P.S. Some final UI comments:
* I'd remove all mentions of the module short names, except in the event of errors. Just hide them and display links like "Run individually." and replace text like

cron_system() run.
Time elapsed was 0.186 seconds.

with Successfully completed in 0.186 seconds. The row of the table already tells you which module it is.

vertikal.dk’s picture

vertikal.dk commented

Brad,

Here's what I did:
1) Removed function name from form links and results list

2) Added separate watchdog message with specified result so that the log message is clear

3) Expanded on-screen help with separate mention of server cron and internal cron and added link to cron setup for convenience.

4) Changed log variable names for clarity and to avoid namespace conflicts.

5) Changed wording, formatting and severity level of messages displayed if the run failed: error for the fail message, neutral for the information about the log. These were opposite before: error on the information and neutral on the error. Duh!

6) I edited a couple of docblocks for more clarity and correct formatting.

Committed these changes 5 minutes ago.

PS: The $id issue in Coder has been posted, although not by me: http://drupal.org/node/990940

Martin

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Wondering if this duplicates the functionality at http://drupal.org/project/supercron at all?

bfroehle’s picture

bfroehle commented

Dave Reid: Yes, it does duplicate part of the functionality:

Call hooks individually on demand (great for identifying problems)

Measure the time it takes for a cron hook to execute (we display the last call timings and the average timings)

One key difference is this was written for D7 and SuperCron is stuck at D6 #1055444: D7CX:

We purposefully didn't make that commitment; there is still quite a lot of work left to get 2.x stable under D6, and especially under multisite environments. Once it works well, D7 will be on the drawing board.

vertikal.dk’s picture

vertikal.dk commented

Dave Reid,

Indeed there are overlaps, but this module is not an über cron module meant to be a replacement or even a supplement to the ordinary cron features. This module was made solely to do quick and dirty debugging of cron routines and nothing else. Enable, debug. disable. I have personally needed such a module many times.

For my personal use SuperCron does way more than I need.

There are no plans to expand this module to do most of what SuperCron does.

And of course this is D7.

Martin

vertikal.dk’s picture

vertikal.dk commented

I know it's DrupalCon week and people are busy, but out of pure curiosity: what happens now? The project is RTBC as it seems to be called, but what's the next step?

The project is still a sandbox.

Does it require further action from me to get it on in the process?

According to this http://drupal.org/node/1068952 a Promote tab should occur on the sandbox project page, and I should become able to promote it. None of this is yet the case. Who or what makes this happen?

Martin

zzolo’s picture

zzolo commented
Status: Reviewed & tested by the community » Fixed

This review was done, in part, at the DrupalCon Chicago Code Review.

You have been approved. Thank you for your patience and dedication on this process. We look forward to your continuing contributions to Drupal and its community. Please read the following pages (and subpages) about Git usage and Drupal best practices:

The following points are things to consider going forward:

  • Though there are differences between this module and supercron. The community would benefit from a combined project. Maybe the combined project just has the debug module in it, so users don't have to use all the functionality at once.

Thank you to the following people for helping with this review:

  • @kiamlaluno
  • @bfroehle

--
Please be patient with the Full Project Application Review process as it is done by volunteers, we understand that this process is not the most efficient. The goal of the process is to ensure that contributions to the Drupal community remain valuable, and to help applicants build their skills as contributors.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

PA robot’s picture

PA robot commented
Component: new project application » module
Status: Closed (fixed) » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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