Fixed bug that randomly prevented user or meta options from overridin…

…g the defaults, based on hashing (non)order.
kingpong · May 26, 2011 · 87c47ee · 87c47ee
1 parent 1681c07
commit 87c47ee
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 26 deletions.
diff --git a/Changes b/Changes
@@ -1,5 +1,9 @@
 Revision history for Perl extension PDF::WebKit.
 
+0.9   Thu May 26 23:22:00 2011
+  - Fixed bug that randomly prevented user or meta options from
+    overriding the defaults, based on hashing (non)order.
+
 0.8   Thu May 26 17:05:00 2011
   - Raised required version of XML::LibXML to 1.62 since that is the 
     first version that allows options to parse_html_string().

diff --git a/lib/PDF/WebKit.pm b/lib/PDF/WebKit.pm
@@ -9,7 +9,7 @@ use IPC::Run ();
 use PDF::WebKit::Configuration;
 use PDF::WebKit::Source;
 
-our $VERSION = 0.8;
+our $VERSION = 0.9;
 
 use Moose;
 
@@ -34,14 +34,12 @@ sub BUILD {
   my ($self,$args) = @_;
 
   $self->source( PDF::WebKit::Source->new($args->{source}) );
-
   $self->stylesheets( [] );
-
-  my %options;
-  %options = ( %{ $self->configuration->default_options }, %{ $args->{options} } );
-  %options = ( %options, $self->_find_options_in_meta($self->source) ) unless $self->source->is_url;
-  %options = $self->_normalize_options(%options);
-  $self->_set_options(\%options);
+  $self->_set_options({
+    $self->_normalize_options(%{ $self->configuration->default_options }),
+    $self->_normalize_options(%{ $args->{options} }),
+    $self->_normalize_options($self->_find_options_in_meta),
+  });
 
   if (not -e $self->configuration->wkhtmltopdf) {
     my $msg = "No wkhtmltopdf executable found\n";
@@ -62,9 +60,7 @@ sub configure {
 sub command {
   my $self = shift;
   my $path = shift;
-  my @args = ( $self->_executable );
-  push @args, %{ $self->options };
-  push @args, '--quiet';
+  my @args = ( $self->_executable, $self->_prepare_options, '--quiet' );
 
   if ($self->source->is_html) {
     push @args, '-';  # Get HTML from stdin
@@ -123,15 +119,17 @@ sub to_file {
 }
 
 sub _find_options_in_meta {
-  my ($self,$source) = @_;
+  my ($self) = @_;
+  return () if $self->source->is_url;
   # if we can't parse for whatever reason, keep calm and carry on.
-  my @result = eval { $self->_pdf_webkit_meta_tags($source) };
+  my @result = eval { $self->_pdf_webkit_meta_tags };
   return $@ ? () : @result;
 }
 
 sub _pdf_webkit_meta_tags {
-  my ($self,$source) = @_;
+  my ($self) = @_;
   return () unless eval { require XML::LibXML };
+  my $source = $self->source;
 
   my $prefix = $self->configuration->meta_tag_prefix;
 
@@ -184,14 +182,29 @@ sub _append_stylesheets {
   $self->source->string(\$html);
 }
 
+sub _prepare_options {
+  my ($self) = @_;
+  my $options = $self->options;
+  my @args;
+  while (my ($name,$val) = each %$options) {
+    next unless defined($val) && length($val);
+    if ($val eq 'yes' || $val eq 'YES') {
+      push @args, $name;
+    }
+    else {
+      push @args, $name, $val;
+    }
+  }
+  return @args;
+}
+
 sub _normalize_options {
   my $self = shift;
   my %orig_options = @_;
   my %normalized_options;
   while (my ($key,$val) = each %orig_options) {
-    next unless defined($val) && length($val);
     my $normalized_key = "--" . $self->_normalize_arg($key);
-    $normalized_options{$normalized_key} = $self->_normalize_value($val);
+    $normalized_options{$normalized_key} = $val;
   }
   return %normalized_options;
 }
@@ -203,16 +216,6 @@ sub _normalize_arg {
   return $arg;
 }
 
-sub _normalize_value {
-  my ($self,$value) = @_;
-  if (defined($value) && ($value eq 'yes' || $value eq 'YES')) {
-    return undef;
-  }
-  else {
-    return $value;
-  }
-}
-
 no Moose;
 __PACKAGE__->meta->make_immutable;
 

diff --git a/t/pdf-webkit.t b/t/pdf-webkit.t
@@ -141,6 +141,32 @@ describe "PDF::WebKit" => sub {
         is( $command[ index_of('--page-size',@command) + 1 ], 'Legal' );
         is( $command[ index_of('--orientation',@command) + 1 ], 'Landscape' );
       };
+
+      it "should normalize options before combining, so e.g. page-size can override default page_size" => sub {
+        # This test can pass even if the behavior is broken.
+        # I'm not sure how to fix it without converting PDF::WebKit to
+        # use an ordered hash. The hashing order created by this data
+        # set evokes the bug in perl 5.8.9, at least.
+        my $body = q{
+          <html>
+            <head>
+              <meta name="pdf-webkit-page-size" content="Legal"/>
+              <meta name="pdf-webkit-margin-top" content="0"/>
+              <meta name="pdf-webkit-margin-bottom" content="0"/>
+              <meta name="pdf-webkit-margin-left" content="0"/>
+              <meta name="pdf-webkit-margin-right" content="0"/>
+            </head>
+          </html>
+        };
+        my $pdfkit = PDF::WebKit->new(\$body);
+        my @command = $pdfkit->command;
+        is( scalar(grep { /page.*size/ } @command), 1 );
+        is( scalar(grep { /margin.*top/ } @command), 1 );
+        is( scalar(grep { /margin.*bottom/ } @command), 1 );
+        is( $command[ index_of('--page-size',@command) + 1 ], 'Legal' );
+        is( $command[ index_of('--margin-top',@command) + 1 ], '0' );
+        is( $command[ index_of('--margin-bottom',@command) + 1 ], '0' );
+      };
     }
   };