282748 - uninitialized value in localtime in Format.pm

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Since the unfreezing of the trunk on Jan 16, my apache log file is full of this
error message (the first time it appears is Jan 18):

Use of uninitialized value in localtime at
/usr/lib/perl5/vendor_perl/5.8.1/Date/Format.pm line 123.

This is related to the time2str() subroutine. I suspect bug 277618 or bug 103636
to be responsible for this.

I'm investigating.... :)

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Remove all "uninitialized value" error messages about Format.pm from the web
server log file. :)

The patch checks if str2time() is defined and does not call time2str() if it
isn't. Moreover, deadlines which are saved in the "bugs_activity" table now all
have the YYYY-MM-DD format. This way, there is no need to reformat them
everytime someone accesses show_activity.cgi or when mails are sent. I also
checked that the deadline format in email is correct.

A nice side effect is that it displays now '' instead of '1970-01-01' both in
the activity table and in mails when the deadline does not exist (I think there
is a bug # about this point, which is then fixed by this patch).

Comment 2

[Frédéric Buclin]

Note also that these error messages were due to bug 103636 which landed one
month ago (just after the 2.19.2 release)!

Comment 3

[Max Kanat-Alexander]

OK, so why do we have to check the return value of str2time? That doesn't make
sense -- shouldn't we always be getting a valid return value? We just shouldn't
be calling that code at all if we don't have a valid date...

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> OK, so why do we have to check the return value of str2time? That doesn't make
> sense -- shouldn't we always be getting a valid return value? We just shouldn't
> be calling that code at all if we don't have a valid date...

mkanat: how do you know if a string given by the user has a valid date format or
not? That's the job of str2time which parses the input and converts it to an
integer if the format is correct, else an undefined value is returned (I read
the docs!). As you say, we should not call time2str at all if the string is not
a valid date and that's what my patch does! You *have to* call str2time in order
to determine if the user input is correct (by looking at the returned value),
and then you call time2str *only if* str2time says it's OK! Actually, the code
does not check the returned value of str2time and IMO this check should be
added. This explains all the error messages we get in the apache log file!

So I'm *100%* convinced that what I do here is correct! ;)

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 174760 [details] [diff] [review]
patch, v1

>--- Bugzilla/BugMail.pm 16 Feb 2005 23:54:47 -0000      1.32
>+++ Bugzilla/BugMail.pm 19 Feb 2005 03:16:09 -0000
>-        if( $fieldname eq 'deadline' ) {
>-            $old = time2str("%Y-%m-%d", str2time($old));
>-            $new = time2str("%Y-%m-%d", str2time($new));
>-        }  

Why are you removing these?

>--- CGI.pl      8 Feb 2005 16:51:02 -0000       1.227
>+++ CGI.pl      19 Feb 2005 03:16:11 -0000
>-            if ($fieldname eq 'deadline') {
>-                $removed = time2str("%Y-%m-%d", str2time($removed));
>-                $added = time2str("%Y-%m-%d", str2time($added));
>-            }
>-        

...or these? This makes show_activity.cgi display the time along with the date.
Didn't test bugmail, but I expect it to behave similarly.

>--- process_bug.cgi     18 Feb 2005 16:14:26 -0000      1.236
>+++ process_bug.cgi     19 Feb 2005 03:16:17 -0000
>@@ -1761,7 +1761,17 @@
>+
>+            # Convert deadlines to the YYYY-MM-DD format. We use an
>+            # intermediate $xxxtime to prevent errors in the web
>+            # server log file when str2time($xxx) is undefined.
>+            if ($col eq 'deadline') {
>+                my $oldtime = str2time($old);
>+                $old = ($oldtime) ? time2str("%Y-%m-%d", $oldtime) : '';
>+                my $newtime = str2time($new);
>+                $new = ($newtime) ? time2str("%Y-%m-%d", $newtime) : '';
>+            }
>+
>             LogActivityEntry($id,$col,$old,$new,$whoid,$timestamp);

This would heal the show_activity.cgi issue, but for new entries only.
Pre-your-patch entries still need the str2time+time2str treatment.
I think the formatting for display should be done on output, not before writing
to the database. If we do it on output, we retain the (theoretical) chance to
do it in a localized way :)

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 174760 [details] [diff] [review]
patch, v1

Myk, per discussion with Fred:
This works for new entries (see comment 5), so we might choose to live with
"time-added" activity entries created in the time between bug 103636 and
checkin of this patch.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

What can be done to fix the corrupt values?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Per lpsolit in IRC the corruption isn't in any released version, so a=myk
without the corruption fixing.  I hope this does *not* lead developers to think
that they can be lenient with potentially data corrupting code as long as they
catch the problems before the next release--such a laissez faire attitude would
inevitably result in more data corruption bugs making it into actual releases
and hurt Bugzilla in the long run.

Comment 9

[Max Kanat-Alexander]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.229; previous revision: 1.228
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.238; previous revision: 1.237
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.34; previous revision: 1.33
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.22; previous revision: 1.21
done