fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
fu ([personal profile] fu) wrote in [site community profile] changelog2011-10-03 09:16 am
  • Add Memory
  • Share This Entry

[dw-free] Non-Apache-specific way to to grab multiple form values

[commit: http://hg.dwscoalition.org/dw-free/rev/4a41024ab8ed]

http://bugs.dwscoalition.org/show_bug.cgi?id=3729

Use Hash::MultiValue as the backer for post_args and get_args . When we
expect an array, use get_all instead of get. Works with both new code and
legacy BML \0 separators.

Patch by [personal profile] exor674; additional modifications to controllers by
[personal profile] fu

Files modified:
  • bin/checkconfig.pl
  • cgi-bin/DW/Controller/Rename.pm
  • cgi-bin/DW/Request.pm
  • cgi-bin/DW/Request/Apache2.pm
  • cgi-bin/DW/Request/Base.pm
  • cgi-bin/DW/Request/Standard.pm
  • cgi-bin/DW/Template/Plugin/FormHTML.pm
  • cgi-bin/ljtextutil.pl
  • t/request-multi.t
--------------------------------------------------------------------------------
diff -r 29ee796ebdae -r 4a41024ab8ed bin/checkconfig.pl
--- a/bin/checkconfig.pl	Mon Oct 03 15:27:38 2011 +0800
+++ b/bin/checkconfig.pl	Mon Oct 03 17:16:38 2011 +0800
@@ -207,6 +207,7 @@
                    'deb' => "libbusiness-creditcard-perl",
                    'opt' => "Required for taking credit/debit cards in the shop.",
                },
+               "Hash::MultiValue" => {},
               );
 
 
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Controller/Rename.pm
--- a/cgi-bin/DW/Controller/Rename.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Controller/Rename.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -115,9 +115,9 @@
                 touser      => $post_args->{touser} || $get_args->{to} || "",
                 redirect    => $post_args->{redirect} || "disconnect",
                 rel_types   => \@rel_types,
-                rel_options => %$post_args ? { map { $_ => 1 } $post_args->get( "rel_options" ) }
+                rel_options => %$post_args ? { map { $_ => 1 } $post_args->get_all( "rel_options" ) }
                                             : { map { $_ => 1 } @rel_types },
-                others      => %$post_args ? { map { $_ => 1 } $post_args->get( "others" ) }
+                others      => %$post_args ? { map { $_ => 1 } $post_args->get_all( "others" ) }
                                             : { email => 0 },
             };
 
@@ -152,7 +152,7 @@
 
     # since you can't recover deleted relationships, but you can delete the relationships later if something was missed
     # negate the form submission so we're explicitly stating which rels we want to delete, rather than deleting everything not listed
-    my %keep_rel = map { $_ => 1 } $post_args->get( "rel_options" );
+    my %keep_rel = map { $_ => 1 } $post_args->get_all( "rel_options" );
     my %del_rel = map { +"del_$_" => ! $keep_rel{$_} } qw( trusted_by watched_by trusted watched communities );
 
     my %other_opts = map { $_ => 1 } $post_args->get( "others" );
