Bug #18993 for libwww-perl: HTML::Form does not recognise multiple select items in the same form

Sun Apr 30 13:52:49 2006 Guest - Ticket created

Subject:

HTML::Form does not recognise multiple select items in the same form

It is legal HTML to have multiple fields in the same form with the same name. HTML::Form even recognises this. Sadly, <select> form elements are treated as if they're open. See the attached HTML in any web browser, notice three select boxes? Now, parse it with HTML::Form, see the pain? I'm working on fixing this now, so probably I'll have a patch in a few hours to attach.

Subject:

simple.html

hi mom hi mom hi mom

Mon May 01 09:40:10 2006 Guest - Correspondence added

From:

Gavin Peters <gpeters [...] deepsky.com

I have now fixed this bug, in the case of select lists. It was a simple fix, and I haven't bothered going to see how many other HTML elements in forms were being left open when they should have been closed. Incidentally: I needed this to handle WWW::Mechanize accessing the MTA FastLane system, a website for managing your automatic toll paying aparatus from the MTA.

diff -ru libwww-perl-5.805.orig/lib/HTML/Form.pm libwww-perl-5.805/lib/HTML/Form.pm --- libwww-perl-5.805.orig/lib/HTML/Form.pm 2005-12-07 09:32:27.000000000 -0500 +++ libwww-perl-5.805/lib/HTML/Form.pm 2006-04-30 18:27:19.000000000 -0400 @@ -139,6 +139,8 @@ my @forms; my $f; # current form + my %openselect; # index to the open instance of a select + while (my $t = $p->get_tag) { my($tag,$attr) = @$t; if ($tag eq "form") { @@ -196,6 +198,9 @@ $attr->{"select_$_"} = delete $attr->{$_} if exists $attr->{$_}; } + # count this new select option separately + $openselect{$attr->{name}}++; + while ($t = $p->get_tag) { my $tag = shift @$t; last if $tag eq "/select"; @@ -214,6 +219,7 @@ $a{value_name} = $p->get_trimmed_text; $a{value} = delete $a{value_name} unless defined $a{value}; + $a{idx} = $openselect{$attr->{name}}; $f->push_input("option", \%a); } else { @@ -1043,7 +1049,7 @@ my $m = $self->{menu}[0]; $m->{disabled}++ if delete $self->{option_disabled}; - my $prev = $form->find_input($self->{name}, $self->{type}); + my $prev = $form->find_input($self->{name}, $self->{type}, $self->{idx}); return $self->SUPER::add_to_form($form) unless $prev; # merge menues

Mon May 01 09:40:12 2006 The RT System itself - Status changed from 'new' to 'open'

Wed Apr 09 04:29:23 2008 GAAS [...] cpan.org - Correspondence added

It took a while :( I've now applied this patch. Thanks!

Wed Apr 09 04:29:25 2008 GAAS [...] cpan.org - Status changed from 'open' to 'resolved'

Mon May 05 11:13:16 2008 MARKSTOS [...] cpan.org - Correspondence added

The patch for this introduced a new bug, which is filed here: http://rt.cpan.org/Ticket/Display.html?id=35607 It would be good to double-check my refinement of the patch submitted in that ticket to confirm it doesn't re-open this defect.

Mon May 05 11:13:18 2008 The RT System itself - Status changed from 'resolved' to 'open'

Mon May 05 14:01:37 2008 MARKSTOS [...] cpan.org - Correspondence added

Subject:	HTML::Form does not recognise multiple select items (recommend reverting patch)
From:	MARKSTOS [...] cpan.org

On Mon May 05 11:13:16 2008, MARKSTOS wrote: Show quoted text

> The patch for this introduced a new bug, which is filed here: > http://rt.cpan.org/Ticket/Display.html?id=35607 > > It would be good to double-check my refinement of the patch submitted in > that ticket to confirm it doesn't re-open this defect.

After spending hours working with the effects of this patch, I have reverted it in my copy and have come to the conclusion that it should be reverted upstream as well, for two reasons. 1. Clearly, it introduced at least one other bug, as I tried to address in RT#35607. This patch should be re-introduced with at least one automated test to confirm it works as expected. 2. The patch itself introduces a regression of sorts (separate from RT#35607, I think). Before if there were multiple select forms with the same name, it worked to just say "fill in that select form with this value". I suppose since all the <selects> were accidentally lumped together, it just worked. With the patch applied, code that targeted a form that was not first now breaks with an "illegal value" error. I'm not opposed to telling people "You need to update your code because we started working more correctly." However, for this issue there is no easy path forward unless you know exactly by number which <select> form you want to submit to, which I didn't in my application. One possible solution to further extend "find_input" so it can find an input where a value is a possible input. Without something this, other people who upgrade LWP will be stuck like I was when their code starts breaking and there's not a clear way to fix it. In short, we should able to express the following: "Find the <select> tag with this name which accepts this value" I've also added a "wish" in Mechanize to support a fix at that level, but having a smarter find_input() would certainly be helpful. ( http://code.google.com/p/www-mechanize/issues/detail?id=51 ) In the meantime, I do advocate reverting this patch. Mark

Mon May 05 16:35:22 2008 GAAS [...] cpan.org - Correspondence added

Mark, Regarding your 2) concern; if you use the $from->param($name, $value) interface to set the value it should just do the right thing, it will search for inputs that accept the value you provide. The issue only occurs for the $form->value($name, $value) interface. I'm not sure that's worth fixing.

Wed Jul 23 14:44:31 2008 GAAS [...] cpan.org - Status changed from 'open' to 'resolved'