Posted by grobot on October 22, 2008 at 12:03am
| Project: | CiviCRM Error Handler |
| Version: | 5.x-1.1 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I've modified this module slightly to do two things:
- Alters the formatting somewhat, making it easier to scroll through yards and yards of debug malfeasance
- Dumps a copy of the debug output to watchdog (perhaps there should be a switch to choose between watchdog, email and both)
Feedback welcome, happy to keep improving this
Comments
#1
#2
Naughty comment line sneaked in!
Please have a look at this and give feedback; I'd like a chance to integrate an output switch and clean it up a bit before we have this accepted.
Cheers
#3
#4
This comment is to make public my invitation to xurizaemon to be my comaintainer of this module. This is to comply with all the formal requirements of applying for a CVS account under such circumstances.
#5
Wierd, how come I can't just set this to "Ready to be commited"?
#6
Improved patch which makes it possible to choose how errors are delivered from CiviCRM - email, watchdog, or both
#7
I wrote this module before coder.module became popular, and before I understood the importance of code style conformance, so take this comment with a grain of salt since civicrm_error.module does not pass the Drupal coding standards. But we should probably endeavor to not make things any worse.
There are several coding syntax issues with your patch:
* Comments without a trailing period.
* No space between trailing colon and the previous character.
* Missing "break;" in your final case.
* nested arrays should look like (note the indenting):
<?php$var = array(
'foo' => 1,
'bar' => 'one',
'baz' => array(
'yar' => TRUE,
),
);
?>
#8
cnw
#9
This patch cleans up civicrm_error module to pass coder.module without any warnings.
I didn't add any code cleanups in the original patch, as I figured it would be cleaner to supply those fixes as part of a separate issue rather than bundling them with this feature change.
The missing break in the second switch case is intentional; I've added an inline comment to clarify that.
Let me know if I can improve anything further?
Thx
#10
Wow, if we want to do a full audit of the code format that's great too. We probably shouldn't try to put anything else in this patch though ;-)
A few things I noticed. It looks like your last patch does not include the work in your previous patches. Not sure how you managed that. Perhaps with svn diff and you had already committed your previous change to svn? Try
cvs diff -ur BASEto get all your changes diffed against the version that you checked out of CVS.Also you shouldn't use {} on civicrm tables since if they get prefixed things will break badly.
Other than that it looks pretty good.
#11
This is a diff against the CVS version (rather than against my working copy - sorry bout that!)
Removed {} on civicrm tables
Thanks Dalin
#12
#13
Automatically closed -- issue fixed for 2 weeks with no activity.