You are here: Foswiki>Tasks Web>Item5453 (08 Jan 2009, KwangErnLiew)Edit Attach

Item5453: value of "0" improperly handled as param to ENCODE and SPACEOUT

pencil
Priority: Normal
Current State: Closed
Released In: 1.0.0
Target Release: patch
Applies To: Engine
Component:
Branches:
Reported By: TWiki:Main.MarkAdelsberger
Waiting For:
Last Change By: KwangErnLiew
Some variables do not work properly if the value given as an argument is (or expands to) the string "0".

For example, % ENCODE{"% FORMFIELD{"myField"} %" type="..."} % renders as an empty string if myField has the value "0". A more direct example:

  • 0 - %ENCODE{"0" type="entity"}%

This is because Perl considers "0" equivalent to 0 and logically false. So it appears that this code (taken from lib/TWiki.pm around line 3060) replaces the input parameter of "0" with the default value "":

my $text = $params->{_DEFAULT} || '';

Not sure how common this is in the codebase; have only encountered/confirmed the error with ENCODE so far. This is a significant problem as some of my forms have numeric fields or other fields that can reasonably (even commonly) assume the value "0", and a workaround in the wiki markup is possible but not easy (and maybe not efficient).

-- TWiki:Main/MarkAdelsberger - 18 Mar 2008

I'm looking into this - unit tests etc. for v.1.0

oh maybe, maybe not. as Mark points out, this is all over foswiki code - ENCODE is just one trivial example.

what we should be doing, is some -if defined= variation of

my $text = "$params->{_DEFAULT}";

I have ocmmited some unit tests that can be inverted to show the issue, but i don't dare work on it for foswik v1.0 - it may be pretty life changing

I think this one deserves to addressed for 1.0 even if it is not a small partial fix. At least to solve the reporters problem.

I would like the fixes in the release note and it would be wrong to close this generic report. So I keep this open for more fixes after 1.0.0 and for unit tests. For now I use the text book fix for this

$a = defined ( $b ) ? $b : '';

and even though unit tests are good for this I just put a small comment before to warn developers against reverting to the simpler form (that cannot cope with zero correctly)

  • Item569 - ENCODE turns the value 0 into empty string '' DONE
  • Item570 - SPACEOUT fails if value or separator are the string 0 DONE

Looking at the macros we have and just focusing on their main input - I find that we may want to look more at SEARCH and METASEARCH with the search string 0. But many other macros that may fail with a "0" would never in any sensible way get a "0". Topic names, web names, user names, login names etc etc are always something else than the string "0". Same with format strings in macros that have a format attribute. I cannot see a format being "0" making sense.

So I think it is the searches that are worth studying more deeply now. At least ENCODE and SPACEOUT are OK now in 1.0.0

-- KennethLavrsen - 25 Dec 2008

Unit test for ENCODE altered to pass with the fixed ENCODE.

To do - make a unit test for SPACEOUT. There is none.

It's a big job to identify where parameters are incorrectly expanded to the empty string, but it has to be done. Also, all boolean parameters need to be examined and modified to use Foswiki::isTrue. Note that the solution proposed above won't work; "$params->{_DEFAULT}" will die if $params->{_DEFAULT} is undef.

Confirmed.

I assume you talk about the very first solution.

the

$a = defined ( $b ) ? $b : '';

works

It does not die. The defined function is made for this and I have confirmed that it works where I used it.

I am not sure we have many severe places where it is a problem. I have cured the most urgent. The one to look at is SEARCH and METASEARCH. But we risk breaking stuff by changing behavior in SEARCH so I want to defer that till after 1.0.0 release.

Mark's problem is solved in 1.0.0

I think we should set this one Waiting For Release and change the headline and open a new Normal bug that addresses SEARCH and METASEARCH. Other macros we have that take topic names, web names etc etc may also be affected but not in any practical way because you never use these functions this way. I spent an hour going through the macros we have and I think we are OK with the fixes I made. Even the IF has recently been addressed before the fork.

-- KennethLavrsen - 03 Jan 2009

ItemTemplate edit

Summary value of "0" improperly handled as param to ENCODE and SPACEOUT
ReportedBy TWiki:Main.MarkAdelsberger
Codebase trunk
SVN Range TWiki-5.0.0, Sun, 09 Mar 2008, build 16496
AppliesTo Engine
Component
Priority Normal
CurrentState Closed
WaitingFor
Checkins distro:9d4c49d9e2dd distro:6e22e46cef12 distro:1b339130bfa6
TargetRelease patch
ReleasedIn 1.0.0
Topic revision: r13 - 08 Jan 2009, KwangErnLiew
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