Prince XML PDF library

Project page

As per the issue you linked to in the 'print' queue, this is a non-free library. You should clearly announce on the project page that this solution puts a logo on every page until you pay for it.

You should also state that this requires root-like powers on your server to install the binary (as do some of the other solutions AFAIK, but note this dependency clearly on the project page.

Visual code check:

.install : looks fine
.pages.inc : looks god. small clean code, fine comments.
.admin.inc : ditto. Correct API etc.
.module : small and well structured. Would like a comment on print_pdf_princexml_print_pdf_available_libs_alter() as that currently looks like black magic out of context. I'm sure it makes sense though.

Automated code check

There is a small amount of noise from the PAReview check, but certainly nothing I'd worry about.

Functional testing

You add this utility in a new section of modules admin called 'AYALON'. While cute, it's not helpful when it should probably be with the other 'PRINTER, EMAIL AND PDF VERSIONS' modules.

It wasn't too hard to find my way to the settings at /admin/config/user-interface/print/pdf/princexml , though a link to the rest of the install instructions there (link to the download site at least) would be handy in the UI, not just the README.

You could (potentially) add a .make file to assist the download of the wrapper class into /libraries, but given that we need to do a server-side install,

Security issue

The path to binary setting on admin/config/user-interface/print/pdf/princexml is not validated and in fact allows any binary server-side code to be run.
Looking at the way this is used, and prince.php, this input is NOT CHECKED OR SANITIZED in any way and basically means and command can be run there.
This is a huge security thing. It's not the fault of the prince.php library, it's just that the print_pdf_prince_binary value needs protection. Somehow.
writing secure code

System warning

After installing the binary and giving the path to it, my system report at admin/reports/status announced

Printer, email and PDF versions - PDF generation library Unsupported princexml version
The currently selected version of princexml ((unknown version)) is not supported. Please update to a newer version.

(I installed print.module 7.x-2.x - not sure if that's an API change that it made)
But otherwise print.module configs seemed to recognize it.

After all that, I can confirm that the module actually works and produces PDFs using this library. I won't go playing with the layouts, but I'll believe that it provides the additional control you were looking for.

Summary

Please take some steps to mitigate the potential security issue around allowing the webserver to execute ANY commandline command (this is a blocker), and sort out the system warning in the site status report.
The rest of the code and architecture is fine by me though.

Please check the release naming conventions as you are currently using the master branch. http://drupal.org/node/1015226

You also have some things to fix per the automated code review at http://ventral.org/pareview/httpgitdrupalorgsandboxayalon1733058git

FILE: .../all/modules/pareview_temp/test_candidate/print_pdf_princexml.admin.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
21 | WARNING | A comma should follow the last multiline array item. Found: )
30 | WARNING | A comma should follow the last multiline array item. Found: )
--------------------------------------------------------------------------------

FILE: ...sites/all/modules/pareview_temp/test_candidate/print_pdf_princexml.info
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
7 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
8 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
9 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
10 | ERROR | Declared file was not found
12 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...tes/all/modules/pareview_temp/test_candidate/print_pdf_princexml.module
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AND 7 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
18 | WARNING | A comma should follow the last multiline array item. Found:
| | FALSE
33 | WARNING | A comma should follow the last multiline array item. Found:
| | 'print_pdf_princexml_settings'
36 | WARNING | A comma should follow the last multiline array item. Found:
| | 'administer print'
39 | WARNING | A comma should follow the last multiline array item. Found:
| | 'print_pdf_princexml.admin.inc'
58 | ERROR | Additional blank line found at the end of doc comment
72 | ERROR | Additional blank line found at the end of doc comment
73 | ERROR | Function doc comment must end on the line before the function
| | definition
75 | ERROR | print_pdf_princexml_requirements() is an installation hook and
| | must be declared in an install file
79 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
80 | ERROR | There must be no space before the colon in a CASE statement
91 | WARNING | A comma should follow the last multiline array item. Found:
| | $tool
93 | WARNING | A comma should follow the last multiline array item. Found:
| | REQUIREMENT_ERROR
100 | WARNING | A comma should follow the last multiline array item. Found:
| | $tool
--------------------------------------------------------------------------------

FILE: .../all/modules/pareview_temp/test_candidate/print_pdf_princexml.pages.inc
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
18 | ERROR | "require_once" is a statement not a function; no parentheses are
| | required
18 | ERROR | Language constructs must be followed by a single space; expected
| | "require_once (" but found "require_once("
22 | ERROR | No space before comment text; expected "// Secure binary path"
| | but found "//Secure binary path"
22 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
26 | ERROR | No space before comment text; expected "// Load the wrapper
| | class" but found "//Load the wrapper class"
26 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
28 | ERROR | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found
| | "true"
29 | ERROR | No space before comment text; expected "// Enable debugging" but
| | found "//Enable debugging"
30 | ERROR | No space before comment text; expected "//
| | $prince->setLog("log.txt");" but found
| | "//$prince->setLog("log.txt");"
41 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Link to the project page and git clone command are missing in the issue summary, please add them.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

The module is working for me - straight after installing.

As an end-user, it doesn't seem apparent how or where to set the PDF settings a you state:

6. All settings to the pdf output can be made via CSS Style sheet.

Could you provide a bit more documentation on how this works? The PrinceXML documentation seems ok, but the disconnect between your documentation on the Drupal side and what makes PrinceXML work is too great. Where does the stylesheet go? Would it make sense for you to provide a default stylesheet with some sane settings?

All the best,

Richard

With this module installed, when I go to /admin/reports/status, I see this:

Here's a patch to fix the requirements screen issues. This had 2 problems:

1) The function to get the prince binary version was just a stub.
2) the hook_requirements implementation was using the same keys as print_pdf.

Also, the .install file is wrongly named (print_pdf_prince.install, not print_pdf_princexml.install). I spotted that after I made the patch though, so that's not included.

I should add that we're using this module in production and it works well :)

I have applied rupertj's patch on a my local dev site and it all works fine, so you could consider that patch itself as RTBC'd.

Now would be a good time for you to at least deal with adding that patch and demonstrating the proactive way you will be addressing your issue queue!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.