we seem to get stuck in pifr.review.inc all over the place, and often end up peppering the code with watchdog statements. i say we be proactive and do something like this:


define('PIFR_DEBUG', TRUE);

pifr_debug('this is right before CVS HEAD checkout');

function pifr_debug($message) {
  if (PIFR_DEBUG == TRUE) { 
    watchdog('pifr_debug', $message);
  }
}
CommentFileSizeAuthor
#10 pifr_debug.patch3.65 KBboombatower
#5 pifr_debug_5.patch4.1 KBaclight
#3 pifr_debug.patch3.15 KBboombatower
#2 pifr_debug.patch3.38 KBboombatower

Comments

boombatower’s picture

boombatower commented
Title: add massive optional debuggig statements to pifr.review.inc » add massive optional debugging statements to pifr.review.inc
Assigned: Unassigned » boombatower
Status: Active » Postponed (maintainer needs more info)

Should the debug setting be a configuration option or a constant. For easier debug it may be useful to have it as a setting?

I'll work on adding debug statements.

boombatower’s picture

boombatower commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new pifr_debug.patch3.38 KB

Here is an initial stab.

Let me know if you were looking for more detailed.

boombatower’s picture

boombatower commented
StatusFileSize
new pifr_debug.patch3.15 KB

White-space removed.

aclight’s picture

aclight commented
Status: Needs review » Needs work

I'd also recommend that you add something in pifr_review_send() so that it's easy to see from the slave what happened with testing of each patch.

I've added the following code to the version running on my slave, just after the first if {} block.

watchdog('pifr_result', 'Tested file id %file_id and got this result: %return', array('%file_id' => $file_id, '%return' => $return));
aclight’s picture

aclight commented
Status: Needs work » Needs review
StatusFileSize
new pifr_debug_5.patch4.1 KB

Attached patch adds watchdog information on the message that was returned to the PIFR server.

boombatower’s picture

boombatower commented
Status: Needs review » Needs work

Shouldn't that be a pifr_debug() statement? Perhaps there are other areas to convert to pifr_debug()

aclight’s picture

aclight commented

Actually, I think that the watchdog statement in pifr_review_send() should always be executed, regardless of whether or not debugging mode is enabled. Given that it's one line per test run, I don't see that this line will clutter up the watchdog log to a great degree.

aclight’s picture

aclight commented
Status: Needs work » Needs review

Although the severity of the watchdog call shouldn't be WATCHDOG_ERROR as it is in my patch above.

aclight’s picture

aclight commented
Status: Needs review » Needs work

Didn't mean to change status.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new pifr_debug.patch3.65 KB

Remove the WATCHDOG_ERROR, would like hunmonk to approve as he requested.

hunmonk’s picture

hunmonk commented

this is a good first attempt. the only question i have is: should PIFR_DEBUG be set to TRUE by default? i think it should be set to FALSE by default -- i believe this would be in line with most other debugging approaches i've seen. other than that it's ready to go.

going forward, i think it would be helpful to get more of these in other key places. we have a _lot_ of trouble in the HEAD install code, so having a bunch of these peppered throughout would be good. also, in some spots it might be nice to return some data to look at (i'm specifically thinking at the end of the HEAD installation, we could return a formatted $d testing object).

boombatower’s picture

boombatower commented
Status: Needs review » Fixed

Right I agree with you on DEBUG default...must have missed it.

Committed initial patch.

aclight’s picture

aclight commented

If we're going to disable debugging by default, I'd like to see this be an actual drupal setting and not a constant definition in the module's code. That way we can turn on and off debugging without patching the code locally, which I think will potentially lead to additional confusion, and also makes rolling patches for pifr slightly more difficult as it's more likely that debug = TRUE will creep into pifr code via an errant commit at some point.

boombatower’s picture

boombatower commented

Yea, the idea was debated in this issue. #344628: Add setting for to replace PIFR_DEBUG constant

Status: Fixed » Closed (fixed)

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