Item10993: FORMFIELD does not read values from txt file

If the txt file is newer than the top revision in txt,v then FORMFIELD will read from txt,v, whereas QUERY does read from txt.

So

%FORMFIELD{"SomeFormfield" topic="SomeTopic"}%

then returns an old value while

%QUERY{"'SomeTopic'/SomeFormfield"}%

will correctly read the value botched into the txt file.

So FORMFIELD and QUERY do behave differently in this case which they shouldn't.

I do remember that we had a discussion about how to deal with the case that the top revision as checked in differs from the content of the txt file currently being checked out. As far as I remember we agreed that the current content of the txt file overrules the top revision as it is checked in to the VCS. QUERY has been implemented in that sense. FORMFIELD will behave buggy in this case.

Crawford posted a quick hack on IRC which seems to fix the obvious case (rev=undef):

Index: Foswiki/Render.pm
===================================================================
--- Foswiki/Render.pm   (revision 12134)
+++ Foswiki/Render.pm   (working copy)
@@ -940,6 +940,7 @@
     # this may have been a one-off optimisation.
     my $formTopicObject = $this->{ffCache}{ $topicObject->getPath() . $rev };
     unless ($formTopicObject) {
+   undef $rev unless $rev;
         $formTopicObject =
           Foswiki::Meta->load( $this->{session}, $topicObject->web,
             $topicObject->topic, $rev );
@@ -951,7 +952,7 @@
               Foswiki::Meta->new( $this->{session}, $topicObject->web,
                 $topicObject->topic, '' );
         }
-        $this->{ffCache}{ $formTopicObject->getPath() . $rev } =
+        $this->{ffCache}{ $formTopicObject->getPath() . ($rev||0) } =
           $formTopicObject;

However this will still return an old value:

%FORMFIELD{"SomeFormfield" topic="SomeTopic" rev="99999"}%

So while an undefined rev parameter now reads the txt file, specifying a rev > maxRev will still read from the VCS ... this is still wrong as we want to take the txt file as being the latest & greates content no matter if it is checked in or not.

-- MichaelDaum - 21 Jul 2011

I thought I recalled seeing this issue before. Item2387 FORMFIELD reads from rcs file ... should read txt file

-- GeorgeClark - 22 Jul 2011

"specifying a rev > maxRev will still read from the VCS" - yes of course, it has to. You can't know what the maxRev value is unless you read from the VCS. the .txt file cannot be trusted - in many cases the TOPICINFO is missing or, more dangerously, wrong. The .txt has to be thought os as a cache - I repeat, .txt cannot be trusted.

-- CrawfordCurrie - 22 Jul 2011

Note that even if the "cached" .txt file is treated as "the latest rev", in order to get the number of that revision correct you still have to read the ,v, as that is the only authoritative source of how many versions there are.And there's the rub; you end up having to read the ,v in order to discover that you should have read the .txt in the first place.

That might be a better model of behaviour, though. I'll try and run the unit tests with that, see what happens.

-- CrawfordCurrie - 27 Jul 2011

Reading the ,v file should only be a fallback to recover from a lost TOPICINFO in the txt file.

Wrt "trusting the .txt file". The "trust" is about as high as the core manages to takes care of any revision being checked in to be more sane than what is currently checked out. So the current working version (the .txt file) might have changes not being applied to the vcs. But imho this only applies to the TOPICINFO field, not the bulk load of the txt file itself.

And that's what we have to get right here in the first place for the users.

Point is: no matter what other VCS related problem - (including recovering TOPICINFO) there is, when the user says "get me the latest & greatest content" (as is the case for 99.9% of all requests to the store), then the bulk data for this up-to-date content has to be taken from the txt file, not from anywhere else. Read from the vcs only when rev ! = undef (and ! = 0), as the information isn't available otherwise of course.

-- MichaelDaum - 27 Jul 2011

I agree with Michael. Formfields must be read from the .txt file. Admins will often do text mode search and replace to the .txt file when for example som senior manager decides to change the name of a department and you suddenly have some text field in 2000 topics that need to be changed. Hacking .txt files is one of Foswiki's strengths and it has always worked fine in the past. If FORMFIELD suddenly does not show the value in a .txt file then we impose a major problem for our users.

-- KennethLavrsen - 30 Jul 2011

Not just hacking, it might be a vandalism repair, backup recovery, or a Foswiki upgrade. All valid cases where txt and txt,v are going to be out of sync.

-- ArthurClemens - 05 Aug 2011

Crawford - OK to pursue the TOPICINFO part of this but the biggest concern is FORMFIELD. It is not acceptable that the .txt field can have a form with a field with a value but the value shown by FORMFIELD is taken from an out-of-date ,v file.

Form field values should always be read from the .txt file.

I am personally less concerned about the TOPICINFO being wrong.

-- KennethLavrsen - 29 Aug 2011

You're missing the point. The FORMFIELD is wrong because the process for loading the topic is wrong. The TOPICINFO is part of that loading process, and while it isn't central to it, it needs to be considered in the tests.

Also, what I'm doing is deep and complex, and 1.1.4 shouldn't need to depend on it. Go ahead and hack a FORMFIELD solution for 1.1.4

-- CrawfordCurrie - 29 Aug 2011

I am not missing any points here. I am reading what you write. If you want us to receive a different message then please write the message you want us to receive.

I sure hope all core devs will ensure 1.1.4 does not end up as a hack.

-- KennethLavrsen - 29 Aug 2011

There are two choices here; a proper fix that sorts out the cache/history relationship once and for all, and fixes FORMFIELD into the bargain; or a quick hack to fix just FORMFIELD. The first is high risk, as it implies quite a few changes. The second is lower risk, but is a hack that repairs just this urgent report.

I moved the discussion on the deep fix to Item11091 - this task needs to focus on the repair for 1.1.4

-- CrawfordCurrie - 30 Aug 2011

I believe this should now be OK from the fixes for Item11091. However I have not generated a unit test (I had more than enough other tests to write). This report should remain open (and urgent) until someone has added one.

-- CrawfordCurrie - 11 Sep 2011

unit test added, and the problem it showed up fixed.

-- CrawfordCurrie - 21 Sep 2011

Commenting is disabled while not logged in