Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should PDFs be rendered upon viewing orders in CP/generating PDF url? #962

Closed
chasegiunta opened this issue Aug 18, 2019 · 3 comments
Closed
Labels

Comments

@chasegiunta
Copy link

chasegiunta commented Aug 18, 2019

Description

PDFs are being rendered upon visiting the order details screen in the control panel or by calling order.pdfUrl in a template. Is this right?

$pdf = Plugin::getInstance()->getPdf()->renderPdfForOrder($this, $option);

Looks like it's doing as a method of validation, but seems odd. PDF generation isn't even an optional setting so shouldn't the assumption be that there will always be a PDF to render, given the correct number?

It may not be such a huge deal since domPdf can generate PDF's quite fast, but using other means of PDF generation could be timely.

I'm hooking into the EVENT_BEFORE_RENDER_PDF to render our own PDF via Browsershot, which uses node/puppeteer, so it takes a few seconds to complete. It's very noticeable when trying to browse orders or generate a URL.

I'm able to bypass it, for now at least, while browsing orders in control panel with this snippet below in the EVENT_BEFORE_RENDER_PDF event which will still output a working Download PDF button, and valid PDF url.

if (empty($request->queryParams['number'])) {
    return;
}

Additional info

  • Craft version: 3.2.10
  • Commerce version: 2.1.11
@lukeholder
Copy link
Member

lukeholder commented Aug 19, 2019

Thank @chasegiunta, you are correct, it is rendering the PDF to know whether to show the button or not. It's not ideal.

I just removed the pre-render for next release but added basic pre-validation of the on the existence of the PDF template.

@chasegiunta
Copy link
Author

@lukeholder FYI, while your fix did address the PDF rendering on every order page (and anything that uses order.pdfUrl) for me, it it also prevented the "Download PDF" button showing up on any order.

It's not showing up due to, literally any template path I pass into Order PDF Template not passing $view->doesTemplateExist($file)

if (!$file || !$view->doesTemplateExist($file)) {

It passes if it's just checking $file existence.

I believe since the request is coming from the Control Panel, resolveTemplate() method is trying to resolve the template relative to vendor/craftcms/cms/src/templates/ as described here: https://github.com/craftcms/cms/blob/55bc94128257ebf6d444c8fa5adfb8a06f45a0df/src/web/View.php#L672

@nfourtythree
Copy link
Contributor

Hi @chasegiunta

Thanks for the info, I believe it is, as you mentioned, not looking in the proper templating scope. It needs to be looking in the site scope.

Will look to get a fix for this pushed out.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants