Followup to: Issue #3052757 by Pancho: Fix inconsistent parsing of boolean query parameters

src/Service/FillPdfLinkManipulator.php

@@ -72,6 +72,14 @@ class FillPdfLinkManipulator implements FillPdfLinkManipulatorInterface {
       throw new \InvalidArgumentException('No FillPdfForm was specified in the query string, so failing.');
     }
 
+    if (isset($query['download']) && filter_var($query['download'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) === TRUE) {
+      $request_context['force_download'] = TRUE;
+    }
+
+    if (isset($query['flatten']) && $query['flatten'] !== '' && filter_var($query['flatten'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) === FALSE) {
+      $request_context['flatten'] = FALSE;
+    }
+
     // Is this just the PDF populated with sample data?
     if (isset($query['sample']) && filter_var($query['sample'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) === TRUE) {
       $request_context['sample'] = TRUE;
@@ -132,13 +140,6 @@ class FillPdfLinkManipulator implements FillPdfLinkManipulatorInterface {
     // We've processed the shorthand forms, so unset them.
     unset($request_context['entity_id'], $request_context['entity_type']);
 
-    if (isset($query['download']) && filter_var($query['download'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) === TRUE) {
-      $request_context['force_download'] = TRUE;
-    }
-    if (isset($query['flatten']) && filter_var($query['flatten'], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) === FALSE) {
-      $request_context['flatten'] = FALSE;
-    }
-
     return $request_context;
   }
 

tests/src/Unit/LinkManipulator/ParseLinkTest.php → tests/src/Unit/LinkManipulator/ParseLinkBooleansTest.php

@@ -11,40 +11,25 @@ use Drupal\Tests\UnitTestCase;
  *
  * @group fillpdf
  */
-class ParseLinkTest extends UnitTestCase {
+class ParseLinkBooleansTest extends UnitTestCase {
 
   /**
-   * Tests &sample= query parameter.
+   * Tests &sample=, &download= and &flatten= query parameters.
    *
    * @dataProvider dataProvider
    */
-  public function testSample($input, $expected) {
+  public function testBooleans($input, $expected) {
     $request_context = FillPdfLinkManipulator::parseLink($this->link($input));
+
     $this->assertEquals(is_null($expected) ? FALSE : $expected, $request_context['sample']);
-  }
 
-  /**
-   * Tests &download= query parameter.
-   *
-   * @dataProvider dataProvider
-   */
-  public function testDownload($input, $expected) {
-    $request_context = FillPdfLinkManipulator::parseLink($this->link($input));
     $this->assertEquals(is_null($expected) ? FALSE : $expected, $request_context['force_download']);
-  }
 
-  /**
-   * Tests &flatten= query parameter.
-   *
-   * @dataProvider dataProvider
-   */
-  public function testFlatten($input, $expected) {
-    $request_context = FillPdfLinkManipulator::parseLink($this->link($input));
     $this->assertEquals(is_null($expected) ? TRUE : $expected, $request_context['flatten']);
   }
 
   /**
-   * Data provider for testValueCallback().
+   * Input helper for testBooleans().
    */
   public function link($input) {
     return Url::fromRoute('fillpdf.populate_pdf', [], [
@@ -60,7 +45,7 @@ class ParseLinkTest extends UnitTestCase {
   }
 
   /**
-   * Data provider for testValueCallback().
+   * Data provider for testBooleans().
    */
   public function dataProvider() {
     return [
@@ -86,9 +71,12 @@ class ParseLinkTest extends UnitTestCase {
       ['No', FALSE],
       ['NO', FALSE],
 
+      // These three are important, so should always be obeyed:
+      ['', NULL],
       ['foo', NULL],
       ['bar', NULL],
-      // The following mappings are a bit unfortunate, so are no requirements:
+
+      // The following ones are less fortunate, so may be refactored:
       ['-1', NULL],
       ['2', NULL],
       ['y', NULL],