@@ -352,7 +352,7 @@
     if( $post_args->{override_relationships} ) {
         # since you can't recover deleted relationships, but you can delete the relationships later if something was missed
         # negate the form submission so we're explicitly stating which rels we want to delete, rather than deleting everything not listed
-        my %keep_rel = map { $_ => 1 } $post_args->get( "rel_options" );
+        my %keep_rel = map { $_ => 1 } $post_args->get_all( "rel_options" );
         my %del_rel = map { +"del_$_" => ! $keep_rel{$_} } qw( trusted_by watched_by trusted watched communities );
 
         $rename_opts{del} = \%del_rel;
@@ -360,7 +360,7 @@
 
 
     if( $post_args->{override_others} ) {
-        my %other_opts = map { $_ => 1 } $post_args->get( "others" );
+        my %other_opts = map { $_ => 1 } $post_args->get_all( "others" );
 
         # force email to false if we can't support forwarding for this user
         if ( $other_opts{email} ) {
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Request.pm
--- a/cgi-bin/DW/Request.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Request.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -29,6 +29,7 @@
 use strict;
 use DW::Request::Apache2;
 use DW::Request::Standard;
+use Hash::MultiValue;
 
 our ( $cur_req, $determined );
 
@@ -166,7 +167,7 @@
 
 =head2 C<< $r->post_args >>
 
-Get the POST arguments.
+Return the POST arguments.
 
 =head2 C<< $r->print( $string ) >>
 
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Request/Apache2.pm
--- a/cgi-bin/DW/Request/Apache2.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Request/Apache2.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -28,6 +28,7 @@
 use Apache2::RequestUtil ();
 use Apache2::RequestIO ();
 use Apache2::SubProcess ();
+use Hash::MultiValue;
 
 use fields (
             'r',         # The Apache2::Request object
@@ -35,7 +36,6 @@
             # these are mutually exclusive; if you use one you can't use the other
             'content',   # raw content
             'post_args', # hashref of POST arguments
-            'get_args',  # hashref of GET arguments
         );
 
 # creates a new DW::Request object, based on what type of server environment we
@@ -101,26 +101,25 @@
     return $self->{content} = $buff;
 }
 
-# get POST arguments as an APR::Table object (which is a tied hashref)
 sub post_args {
     my DW::Request::Apache2 $self = $_[0];
 
     die "already loaded content\n"
         if defined $self->{content};
 
-    unless ( defined $self->{post_args} ) {
-        my $tmp_r = Apache2::Request->new( $self->{r} );
-        $self->{post_args} = $tmp_r->body;
+    return $self->{post_args} if defined $self->{post_args};
+
+    my $tmp_r = Apache2::Request->new( $self->{r} );
+    my $data = $tmp_r->body;
+
+    my @out;
+    foreach my $key ( keys %$data ) {
+        my @val = $data->get( $key );
+        next unless @val;
+        push @out, map { $key => $_ } @val;
     }
-    return $self->{post_args};
-}
 
-sub get_args {
-    my DW::Request::Apache2 $self = $_[0];
-    return $self->{get_args} if defined $self->{get_args};
-
-    my %gets = LJ::parse_args( $self->query_string );
-    return $self->{get_args} = \%gets;
+    return $self->{post_args} = Hash::MultiValue->new( @out );
 }
 
 # searches for a given note and returns the value, or sets it
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Request/Base.pm
--- a/cgi-bin/DW/Request/Base.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Request/Base.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -24,6 +24,8 @@
 use fields (
             'cookies_in',
             'cookies_in_multi',
+
+            'get_args',
         );
 
 sub new {
@@ -33,6 +35,8 @@
 
     $self->{cookies_in} = undef;
     $self->{cookies_in_multi} = undef;
+
+    $self->{get_args} = undef;
 }
 
 sub cookie {
@@ -76,6 +80,29 @@
     return $self->add_cookie( %args );
 }
 
+# FIXME: This relies on the behavior parse_args
+#   and the \0 seperated arguments. This should be cleaned
+#   up at the same point parse_args is.
+sub _string_to_multivalue {
+    my %gets = LJ::parse_args( $_[1] );
+
+    my @out;
+    foreach my $key ( keys %gets ) {
+        my @parts = split(/\0/, $gets{$key});
+        push @out, map { $key => $_ } @parts;
+    }
+
+    return Hash::MultiValue->new( @out );
+}
+
+sub get_args {
+    my DW::Request $self = $_[0];
+    return $self->{get_args} if defined $self->{get_args};
+
+    return $self->{get_args} =
+        $self->_string_to_multivalue( $self->query_string );
+}
+
 #
 # Following sub was copied from CGI::Cookie and modified.
 #
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Request/Standard.pm
--- a/cgi-bin/DW/Request/Standard.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Request/Standard.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -35,7 +35,6 @@
             # these are mutually exclusive; if you use one you can't use the other
             'content',   # raw content
             'post_args', # hashref of POST arguments
-            'get_args',  # hashref of GET arguments
 
             # we have to parse these out ourselves
             'uri',
@@ -124,7 +123,6 @@
     return $self->{res}->as_string;
 }
 
-# get POST arguments as an APR::Table object (which is a tied hashref)
 sub post_args {
     my DW::Request::Standard $self = $_[0];
 
@@ -135,13 +133,8 @@
 
     # get the content and parse it.  I would have expected there to be some
     # official method of doing this on HTTP::Request?  guess not.
-    return $self->{post_args} = { LJ::parse_args( $self->{req}->content ) };
-}
-
-sub get_args {
-    my DW::Request::Standard $self = $_[0];
-    return $self->{get_args} if defined $self->{get_args};
-    return $self->{get_args} = { LJ::parse_args( $self->query_string ) };
+    return $self->{post_args} =
+        $self->_string_to_multivalue( $self->{req}->content );
 }
 
 # searches for a given note and returns the value, or sets it
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/DW/Template/Plugin/FormHTML.pm
--- a/cgi-bin/DW/Template/Plugin/FormHTML.pm	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/DW/Template/Plugin/FormHTML.pm	Mon Oct 03 17:16:38 2011 +0800
@@ -26,7 +26,7 @@
 
 The form plugin generates HTML elements with attributes suitably escaped, and values automatically prepopulated, depending on the form's data field.
 
-The "data" field is a hashref, with the keys being the form element's name, and the values being the form element's desired value.
+The "data" field is an instance of Hash::MultiValue, with the keys being the form element's name, and the values being the form element's desired value.
 
 If a "formdata" property is available via the context, this is used to automatically populate the plugin's data field.
 =cut
@@ -79,10 +79,8 @@
 
     my $ret = "";
 
-    # FIXME: check an array of args, rather than expecting this to be an APR::RequestTable
-    # processes any previous form submissions. Doing this out here as it's special-cased
     if ( ! defined $args->{selected} && $self->{data} ) {
-        my %selected = map { $_ => 1 } $self->{data}->get( $args->{name} );
+        my %selected = map { $_ => 1 } $self->{data}->get_all( $args->{name} );
         $args->{selected} = $selected{$args->{value}};
     }
 
@@ -121,10 +119,8 @@
 
     my $ret = "";
 
-    # FIXME: check an array of args, rather than expecting this to be an APR::RequestTable
-    # processes any previous form submissions. Doing this out here as it's special-cased
     if ( ! defined $args->{selected} && $self->{data} ) {
-        my %selected = map { $_ => 1 } $self->{data}->get( $args->{name} );
+        my %selected = map { $_ => 1 } $self->{data}->get_all( $args->{name} );
         $args->{selected} = $selected{$args->{value}};
     }
 
diff -r 29ee796ebdae -r 4a41024ab8ed cgi-bin/ljtextutil.pl
--- a/cgi-bin/ljtextutil.pl	Mon Oct 03 15:27:38 2011 +0800
+++ b/cgi-bin/ljtextutil.pl	Mon Oct 03 17:16:38 2011 +0800
@@ -36,6 +36,10 @@
 
 # similar to decode_url_string below, but a nicer calling convention.  returns
 # a hash of items parsed from the string passed in as the only argument.
+
+# FIXME: This method using \0 is being used in legacy locations
+#  however should be factored out ( to Hash::MultiValue )
+#  as soon as the need for the legacy use is removed.
 sub parse_args {
     my $args = $_[0];
     return unless defined $args;
diff -r 29ee796ebdae -r 4a41024ab8ed t/request-multi.t
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/t/request-multi.t	Mon Oct 03 17:16:38 2011 +0800
@@ -0,0 +1,76 @@
+# -*-perl-*-
+use strict;
+use Test::More tests => 2;
+use lib "$ENV{LJHOME}/cgi-bin";
+
+require 'ljlib.pl';
+use DW::Request::Standard;
+use HTTP::Request;
+
+check_get(
+    "foo=bar&bar=baz&foo=qux",
+    sub {
+        plan tests => 6;
+
+        my $r = DW::Request->get;
+        my $args = $r->get_args;
+
+        is( 'qux', $args->{foo} );
+        is( 'qux', $args->get('foo') );
+        is_deeply( ['bar','qux'], [ $args->get_all('foo') ] );
+
+        is( 'baz', $args->{bar} );
+        is( 'baz', $args->get('bar') );
+        is_deeply( ['baz'], [ $args->get_all('bar') ] );
+    }
+);
+
+check_post(
+    "foo=bar&bar=baz&foo=qux",
+    sub {
+        plan tests => 6;
+
+        my $r = DW::Request->get;
+        my $args = $r->post_args;
+
+        is( 'qux', $args->{foo} );
+        is( 'qux', $args->get('foo') );
+        is_deeply( ['bar','qux'], [ $args->get_all('foo') ] );
+
+        is( 'baz', $args->{bar} );
+        is( 'baz', $args->get('bar') );
+        is_deeply( ['baz'], [ $args->get_all('bar') ] );
+    }
+);
+
+sub check_get {
+    my ( $args, $sv ) = @_;
+
+    # Telling Test::Builder ( which Test::More uses ) to
+    # look one level further up the call stack.
+    local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+    my $rq = HTTP::Request->new(GET => "http://www.example.com/test?$args");
+
+    DW::Request->reset;
+    my $r = DW::Request::Standard->new( $rq );
+
+    subtest "GET $args", sub { $sv->() };
+}
+
+sub check_post {
+    my ( $args, $sv ) = @_;
+
+    # Telling Test::Builder ( which Test::More uses ) to
+    # look one level further up the call stack.
+    local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+    my $rq = HTTP::Request->new(POST => "http://www.example.com/test");
+    $rq->header( 'Content-Type' => 'multipart/form-data' );
+    $rq->add_content_utf8( $args );
+
+    DW::Request->reset;
+    my $r = DW::Request::Standard->new( $rq );
+
+    subtest "POST $args", sub { $sv->() };
+}
--------------------------------------------------------------------------------
  • Add Memory
  • Share This Entry
(0 comments)

Post a comment in response:

This account has disabled anonymous posting.
If you don't have an account you can create one now.
HTML doesn't work in the subject.
More info about formatting

If you are unable to use this captcha for any reason, please contact us by email at support@dreamwidth.org

 
Notice: This account is set to log the IP addresses of everyone who comments.
Links will be displayed as unclickable URLs to help prevent spam.