Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the CGI CPAN distribution.

Report information
The Basics
Id: 82364
Status: resolved
Priority: 0/
Queue: CGI

People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: (no value)
Fixed in: (no value)

Attachments


Subject: Patch - add noTimestamp option to CGI::Carp
Download (untitled) / with headers
text/plain 385b
Sometimes it is useful for CGI::Carp not to add a timestamp to error messages. The web server may add its own timestamp anyway, and you may prefer that any error output from the script should be identical from one run to the next. This patch adds a new 'noTimestamp' option or $NO_TIMESTAMP package variable. Please could you consider this feature for inclusion in the next version?
Subject: CGI.pm-3.63-noTimestamp.diff
diff -ru Changes Changes --- Changes 2012-11-14 23:43:13.000000000 +0000 +++ Changes 2013-01-02 13:47:52.543627766 +0000 @@ -1,3 +1,9 @@ +Version 3.64? Jan 2, 2013 + + [NEW FEATURES] + - If you specify the new option noTimestamp, CGI::Carp no longer adds a timestamp + to each line of error messages. + Version 3.63 Nov 12, 2012 [SECURITY] diff -ru lib/CGI/Carp.pm lib/CGI/Carp.pm --- lib/CGI/Carp.pm 2012-11-03 00:33:07.000000000 +0000 +++ lib/CGI/Carp.pm 2013-01-02 13:52:37.480195214 +0000 @@ -265,9 +265,24 @@ Note that this override doesn't happen until after the program has compiled, so any compile-time errors will still show up with the non-overridden program name - + +=head1 TURNING OFF TIMESTAMPS + +If your web server automatically adds a timestamp to each log line, +you may not need CGI::Carp to add its own. You can disable timestamping +by importing "noTimestamp": + + use CGI::Carp qw(noTimestamp); + +Alternatively you can set C<$CGI::Carp::NO_TIMESTAMP> to 1. + +Note that the name of the program is still automatically included in +the message. + =head1 CHANGE LOG +3.54? Added noTimestamp option + 3.51 Added $CGI::Carp::TO_BROWSER 1.29 Patch from Peter Whaite to fix the unfixable problem of CGI::Carp @@ -340,7 +355,7 @@ @ISA = qw(Exporter); @EXPORT = qw(confess croak carp); -@EXPORT_OK = qw(carpout fatalsToBrowser warningsToBrowser wrap set_message set_die_handler set_progname cluck ^name= die); +@EXPORT_OK = qw(carpout fatalsToBrowser warningsToBrowser wrap noTimestamp set_message set_die_handler set_progname cluck ^name= die); $main::SIG{__WARN__}=\&CGI::Carp::warn; @@ -348,7 +363,7 @@ $CGI::Carp::CUSTOM_MSG = undef; $CGI::Carp::DIE_HANDLER = undef; $CGI::Carp::TO_BROWSER = 1; - +$CGI::Carp::NO_TIMESTAMP = 0; # fancy import routine detects and handles 'errorWrap' specially. sub import { @@ -370,6 +385,7 @@ Exporter::import($pkg,keys %routines); $Exporter::ExportLevel = $oldlevel; $main::SIG{__DIE__} =\&CGI::Carp::die if $routines{'fatalsToBrowser'}; + $CGI::Carp::NO_TIMESTAMP = 1 if $routines{'noTimestamp'}; # $pkg->export('CORE::GLOBAL','die'); } @@ -385,7 +401,6 @@ } sub stamp { - my $time = scalar(localtime); my $frame = 0; my ($id,$pack,$file,$dev,$dirs); if (defined($CGI::Carp::PROGNAME)) { @@ -397,6 +412,8 @@ } until !$file; } ($dev,$dirs,$id) = File::Spec->splitpath($id); + return "$id: " if $CGI::Carp::NO_TIMESTAMP; + my $time = scalar(localtime); return "[$time] $id: "; } diff -ru t/carp.t t/carp.t --- t/carp.t 2011-01-24 03:20:03.000000000 +0000 +++ t/carp.t 2013-01-02 13:57:27.941730756 +0000 @@ -3,7 +3,7 @@ use strict; -use Test::More tests => 61; +use Test::More tests => 62; use IO::Handle; use CGI::Carp; @@ -85,6 +85,15 @@ "/] $id: There is a problem at $q_file line $expect_l.".'$/', "CGI::Carp::warn builds correct message"); +# Test $NO_TIMESTAMP +{ + local $CGI::Carp::NO_TIMESTAMP = 1; + $expect_l = __LINE__ + 1; + like(CGI::Carp::warn("There is a problem"), + qr/\A\Q$id: There is a problem at $file line $expect_l.\E\s*\z/, + "noTimestamp"); +} + # Test that _warn is called at the correct time $CGI::Carp::WARN = 1;
Subject: Re: [rt.cpan.org #82364] Patch - add noTimestamp option to CGI::Carp
Date: Wed, 02 Jan 2013 09:23:44 -0500
To: bug-CGI.pm [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Download (untitled) / with headers
text/plain 1.4k
Thanks for the contribution. I agree that it it could be a feature to not have timestamps added by CGI::Carp, but I'm unsure if the suggested way is the best way to go about it. Using global variables as an interface is not considered a great practice, can be problematic when multiple users of CGI::Carp expect different behaviors. Here's an alternative implementation to consider: use CGI::Carp: use CGI::Carp:noTimestamps; # The implementation in CGI::Carp::noTimestamps would look something like this: # Redefine the routine responsible for adding the timestamp no warnings; sub CGI::Carp::stamp { ... } An advantage of this approach is that it doesn't require people to upgrade the CGI.pm distro to use it... it also works with older versions. (Nor do you have wait for us to review and release, since you can release it yourself). A third option be something in between the two approaches proposed so far. CGI::Carp could be refactored so that a function call is used whose only purpose is decide whether to use timestamps or not. Inside stamp, we could call: addTimestamp() Which would be implemented like this: sub addTimestamp { 1 } An extremely minimal mix-in following the pattern above could override it. The downside to this approach is it requires the effort to update and release two distributions, and plus users all need to upgrade CGI.pm plus installing the new plugin as well. Mark
Download (untitled) / with headers
text/plain 294b
Thanks for your comments. In making the noTimestamp option I was following the existing practice of options like fatalsToBrowser, warningsToBrowser, and wrap. Since stamp() is an ordinary subroutine and not a method, it is not straightforward to override it in another package as you suggest.
Download (untitled) / with headers
text/plain 379b
Oh sorry, I see what you mean. The new module would override the stamp() method in the CGI::Carp namespace. That would work but the trouble is then you become dependent on the implementation of CGI::Carp. Either CGI::Carp would have to document the stamp() routine and how to override it, or the new module would risk being broken every time the internals of CGI::Carp change.
Download (untitled) / with headers
text/plain 650b
On Wed Jan 02 12:01:53 2013, EDAVIS wrote: Show quoted text
> Oh sorry, I see what you mean. The new module would override the > stamp() method in the CGI::Carp namespace. > > That would work but the trouble is then you become dependent on the > implementation of CGI::Carp. Either CGI::Carp would have to document > the stamp() routine and how to override it, or the new module would
risk Show quoted text
> being broken every time the internals of CGI::Carp change.
I wouldn't be too concerned about CGI::Carp changing. The module has reached maturity and is only being updated to fix critical issues. Meanwhile, a number of alternatives are increasingly being used instead.
Download (untitled) / with headers
text/plain 129b
OK, could you add a note to the CGI::Carp documentation noting that the CGI::Carp::stamp() routine can be overridden in this way?
Subject: Re: [rt.cpan.org #82364] Patch - add noTimestamp option to CGI::Carp
Date: Wed, 20 Feb 2013 08:04:36 -0500
To: bug-cgi.pm [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Download (untitled) / with headers
text/plain 577b
Show quoted text
> OK, could you add a note to the CGI::Carp documentation noting that the > CGI::Carp::stamp() routine can be overridden in this way?
Most every module in every function on CPAN can be overridden in this way. That's a feature of Perl rather than off this module. While unlike code, I do think it's appropriate for documentation to have some duplication, but I don't think this is place to highlight a broadly applicable programming technique. If you'd like this alternate functionality to be more easily available, I encourage to publish it as a CPAN module. Mark
Download (untitled) / with headers
text/plain 683b
On Wed Feb 20 08:04:53 2013, mark@summersault.com wrote: Show quoted text
>Most every module in every function on CPAN can be >overridden in this way. That's a feature of Perl >rather than off this module.
Yes, it's just that I do not want to do so without permission. Otherwise some future change to CGI::Carp would break my code, and it would be a bug in my code rather than on CGI::Carp's side because this access was never documented to start with. If you feel that overriding the routine is a good way to proceed, it's best to document that this is possible, so that a future CGI::Carp maintainer (you or anybody else) will know that this is something third party code is expecting to work.
Download (untitled) / with headers
text/plain 157b
... or your other suggestion of creating an addTimestamp call, to be overridden in a mixin, would work well too. Would you like me to make a patch for that?
RT-Send-CC: mark [...] summersault.com
On 2013-01-02 09:24:09, mark@summersault.com wrote: Show quoted text
> > Thanks for the contribution.
[...] I vote for applying EDAVIS' initial patch. The argument against global variables is not valid here; CGI::Carp is already heavily using global variables. Also the possible side-effects don't count here: careful users anyway use "local" on globals (and EDAVIS did it also in the patched test), and anyway cgi programs are mostly limited to one request, and anyway, this is about changing the appearance on a server, which would be the same for all programs on that system. The only thing I would amend is to add an example webserver to the Pod ("for example Apache2 with default ErrorLogFormat setting"). Maybe also an codified example $CGI::Carp::NO_TIMESTAMP = $ENV{SERVER_SOFTWARE} =~ m{^Apache/(\d+)} and $1 >= 2; Also, if there are alternatives to CGI::Carp, then it should be listed in SEE ALSO. I just found CGI::Carp::Throw, but it's not what I need here, as it seems to just handle fatalsToBrowser stuff in a smart way. Regards, Slaven
Would you like me to redo the patch with different pod documentation as per Slaven R.'s comment?
Subject: Re: [rt.cpan.org #82364] Patch - add noTimestamp option to CGI::Carp
Date: Fri, 07 Mar 2014 07:26:18 -0800
To: EDAVIS via RT <bug-CGI.pm [...] rt.cpan.org>
From: Mark Stosberg <mark [...] stosberg.com>
Download (untitled) / with headers
text/plain 129b
There are new maintainer(s) starting soon. One of them will confirm for you. Thanks for nudging this ticket along. Mark
Download (untitled) / with headers
text/plain 241b
This issue has been copied to: https://github.com/leejo/CGI.pm/issues/111 please take all future correspondence there. This ticket will remain open but please do not reply here. This ticket will be closed when the github issue is dealt with.
Patched, new release will go out within the next few days. Thanks! commit dd1262ddbdacd18fefc4e567698846e01763c9de Author: Lee Johnson <lee@givengain.ch> Date: Fri Jun 6 13:21:14 2014 +0200 resolve #111 [rt.cpan.org #82364] - carp timestamp in CGI::Carp can switched off by use of the import noTimestamp or by setting the package variable $CGI::Carp::NO_TIMESTAMP to 1. the alternative solutions to doing this, monkey patching the methods, are simple to do; but i am taking the path of least resistance in following the current conventions of these modules by adding the import switch / package variable update t/carp.t to test for the timestamp switch both by the use of the package variable and import option, along with perldoc in the CGI::Carp module to document the feature. tidy up some of the perldoc in CGI::Carp and remove the CHANGE LOG as these are in the Changes file so there is no point duplicating this information thanks to EDAVIS@cpan.org for the patch


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.