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

NEW Import PDF export functionality from BasePage in cwp/cwp #1

Merged

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Jan 22, 2018

@robbieaverill robbieaverill force-pushed the pulls/1.0/import-from-cwp branch from 8d8cd31 to d0fbb6a Compare January 23, 2018 00:44
@robbieaverill
Copy link
Contributor Author

Noted issue: clicking the "Export to PDF" link for a page initially fails with this message:

Fatal error: wkhtmltopdf failed: Exit with code 1 due to network error: ContentNotFoundError in /webroot/vendor/cwp/cwp-pdfexport/src/Extensions/PdfExportControllerExtension.php on line 164

Subsequent requests succeed and open the PDF.

I suspect this could be a macOS issue with piping the wkhtmltopdf output to stdout. If we're happy, I think we should get this merge and raise that as a separate issue. We could write it to the PHP temp dir instead of piping to stdout.

Copy link
Contributor

@NightJar NightJar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, other than what appear to be some non-critical superfluous (and magic) function calls. Would you like to address them, or merge as is?

}

if (!$binaryPath) {
user_error('Neither WKHTMLTOPDF_BINARY nor BasePage.wkhtmltopdf_binary are defined', E_USER_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message needs to no reference BasePage anymore I think, it'll send people off on a wild goose chase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good call, will update

return false;
}

$path = $this->owner->getPdfFilename();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner is going the long way around for this call.

Copy link
Contributor Author

@robbieaverill robbieaverill Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this follows the standard practice for extensions edited there's almost no reason to do this so may as well update it. Will push.

@robbieaverill
Copy link
Contributor Author

I've raised an issue for the error I noted above here: #2

@NightJar NightJar merged commit 4c478a8 into silverstripe:master Jan 24, 2018
@NightJar NightJar deleted the pulls/1.0/import-from-cwp branch January 24, 2018 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants