Hello everybody,

just sharing something I got stuck in today : when using fillpdf_merge_handle_pdf(), I realized that my generated PDF were always sent to browser, even if setting $action to "save" and $force_download to FALSE.

The reason: break instructions missing in the switch nearby line 545. It's currently :

switch ($action) {
    case 'redirect':
      $redirect_to_file = $pdf_info->destination_redirect;
    case 'save':
      fillpdf_save_to_file($pdf_info, $pdf_data, $token_objects, $output_name, !$force_download, $redirect_to_file);
    // Fill PDF classic!
    case 'download':
      drupal_add_http_header("Pragma", "public");
      drupal_add_http_header('Expires', 0);
      drupal_add_http_header('Cache-Control', 'must-revalidate, post-check=0, pre-check=0');
      drupal_add_http_header('Content-type', 'application-download');
      // This must be strlen(), not drupal_strlen() because the length in bytes,
      // not in characters, is what is needed here.
      drupal_add_http_header('Content-Length', strlen($pdf_data));
      drupal_add_http_header('Content-disposition', 'attachment; filename="' . $output_name . '"');
      drupal_add_http_header('Content-Transfer-Encoding', 'binary');
      echo $pdf_data;
      drupal_exit();
    break;

It should be :

switch ($action) {
    case 'redirect':
      $redirect_to_file = $pdf_info->destination_redirect;
    break;
    case 'save':
      fillpdf_save_to_file($pdf_info, $pdf_data, $token_objects, $output_name, !$force_download, $redirect_to_file);
    break;
    // Fill PDF classic!
    case 'download':
      drupal_add_http_header("Pragma", "public");
      drupal_add_http_header('Expires', 0);
      drupal_add_http_header('Cache-Control', 'must-revalidate, post-check=0, pre-check=0');
      drupal_add_http_header('Content-type', 'application-download');
      // This must be strlen(), not drupal_strlen() because the length in bytes,
      // not in characters, is what is needed here.
      drupal_add_http_header('Content-Length', strlen($pdf_data));
      drupal_add_http_header('Content-disposition', 'attachment; filename="' . $output_name . '"');
      drupal_add_http_header('Content-Transfer-Encoding', 'binary');
      echo $pdf_data;
      drupal_exit();
    break;

If someone knows the way to create a patch, I would be happy to make it.
I'm still a "patch -p0" dinosaur... :p

Comments

wizonesolutions’s picture

Version: 7.x-1.9 » 7.x-2.x-dev
Priority: Major » Normal
Issue tags: +Needs tests

Thanks for filing a bug, but they have to be filed against 7.x-1.x-dev (technically, they have to be against 7.x-2.x-dev, but this part of the code hasn't changed yet, I don't think).

That said, the break after 'save' is correct, but not after 'redirect'. The redirect action always. Therefore, it should fall through.

As for making a patch, the instructions are always on http://drupal.org/patch :)

Finally, I can't commit this anywhere without tests. I've developed the basic test setup against Fill PDF 7.x-2.x, and it would be trivial to backport. I don't necessarily expect you to backport that, but unless I get time to (it isn't the priority unless sponsored) or someone else does, I won't be able to commit the patch. Committing patches without adequate test support has burned me too many times.

This particular test should test each delivery method, including the case that was failing for you (a.k.a. it should assert that the PDF is *not* sent to the browser). It doesn't need a lot of assertions, just enough so that if a delivery method broke the tests would fail. For example, they would break if I put a break at the end of the redirect case.

All that said, thanks a lot for reporting this. I will mark this as 7.x-2.x-dev and fix it there first when I get to it.

I am lowering priority. I'll bump this back up if more people report this problem.

wizonesolutions’s picture

Issue summary: View changes

Orthograph

wizonesolutions’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Active » Closed (works as designed)

I've checked the code, and it's behaving properly. Redirection to a file only happens if it should, and redirection only happens at all if $force_download isn't TRUE. This makes sense, since if a download is forced, the last thing that happens has to be the file getting served to the user.

Also, redirecting to a file first has to save it, so it has to fall through to that condition.

Closing this since I think it's working in 7.x-1.x at least, and that's the basis for future versions.