You are here: Foswiki>Tasks Web>Item12362 (05 Jul 2015, GeorgeClark)Edit Attach

Item12362: parseTime can't handle seconds in a Foswiki format time

pencil
Priority: Enhancement
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: FoswikiTime
Branches: trunk
Reported By: GeorgeClark TimotheLitt
Waiting For:
Last Change By: GeorgeClark
The default Foswiki time is defined as

Default Foswiki format
  • 31 Dec 2001 - 23:59
  • 31-Dec-2001 - 23:59

However if called parseTime is called with seconds 31 Dec 2001 - 23:59:23, the seconds are truncated and processed as zero seconds.

Some applications may need resolution to seconds, so enhance parseTime to honor the seconds if supplied, otherwise default to zero seconds.

And a small unrelated issue. If Foswiki::Time is used from Configure, it loads Foswiki.pm and another copy of the configuration.

TimotheLitt provided a patch:

Index: lib/Foswiki/Time.pm
===================================================================
--- lib/Foswiki/Time.pm    (revision 16032)
+++ lib/Foswiki/Time.pm    (working copy)
@@ -34,7 +34,19 @@
 use warnings;
 
 use Assert;
-use Foswiki ();
+
+# In some environments, e.g. configure, we do NOT want Foswiki.pm
+# use Foswiki::Time qw/-nofoswiki/ for that.  Since this module
+# doesn't use Exporter, we don't need anything complicated.
+
+sub import {
+    my $class = shift;
+
+    unless( @_ && $_[0] eq '-nofoswiki' ) {
+        require Foswiki;
+    }
+}
+
 use POSIX qw( strftime );
 
 # Constants
@@ -124,7 +136,9 @@
     # try "31 Dec 2001 - 23:59"  (Foswiki date)
     # or "31 Dec 2001"
     #TODO: allow /.: too
