Item13186: Configure breaks short URLs. "undefined" does not equal "null".

pencil
Priority: Urgent
Current State: Closed
Released In: n/a
Target Release:
Applies To: Engine
Component: Configure
Branches: master
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
The URI Checker defines (invoked by URLPATH checker)

  • # * nullok = allow item to be empty

This is in conflict with the Save wizard states:

  • # A null value and nullok will suppress the item in LSC

This causes entries that are "allowed to be empty", become undefined, which is not the same as defined but empty.

The side effect is that there is no way to get $Foswiki::cfg{ScriptUrlPaths}{view} = '' saved to the configuration. It is removed when the configuration is saved. If I remove the "nullok" option, then the value is saved, but the checker complains about being unable to parse the value.

So there are 3 issues
  1. URLPATH nullok means "empty" is ok.
  2. Wizards::Save::save() while processing the Set values assigns '' as undef.
  3. Wizards::Save::_generateLSC() omits any empty 'nullok' when writing the configuration

I'm not sure the best way to fix this, but as currently operating, it's impossible to configure "Short URLs" with 1.2.

I found this when I noticed that all the "Anchor" links on my site were resulting a redirect instead of just jumping to the anchor.

I believe that at least within the core, {ScriptUrlPaths}{view} is the only parameter that is interpreted differently as undef, empty, or configured.
  • {ScriptUrlPaths}{view} undefined means use {ScriptUrlPath} for the setting and ignore the "...Paths" setting.
  • {ScriptUrlPaths}{view} = '', means write the view urls without any path
  • {ScriptUrlPaths}{view} = '$Foswiki::cfg{ScriptUrlPath}/view$Foswiki::cfg{ScriptSuffix}'; is same as undefined.

-- GeorgeClark - 03 Jan 2015

Crawford,

I'm part way there with a patch adding "undefok" check option, along with "nullok" Bootstrap gets it right, and tools/configure sets it okay. But I can't figure out how to get the UI to recognize that {ScriptUrlPaths}{view} = ''; is empty, not undefined. With the below patch, I can get configure to save the parameter as a defined but empty field, but the UI still claims that empty == undefined. ... Got it. Added the UNDEFINEDOK spec setting back, but only for the above setting.

Damn no I didn't. It won't save ={ScriptUrlPaths}{view} = ''.

I also noted that quite a bit of Foswiki.spec were claiming nullok - for things like the Trash web name, etc. The code does not provide defaults, so I think they have to be specified. Also anything that bootstrap is eligible to set, needs to be # $Foswiki::cfg{} commented within the Spec file. Or the spec overrides the bootstrap. I had to comment out Script Suffix.

The below also fixes an issue where the password changes were no longer being logged.

I didn't want to check it in without your review. This undef & null stuff is rather challenging.

Okay ... one more try. The below patch now seems to work mostly ok. bootstrap is happy, I'm able to set short URLs. Email wizard works, Only minor issue ... when changing ScriptUrlPaths}{view} from undef to '', the change log reports the old default value from the Spec comment. I have no idea where or how it's picking it up. It's not set anywhere other than the spec comment. ... please review.

Patch behind the twisty:
diff --git a/core/lib/Foswiki.spec b/core/lib/Foswiki.spec
index ca7d3d6..1e88b4d 100644
--- a/core/lib/Foswiki.spec
+++ b/core/lib/Foswiki.spec
@@ -97,9 +97,9 @@ $Foswiki::cfg{PermittedRedirectHostUrls} = '';
 # **STRING 10**
 # Suffix of Foswiki CGI scripts. For example, .cgi or .pl.
 # You may need to set this if your webserver requires an extension.
-$Foswiki::cfg{ScriptSuffix} = '';
+# $Foswiki::cfg{ScriptSuffix} = '';
 
-# **URLPATH CHECK='nullok notrail' FEEDBACK="label='Verify';wizard='ScriptHash';method='verify';auth=1" **
+# **URLPATH UNDEFINEDOK CHECK='undefok nullok notrail' FEEDBACK="label='Verify';wizard='ScriptHash';method='verify';auth=1" **
 #! n.b. options should match Pluggables/SCRIPTHASH.pm for dynamic path items
 # This is the complete path used to access the Foswiki view script,
 # including any suffix.
@@ -182,13 +182,13 @@ $Foswiki::cfg{ScriptSuffix} = '';
 #      are pending verification.
 # $Foswiki::cfg{WorkingDir} = '/home/httpd/foswiki/working';
 
-# **PATH CHECK="nullok" EXPERT**
+# **PATH CHECK="undefok" EXPERT**
 # This is used to override the default system temporary file location.
 # Set this if you wish to have control over where working tmp files are
 # created.  It is normally set automatically in the code.
 # $Foswiki::cfg{TempfileDir} = '';
 
-# **PATH EXPERT CHECK='nullok'**
+# **PATH EXPERT CHECK='undefok'**
 # You can override the default PATH setting to control
 # where Foswiki looks for external programs, such as grep.
 # By restricting this path to just a few key
@@ -2098,17 +2098,17 @@ $Foswiki::cfg{MaxLSCBackups} = 10;
 # change it unless you are certain that you know what you are doing!)
 $Foswiki::cfg{SystemWebName} = 'System';
 
-# **STRING 20 CHECK='nullok' EXPERT**
+# **STRING 20 EXPERT**
 # Name of the web used as a trashcan (where deleted topics are moved)
 # If you change this setting, you must make sure the web exists.
 $Foswiki::cfg{TrashWebName} = 'Trash';
 
-# **STRING 20 CHECK='nullok' EXPERT**
+# **STRING 20 EXPERT**
 # Name of the web used as a scratchpad or temporary workarea for users to
 # experiment with Foswiki topics.
 $Foswiki::cfg{SandboxWebName} = 'Sandbox';
 
-# **STRING 20  CHECK='nullok' EXPERT**
+# **STRING 20 EXPERT**
 # Name of site-level preferences topic in the {SystemWebName} web.
 # *If you change this setting you will have to
 # use Foswiki and *manually* rename the existing topic.*
diff --git a/core/lib/Foswiki/Configure/Checker.pm b/core/lib/Foswiki/Configure/Checker.pm
index a5d15a6..7ff2bc2 100755
--- a/core/lib/Foswiki/Configure/Checker.pm
+++ b/core/lib/Foswiki/Configure/Checker.pm
@@ -388,7 +388,7 @@ sub showExpandedValue {
     }
     else {
         my $check = $this->{item}->{CHECK}->[0];
-        unless ( $check && $check->{nullok}[0] ) {
+        unless ( $check && $check->{undefok}[0] ) {
             $reporter->NOTE("$this->{item}->{keys} is undefined");
         }
     }
diff --git a/core/lib/Foswiki/Configure/Checkers/URL.pm b/core/lib/Foswiki/Configure/Checkers/URL.pm
index 95b15b5..b322050 100644
--- a/core/lib/Foswiki/Configure/Checkers/URL.pm
+++ b/core/lib/Foswiki/Configure/Checkers/URL.pm
@@ -16,6 +16,7 @@ our @ISA = ('Foswiki::Configure::Checker');
 # CHECK options:
 #    * expand = expand $Foswiki::cfg variables in value
 #    * nullok = allow item to be empty
+#    * undefok = allow item to be undefined
 #    * parts:scheme,authority,path,query,fragment
 #           Parts allowed in item
 #           Default: scheme,authority,path
@@ -63,6 +64,11 @@ sub checkURI {
     my ( $reporter, $uri, %checks ) = @_;
 
     unless ( defined $uri ) {
+        $reporter->ERROR("Not a valid URI") unless $checks{undefok}[0];
+        return;
+    }
+
+    if (  $uri eq '' ) {
         $reporter->ERROR("Not a valid URI") unless $checks{nullok}[0];
         return;
     }
@@ -78,8 +84,6 @@ sub checkURI {
 
     $uri =~ s/^\s*(.*?)\s*$/$1/ if defined $uri;
 
-    return if ( !( defined $uri && length($uri) ) && $checks{nullok}[0] );
-
     unless ( $uri =~ $uriRE ) {
         $reporter->ERROR("Syntax error: $uri is not a valid URI");
         return;
diff --git a/core/lib/Foswiki/Configure/Checkers/URLPATH.pm b/core/lib/Foswiki/Configure/Checkers/URLPATH.pm
index b6d9bf7..24fbe71 100644
--- a/core/lib/Foswiki/Configure/Checkers/URLPATH.pm
+++ b/core/lib/Foswiki/Configure/Checkers/URLPATH.pm
@@ -17,12 +17,20 @@ sub check_current_value {
     my $value = $this->{item}->getExpandedValue();
     if ( !defined $value ) {
         my $check = $this->{item}->{CHECK}->[0];
-        unless ( $check && $check->{nullok}[0] ) {
+        unless ( $check && $check->{undefok}[0] ) {
             $reporter->ERROR("Cannot be undefined");
         }
         return;
     }
 
+    if ( $value eq '' ) {
+        my $check = $this->{item}->{CHECK}->[0];
+        unless ( $check && $check->{nullok}[0] ) {
+            $reporter->ERROR("Cannot be empty");
+        }
+        return;
+    }
+
     my %check = ();
     if ( scalar( @{ $this->{item}->{CHECK} } ) > 0 ) {
         %check = %{ $this->{item}->{CHECK}->[0] };
diff --git a/core/lib/Foswiki/Configure/Pluggables/SCRIPTHASH.pm b/core/lib/Foswiki/Configure/Pluggables/SCRIPTHASH.pm
index 77db2f6..1ec6630 100644
--- a/core/lib/Foswiki/Configure/Pluggables/SCRIPTHASH.pm
+++ b/core/lib/Foswiki/Configure/Pluggables/SCRIPTHASH.pm
@@ -108,7 +108,7 @@ sub construct {
             # good idea - which it isn't. So don't.
             #default     => "'$default'",
             opts =>
-'FEEDBACK="label=\'Verify\';wizard=\'ScriptHash\';method=\'verify\';auth=1" CHECK="nullok notrail"',
+'UNDEFINEDOK FEEDBACK="label=\'Verify\';wizard=\'ScriptHash\';method=\'verify\';auth=1" CHECK="undefok nullok notrail"',
         );
 
         push( @$settings, $value );
diff --git a/core/lib/Foswiki/Configure/Query.pm b/core/lib/Foswiki/Configure/Query.pm
index 1d5d380..60453a9 100644
--- a/core/lib/Foswiki/Configure/Query.pm
+++ b/core/lib/Foswiki/Configure/Query.pm
@@ -41,7 +41,7 @@ sub _getSetParams {
     my ( $params, $root, $reporter ) = @_;
     if ( $params->{set} ) {
         while ( my ( $k, $v ) = each %{ $params->{set} } ) {
-            ($v) = $v =~ m/^(.*)$/s;    # UNTAINT
+            ($v) = $v =~ m/^(.*)$/s if defined $v;    # UNTAINT
             if ( defined $v && $v ne '' ) {
                 my $spec  = $root->getValueObject($k);
                 my $value = $v;
diff --git a/core/lib/Foswiki/Configure/Value.pm b/core/lib/Foswiki/Configure/Value.pm
index 731c5f1..a474f03 100755
--- a/core/lib/Foswiki/Configure/Value.pm
+++ b/core/lib/Foswiki/Configure/Value.pm
@@ -72,6 +72,7 @@ use constant ATTRSPEC => {
     DISPLAY_IF      => { openclose => 1 },
     ENABLE_IF       => { openclose => 1 },
     EXPERT          => {},
+    UNDEFINEDOK     => {},
     FEEDBACK        => { parse_val => '_FEEDBACK' },
     HIDDEN          => {},
     MULTIPLE        => {},         # Allow multiple select
@@ -91,7 +92,8 @@ our %CHECK_options = (
     max      => 1,     # max value
     min      => 1,     # min value
     notrail  => 0,     # ignore trailing / when checking URL
-    nullok   => 0,     # is undef OK?
+    nullok   => 0,     # is empty OK?
+    undefok  => 0,     # is undefined OK?
     parts    => -1,    # for URL
     partsreq => -1,    # for URL
     perms    => 1,     # file permissions
diff --git a/core/lib/Foswiki/Configure/Wizards/Save.pm b/core/lib/Foswiki/Configure/Wizards/Save.pm
index 9ecd31c..92978ea 100644
--- a/core/lib/Foswiki/Configure/Wizards/Save.pm
+++ b/core/lib/Foswiki/Configure/Wizards/Save.pm
@@ -199,19 +199,26 @@ sub save {
     }
 
     my %save;
-    foreach my $key ( @{ $Foswiki::cfg{BOOTSTRAP} } ) {
-        eval("(\$save$key)=\$Foswiki::cfg$key=~/^(.*)\$/");
-        ASSERT( !$@, $@ ) if DEBUG;
-        delete $Foswiki::cfg{BOOTSTRAP};
-    }
-
     # Clear out the configuration and re-initialize it either
     # with or without the .spec expansion.
     if ( $Foswiki::cfg{isBOOTSTRAPPING} ) {
+
+        foreach my $key ( @{ $Foswiki::cfg{BOOTSTRAP} } ) {
+            eval("(\$save$key)=\$Foswiki::cfg$key=~/^(.*)\$/");
+            ASSERT( !$@, $@ ) if DEBUG;
+            delete $Foswiki::cfg{BOOTSTRAP};
+        }
+
         %Foswiki::cfg = ();
 
         # Read without expansions but with the .spec
         Foswiki::Configure::Load::readConfig( 1, 0, 1 );
+
+        # apply bootstrapped settings
+        # print STDERR join( '', _generateLSC( $root, \%save, '', $reporter ) );
+        eval( join( '', _generateLSC( $root, \%save, '', $reporter ) ) );
+        die "Internal error: $@" if ($@);
+
     }
     else {
         %Foswiki::cfg = ();
@@ -220,16 +227,12 @@ sub save {
         Foswiki::Configure::Load::readConfig( 1, 1 );
     }
 
-    # apply bootstrapped settings
-    # print STDERR join( '', _generateLSC( $root, \%save, '', $reporter ) );
-    eval( join( '', _generateLSC( $root, \%save, '', $reporter ) ) );
-    die "Internal error: $@" if ($@);
-
     # Get changes from 'set' *without* expanding values
     if ( $this->param('set') ) {
         while ( my ( $k, $v ) = each %{ $this->param('set') } ) {
-            if ( defined $v && $v ne '' ) {
-                my $spec = $root->getValueObject($k);
+            my $spec = $root->getValueObject($k);
+            my $check = $spec->{CHECK}->[0];
+            if ( defined $v ) {
                 my ($value) = $v =~ m/^(.*)$/s;    #UNTAINT
                 if ($spec) {
                     eval { $value = $spec->decodeValue($value) };
@@ -245,11 +248,21 @@ sub save {
                     eval "\$Foswiki::cfg$k=\$value";
                 }
                 else {
-                    eval "undef \$Foswiki::cfg$k";
+                    if ( $check && $check->{undefok}[0] ) {
+                        eval "undef \$Foswiki::cfg$k";
+                    }
+                    else {
+                        eval "\$Foswiki::cfg$k=''";
+                    }
                 }
             }
             else {
-                eval "undef \$Foswiki::cfg$k";
+                if ( $check && $check->{undefok}[0] ) {
+                    eval "undef \$Foswiki::cfg$k";
+                }
+                else {
+                    eval "\$Foswiki::cfg$k=''";
+                }
             }
             ASSERT( !$@, $@ ) if DEBUG;
         }
@@ -306,11 +319,11 @@ sub _compareConfigs {
     if ($vs) {
 
         #print STDERR "REPORT ON $vs->{keys} $old $new\n";
-        if ( $vs->{typename} eq 'PASSWORD' ) {
-            $old = '_[redacted]_';
-            $new = '_[redacted]_';
-        }
         if ( $old ne $new ) {
+            if ( $vs->{typename} eq 'PASSWORD' ) {
+                $old = '_[redacted]_';
+                $new = '_[redacted]_';
+            }
             $old = "($vs->{default})" if $old eq 'undef' && $vs->{default};
             _logAndReport( $reporter, $logger, $keypath, $old, $new );
             return 0;
@@ -418,11 +431,11 @@ sub _generateLSC {
 
     my $vs = $spec->getValueObject($keys);
     if ($vs) {
-        if ( !defined $datum || $datum eq '' ) {
+        if ( !defined $datum ) {
 
-            # A null value and nullok will suppress the item in LSC
+            # A null value and undefok will suppress the item in LSC
             my $check = $vs->{CHECK}->[0];
-            if ( $check && $check->{nullok}[0] ) {
+            if ( $check && $check->{undefok}[0] ) {
                 return ();
             }
         }

I didn't document the CHECK=undefok vs. UNDEFINEDOK. I think both are needed but UNDEFINEDOK would be very rare.
  • UNDEFINEDOK - Present the undefined checkbox to the UI. Only needed when undefined is meaningful to the configuration and the user needs to be able to configure it.
  • CHECK=undefok - Checker and Save wizard will allow settings to be undefined in the LSC.

-- GeorgeClark - 04 Jan 2015

OK, I modified the patch a bit, to use undefok and emptyok as CHECK options; otherwise it's much the same.

-- CrawfordCurrie - 09 Jan 2015

I had to fix one thing ... not sure if it's completely correct. Not all definitions have that CHECK flag available. Wasn't sure if it was if ->can() && undefok or if not ->can() || undefok

-- GeorgeClark - 10 Jan 2015

I see you fixed my fix. Still crashing on save.

Can't call method "CHECK_option" on an undefined value at /var/www/foswiki/distro/core/lib/Foswiki/Configure/Wizards/Save.pm line 440.
at /var/www/foswiki/distro/core/lib/Foswiki/Configure/Wizards/Save.pm line 429.
Foswiki::Configure::Wizards::Save::_generateLSC('Foswiki::Configure::Root=HASH(0x96ce088)', '', '{AntiSpam}{EmailPadding}', 
...

I think I managed to fix this. Go ahead and close it if my fix is okay.

-- GeorgeClark - 10 Jan 2015
 
Topic revision: r13 - 16 Jan 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