Jakub Narebski [off-list ref] writes:
In first version of href() function we had
(commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} keys %params
) );
First, there was a question what happend if someone would enter
parameter name incorrectly, and some key of %params is not found in
%mapping hash. The above code would generate warnings (which web admins
frown upon), and empty (because undef) parameters corresponding to e.g.
mistyped parameter name.
The one in "next" seems to do this (the diff is between "master"
and "next"):
@@ -204,7 +277,9 @@ sub href(%) {
my $href = "$my_uri?";
$href .= esc_param( join(";",
- map { "$mapping{$_}=$params{$_}" } keys %params
+ map {
+ "$mapping{$_}=$params{$_}" if defined $params{$_}
+ } keys %params
) );
return $href;
So we perhaps would want to say:
if (defined $params{$_} && exists $mapping{$_})
instead there?
Second problem is that using href() function, although it consolidates
to generate URL for CGI, it changes the order of CGI parameters. It
used to be that 'p' (project) parameter was first, then 'a' (action)
parameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or
'p' (page) or 's' (searchtext). The simplest and fastest solution would
be to create array with all keys of %mapping in appropriate order and
do something like this:
my @mapping_sorted = ('project', 'action', 'hash',
'hash_parent', 'hash_base', 'file_name', 'searchtext');
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} grep { exists $params{$_}} @mapping_sorted;
) );
The problem is of course updating both %mappings and @mapping_sorted.
Is this really a problem, should this (ordering of CGI parameters)
addressed?
Keeping the generated URL stable would be a very desirable
feature. Perhaps something like this?
sub href(%) {
my @mapping = ( project => "p",
action => "a",
hash => "h",
hash_parent => "hp",
hash_base => "hb",
file_name => "f",
file_parent => "fp",
page => "pg",
searchtext => "s",
);
my %mapping;
for (my $i = 0; $i < @mapping; $i += 2) {
my ($k, $v) = ($mapping[$i], $mapping[$i+1]);
$mapping{$k} = [$i, $v];
}
my %params = @_;
$params{"project"} ||= $project;
my $href = "$my_uri?";
$href .= esc_param( join(";",
map { $_->[1] }
sort { $a->[0] <=> $b->[0] }
map {
(defined $params{$_} && exists $mapping{$_})
? [ $mapping{$_}[0], "$mapping{$_}[1]=$params{$_}" ]
: ();
} keys %params
) );
return $href;
}
Junio C Hamano wrote:
quoted hunk
Jakub Narebski [off-list ref] writes:
quoted
In first version of href() function we had
(commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} keys %params
) );
First, there was a question what happend if someone would enter
parameter name incorrectly, and some key of %params is not found in
%mapping hash. The above code would generate warnings (which web admins
frown upon), and empty (because undef) parameters corresponding to e.g.
mistyped parameter name.
The one in "next" seems to do this (the diff is between "master"
and "next"):
@@ -204,7 +277,9 @@ sub href(%) {
my $href = "$my_uri?";
$href .= esc_param( join(";",
- map { "$mapping{$_}=$params{$_}" } keys %params
+ map {
+ "$mapping{$_}=$params{$_}" if defined $params{$_}
+ } keys %params
) );
return $href;
This doesn't work as expected. Map works on _every_ element of list; if
expression doesn't return anything it just puts undef I think. For example
while
print join(";",
map {
"a/$_" if ($_ eq lc($_))
} ("a", "b", "C", "d")),
"\n";'
returns "a/a;a/b;;a/d" (notice the doubled ';'), the correct way would be to
use grep instead:
print join(";",
map {
"a/$_"
} grep {
($_ eq lc($_))
} ("a", "b", "C", "d")),
"\n";
returns correct "a/a;a/b;a/d". So the fragment should read what I wrote:
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} grep { exists $mapping{$_} } keys %params
) );
quoted
Second problem is that using href() function, although it consolidates
to generate URL for CGI, it changes the order of CGI parameters. It
used to be that 'p' (project) parameter was first, then 'a' (action)
parameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or
'p' (page) or 's' (searchtext). The simplest and fastest solution would
be to create array with all keys of %mapping in appropriate order and
do something like this:
my @mapping_sorted = ('project', 'action', 'hash',
'hash_parent', 'hash_base', 'file_name', 'searchtext');
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} grep { exists $params{$_}} @mapping_sorted;
) );
The problem is of course updating both %mappings and @mapping_sorted.
Is this really a problem, should this (ordering of CGI parameters)
addressed?
Keeping the generated URL stable would be a very desirable
feature. Perhaps something like this?
sub href(%) {
my @mapping = ( project => "p",
action => "a",
hash => "h",
hash_parent => "hp",
hash_base => "hb",
file_name => "f",
file_parent => "fp",
page => "pg",
searchtext => "s",
);
my %mapping;
for (my $i = 0; $i < @mapping; $i += 2) {
my ($k, $v) = ($mapping[$i], $mapping[$i+1]);
$mapping{$k} = [$i, $v];
}
my %params = @_;
$params{"project"} ||= $project;
my $href = "$my_uri?";
$href .= esc_param( join(";",
map { $_->[1] }
sort { $a->[0] <=> $b->[0] }
map {
(defined $params{$_} && exists $mapping{$_})
? [ $mapping{$_}[0], "$mapping{$_}[1]=$params{$_}" ]
: ();
} keys %params
) );
return $href;
}
What about my proposed solution? Don't sort, use sorted keys instead, i.e.
something like (after unmangling whitespace)
sub href(%) {
my @mapping = ( project => "p",
action => "a",
hash => "h",
hash_parent => "hp",
hash_base => "hb",
file_name => "f",
file_parent => "fp",
page => "pg",
searchtext => "s",
);
my %mapping;
my @mapping_keys;
for (my $i = 0; $i < @mapping; $i += 2) {
my ($k, $v) = ($mapping[$i], $mapping[$i+1]);
$mapping{$k} = $v;
push @mapping_keys, $k;
}
my %params = @_;
$params{"project"} ||= $project;
my $href = "$my_uri?";
$href .= esc_param( join(";",
map { "$mapping{$_}=$params{$_}" }
grep { exists $params{$_} }
@mapping_keys
) );
return $href;
}
This has the advantage and disadvantage of constant cost, linear with number
of %mapping keys, instead of N log N cost where N is number of parameters.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git