-    if ( $date =~ /(\d+)[-\s]+([a-z]{3})[-\s]+(\d+)(?:[-\s]+(\d+):(\d+))?/i ) {
+    if ( $date =~
+        /(\d+)[-\s]+([a-z]{3})[-\s]+(\d+)(?:[-\s]+(\d+):(\d+)(?::(\d+))?)?/i )
+    {
         my $year = $3;
 
         #$year -= 1900 if ( $year > 1900 );
@@ -134,7 +148,7 @@
 
         #TODO: %MON2NUM needs to be updated to use i8n
         #TODO: and should really work for long form of the month name too.
-        return &$timelocal( 0, $5 || 0, $4 || 0, $1, $mon, $year );
+        return &$timelocal( $6 || 0, $5 || 0, $4 || 0, $1, $mon, $year );
     }
 
     # ISO date 2001-12-31T23:59:59+01:00

-- GeorgeClark - 20 Jan 2013

Babar, care to review the solution for "Use Foswiki"

-- GeorgeClark - 20 Jan 2013

Note that the scheme for telling Time whether or not to load Foswiki was Babar's idea, passed on to me by George.

I just did the coding. Can't take credit for someone else's idea.

The change for parsing seconds is required for the Log viewer implementation, and is also consumed by the (new) DATE item checker in Configure.

Both require the -nofoswiki change - or we introduce a dependency on some CPAN date/time parsing modules as we can't tolerate what loading Foswiki does to Configure..

Obviously, I support this change....

However, it does raise another issue:

Foswiki::Time is loading all of Foswiki (roughly 83 MODULES) to get two %cfg key values.

There ought to be a better way.

Here is some data:

    perl -de1
    [snip]
    DB<1>  x scalar keys %INC
    0  24
    DB<2> require "/home/litt/wikisvn/foswiki/trunk/core/bin/setlib.cfg"
    DB<3>  x scalar keys %INC
    0  28
    DB<4> use Foswiki

    DB<5>  x scalar keys %INC
    0  107               <<<--- 107 - 24 = 83 MODULES loaded
    DB<6>
    ------------For example:  
    DB<1> require "/home/litt/wikisvn/foswiki/trunk/core/bin/setlib.cfg"
    DB<2> require "LocalSite.cfg"
    DB<3>  x scalar keys %INC
    0  29                 <<<--- 29 - 24 = 5 MODULES (LocalSite accounts for 2, setlib for LocalLib.cfg, Cwd and itself.  The debugger 1.)
    DB<3> x $Foswiki::cfg{DisplayTimeValues}, $Foswiki::cfg{DefaultDateFormat}
    0  'gmtime'
    1  '$day $month $year'

So while my patch provides exactly the previous behavior, I don't think that behavior is optimal!

-- TimotheLitt - 20 Jan 2013

I fully agree with your analysis. There ought to be a better way to simply load the configuration. I'm wondering if a simple thing like the following would work:
diff --git a/core/lib/Foswiki/Time.pm b/core/lib/Foswiki/Time.pm
index 57517c1..8b118b9 100644
--- a/core/lib/Foswiki/Time.pm
+++ b/core/lib/Foswiki/Time.pm
@@ -34,7 +34,9 @@ use strict;
 use warnings;
 
 use Assert;
-use Foswiki ();
+use Foswiki::Configure::Load ();
+Foswiki::Configure::Load::readConfig();
 
 # Constants
 our @ISOMONTH = (

The TimeTests unit tests all pass like this, but I'd like peer review to tell me if I'm insane.

-- OlivierRaginel - 20 Jan 2013

Whatever is done to read the configuration has to be conditional.

Re-reading the configuration under Configure won't work. (That is, it WILL work, but has VERY bad side-effects.)

So if you want to put Foswiki::Configure::Load and readConfig in an import method, that would be OK. E.g.
+sub import {
+    my $class = shift;
+
+    unless( @_ && $_[0] eq '-nofoswiki' ) {
+        require Foswiki::Configure::Load;           # use can't be conditional, and Load doesn't export any symbols. 
+                                                    # It does use Configure, but Time doesn't depend on that to compile.
+        Foswiki::Configure::Load::readConfig(1,1);  # or 0, 1 if you think expansion is required
+    }
+}
+

It certainly would reduce the overhead, though in this particular case, it's still doing work that is completely unnecessary.

These items will not be expanded, and reading the spec files for defaults is also not likely to be productive - the use case that was passed-on to me was stand-alone applications like RSS feed. If they are active, certainly at least one configuration save has been done. And in that case, defaults from the spec files won't happen.

Although it's not elegant, just do "LocalSite.cfg"; in an import is probably the minimal solution. (LSC is never require ed.) We could wrap it in a Foswiki::Cfg module for a touch of modesty... (require Foswiki::Cfg;  package Foswiki::Cfg; do "LocalSite.cfg"; 1;)

P.S. I'm qualified on the matter of whether Perl code is sane - but as for humans, that's beyond my competence smile I think the rule of thumb is that if you're asking the question, you're probably OK. But that's not official - if I were competent in that field, I'd have to charge a lot more. smile smile

-- TimotheLitt - 20 Jan 2013

From what I could read in the code, the use won't really do much, as for both our use-cases (configure and Foswiki), they've already been used. The readConfig will ensure the configuration is only read once, which is why I didn't make it conditionnal. There is a parameter in %Foswiki::cfg which is set to 1 first time readConfig is run:
 $Foswiki::cfg{ConfigurationFinished} = 1;
So my guess was that it was enough to ensure it wouldn't be read again. But Timothe, please try it in your use-case. I've uploaded it to github so ease testing and code sharing: https://github.com/foswiki/foswiki/tree/Item12362-FoswikiTime I'm sorry I branched Release01x01, but the changes should be identical in trunk.

-- OlivierRaginel - 20 Jan 2013

Right. Top-level was stupid. Putting the readConfig inside the formatTime function should do the trick.

-- OlivierRaginel - 20 Jan 2013

I'm confused. If your applications already use Foswiki or otherwise load %cfg ("should be a no-op" in your code), then the simplest thing to do is to simply remove the use Foswiki and replace it with nothing - as George orginally suggested.

I do not want to rely on a flag in the %cfg hash. Why do unnecessary work?

One example of corruption is that readConfig will (by default) expand all $Foswki::cfg items - and Configure won't work. It requires the unexpanded data. Another is that it postpones loading modules until they are required. So re-reading the config later (like in the DATE checker) will step on setup, but un-saved values.

My current users have this bit of paranoia:
use Foswiki::Time qw/-nofoswiki/;

BEGIN {
    die "Bad version of Foswiki::Time" if ( exists $INC{'Foswiki.pm'} );
}

Also, note that I use both parseTime and formatTime in configure. Loading %cfg only in formatTime doesn't solve my problem.

Bottom line: I like some variant of your original idea.

If we can make your use case more efficient, that's a bonus. But not required.

I think the best approach is my last suggestion:

Add a Foswiki::Cfg module for lightweight users of %cfg, and let it do "LocalSite.cfg"; This won't default or expand values, but that's OK for Time, and any other post-install comsumers. It's a lot cheaper than the current approach, but it encapsulates where the data is stored - that's good for our future sanity.

Time can do that conditionally in an import sub. And we're both happy.

-- TimotheLitt - 20 Jan 2013

What happens if something that configure calls also "use"s Foswiki::Time.

As all of the Logger classes contain "use Foswiki::Time", if configure issues "use Foswiki::Logger", wont' that defeat the purpose of the qw/-nofoswiki/? Or is it the "first use" that counts?

-- GeorgeClark - 20 Jan 2013

Logger would also use Time qw/-nofoswiki/

The only reason for Time to load %cfg is if it finds itself in some environment where it's not been done by a caller.

I'm not conviced (based on Babar's comments above) that such an environment exists. So your original thought of just removing the 'use Foswiki' and not replacing it seems to have merit.

If some random user breaks, they can just use Foswiki themselves and we're done.

But if we really have to support that use case, use -nofoswiki seems the most straightforward kludge.

The secondary question is what the no -nofoswiki (default) case does.

The old thing? Well, that's no worse than today. But I did notice just how expensive it is.

Something different? I like a Foswiki::Cfg wrapper that only contracts to provide unexpanded data. It solves today's problem, encapsulates the solution, is very lightweight -- not too general or over-engineered.

If we were starting from scratch, I'd say that Time should NEVER be loading the config. That's the callers job. Or maybe a method in Time for setTimeDefaults($a, $b)

But it's too late for that, so we have to adapt.

-- TimotheLitt - 20 Jan 2013

So, trying to answer Timothe's remarks:
  • I'm not doing any un-necessary work. This flag is already there. readConfig puts it there. The test can be moved inside Foswiki::Time if people are concerned about speed.
  • parseTime does not require anything in %Foswiki::cfg, which is why I only put the call inside formatTime, which is the only part of Foswiki::Time requiring %Foswiki::cfg.
  • If the application already "use"s Foswiki, then the use Foswiki::Configure::Load will be a no-op, and each call of formatTime will have an additional sub-routine call, which will test the cfg value, and return. If we want to optimize, see above.
  • I thought about doing a Foswiki::Cfg wrapper, but then I realized we would most certainly end up shooting ourselves in the foot, as either we would have to use readConfig without the check of $Foswiki::cfg{ConfigurationFinished};, or patch it to bypass that when using this module. (imagine Foswiki::Time using that module right now, using the default readConfig, then most likely it will end up filling up %Foswiki::cfg with expanded values, and when configure will call readConfig again, to get the unexpanded ones, it will simply return)

Bottom line is, I think what I wrote on github makes sense, and we might want to optimize the test inside formatTime to save some cycles. But it should fit Timothe's requirements, and shouldn't break anything (at least no use-case I can think of)

-- OlivierRaginel - 21 Jan 2013

Babar, your original idea, as implemented by Timothe, is a bit more efficient. Around 5% better compared to your solution on github.

In this code Foswiki::Time uses the import method to check for qw/-nofoswiki/, and Foswiki::TimeB is your code checked out from github.

use warnings;
use strict;
use Data::Dumper;


    print "COMPARING\n";
    use Benchmark qw( cmpthese ) ;
    our $i;
    cmpthese( -10, {
        a => sub{
            use Foswiki::Time;
            my $t = Foswiki::Time::formatTime($i+=1000);
        },
        b => sub{
            use Foswiki::TimeB;
            my $t = Foswiki::TimeB::formatTime($i+=1000);
        }
    } );

And the results:

COMPARING
     Rate   b   a
b 38788/s  -- -5%
a 40733/s  5%  --

-- GeorgeClark - 28 Jan 2013

I have no real objection, except what I think I stated above (that I'd rather do the right thing all the time rather than force the code to be changed).

Anyway, as I said the performances in my version could be improved by putting the check directly inside the formatTime, I've committed a new version. I didn't check Timothe's version, so I couldn't benchmark it. I would assume it recovers the 5%, because that's pretty much the only difference.

You cannot easily benchmark such code like you did, because the use Foswiki::Time; will be evaluated at compilation time, and therefore the only thing you will compare is the formatTime itself. I would argue that comparing the loading time isn't that important anyway, and would then hope that my last version is much faster.

But feel free to commit whichever version you like (even though both where my idea, I think I like my code better, but I can live with my other idea).

-- OlivierRaginel - 28 Jan 2013

Okay, latest version is ~1% difference. Yes, I realized that the use is only done once, and also moved it outside of the loops, but there was very little impact. The overhead obviously being extra instructions inside formatTime.

All this aside, I suspect that formatTime could be considerably improved in some places by reducing the repetitive regex scans where possible. For another task...

-- GeorgeClark - 28 Jan 2013

No work for > 1 year, trunk has been stable. Setting this waiting for release.

-- GeorgeClark - 02 Jun 2014
 

ItemTemplate edit

Summary parseTime can't handle seconds in a Foswiki format time
ReportedBy GeorgeClark TimotheLitt
Codebase trunk
SVN Range
AppliesTo Engine
Component FoswikiTime
Priority Enhancement
CurrentState Closed
WaitingFor
Checkins distro:d30a714da9ce
TargetRelease major
ReleasedIn 2.0.0
CheckinsOnBranches trunk
trunkCheckins distro:d30a714da9ce
Release01x01Checkins
Topic revision: r16 - 05 Jul 2015, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy