Depends on RT#35376: CGI.pm popup_menu multiple selects [rt.cpan.org #30057]

[leejo]

https://rt.cpan.org/Ticket/Display.html?id=30057

When you create a popup_menu with CGI.pm (ex: print my_popup_menu(-
name=>'selection_serverlist',-default=>'' ,-multiple=>'true',-
values=>\@ep, -labels=>\%serverlist);) and you use multiple=>'true' 
you can select multiple items in the list and post them.

The problem in CGI.pm is that when you rebuild you page your selection 
is lost.
The CGI.pm module doesn't work with the hash that's being returned.
It only uses the first item in the has.

As a quick-fix I changed the following:

foreach (@values) {
    if (/<optgroup/) {
        foreach (split(/\n/)) {
            my $selectit = $XHTML ? 'selected="selected"' : 'selected';
            s/(value="$selected")/$selectit $1/ if defined $selected;
            $result .= "$_\n";
        }
    }
    else {
    my $temp_value = $_;
        for ( $self->param($name)) {
        if ($temp_value eq $_)  {
            $selected = $_;
        }
    }
        my $attribs = $self->_set_attributes($_, $attributes);
    my($selectit) = defined($selected) ? $self->_selected
($selected eq $_) : '';
    my($label) = $_;
    $label = $labels->{$_} if defined($labels) && defined($labels->
{$_});
    my($value) = $self->escapeHTML($_);
    $label=$self->escapeHTML($label,1);
        $result .= "<option${attribs} ${selectit}
value=\"$value\">$label</option>\n";
    }
}

The first 6 lines of the else are mine. They fix my problem with the 
multiple select.

Kind regards,
Tom

[leejo]

MARKSTOS - 2009-07-22 01:24:10

On Wed Oct 17 09:08:47 2007, Wivern wrote:

When you create a popup_menu with CGI.pm (ex: print my_popup_menu(-
name=>'selection_serverlist',-default=>'' ,-multiple=>'true',-
values=>@ep, -labels=>%serverlist);) and you use multiple=>'true'
you can select multiple items in the list and post them.

The problem in CGI.pm is that when you rebuild you page your selection
is lost.
The CGI.pm module doesn't work with the hash that's being returned.
It only uses the first item in the has.

As a quick-fix I changed the following:

foreach (@values) {
if (/<optgroup/) {
foreach (split(/\n/)) {
my $selectit = $XHTML ? 'selected="selected"' : 'selected';
s/(value="$selected")/$selectit $1/ if defined $selected;
$result .= "$\n";
}
}
else {
my $temp_value = $;
for ( $self->param($name)) {
if ($temp_value eq $) {
$selected = $;
}
}
my $attribs = $self->set_attributes($, $attributes);
my($selectit) = defined($selected) ? $self->selected
($selected eq $) : '';
my($label) = $;
$label = $labels->{$} if defined($labels) && defined($labels->
{$});
my($value) = $self->escapeHTML($);
$label=$self->escapeHTML($label,1);
$result .= "&lt;option${attribs} ${selectit}
value="$value">$label\n";
}
}

The first 6 lines of the else are mine. They fix my problem with the
multiple select.

Thanks for the report.

If the issue still persists, could you prepare a Test::More style test
case which illustrates it?

I would also recommend considering using a templating system to generate
the forms, and using HTML::FIllInForm to handle selecting values in the
form.

Mark

[leejo]

WSDOOKADR - 2009-08-16 13:45:43

Hi,

I've just fixed this bug.
I have the code from github cloned
I replaced only one line in CGI.pm and that will be my patch.
Can I write here the patch.
Also , as I understand , Somni on #perl on FreeNode argued
that this isn't actually a bug-report but it is a feature because it
wasn't mentioned in the initial specs for a popup_menu
to support multiple selection.
Do I just send the patch or also write a test for it ?

Thank you,
Stefan

[leejo]

WSDOOKADR - 2009-08-16 20:43:51

Ok, patches rolled in.
RFC please :)

[leejo]

MARKSTOS - 2009-08-16 21:29:48

On Sun Aug 16 16:43:51 2009, WSDOOKADR wrote:

Ok, patches rolled in.
RFC please :)

Thanks for the patch!

Here's the line you added:

$selectit = ' selected="selected" ' if $_ ~~ @{$self->{param}->{$name}}
// '';

For CGI.pm we would like compatibility with Perl 5.6. Could you rewrite
it, avoiding "~~" and "//" ?

You are also welcome to send me a pull request on github.

Thanks!

 Mark

[leejo]

WSDOOKADR - 2009-08-17 02:39:46

ok

• replaced the ~~ with grep.
• added some description of popup_menu to the POD.
• added check for "-multiple" in order to not break other tests in
  ( if I don't check for "-multiple" some tests form.t:106 and
  no_tabindex.t:108)

[leejo]

MARKSTOS - 2009-08-17 11:58:31

Thanks!

I'm about to leave for work now, but another helper watching the bug
tracker should feel feel to peer-review this and apply it in their
github branch if approved. Otherwise I should get to in the next 24 hours.

Mark

[leejo]

MARKSTOS - 2009-08-18 00:48:52

Thanks.

However, when I applied this patch and the new test, one of the tests
failed:

not ok 2 - popup_menu(): test related to having multiple items selected
and after select they should still be selected

Failed test (t/popup_menu.t at line 37)

got: ' aaa bbb ccc '

expected: ' aaa bbb ccc ' 1..2

If you submit a refined patch, there are a couple ways it could be
cleaned up:

• In the test it said "use feature 'say'", but it didn't seem to do
  anything.
• In CGI.pm, there was commented out code related to "sub deb" and the
  call to Dumper can be removed, as can the comment about Perl 5.6
  compatibility.

Also, I didn't understand this idiom:

my $occ_val = () = grep /^$val$/,@{ $self->{param}->{$name} };

What is the intent of assigning an empty list to a value, and then
assigning the result to a variable?

Mark

[leejo]

WSDOOKADR - 2009-08-18 00:59:42

On Mon Aug 17 20:48:52 2009, MARKSTOS wrote:

Also, I didn't understand this idiom:

my $occ_val = () = grep /^$val$/,@{ $self->{param}->{$name} };

What is the intent of assigning an empty list to a value, and then
assigning the result to a variable?

Sorry , here I intended
my $occ_val = grep /^$val$/,@{ $self->{param}->{$name} };

[leejo]

MARKSTOS - 2009-08-18 01:23:54

Another tip: Use "diff -u" to generate the patches. This will include
the file name and some more context in the file, making it easier to
review the patch. You can also concatenate two of these diff files
together to make one attachment which been reviewed and re-applied in
one action.