Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix and clean up the WeasyPrint PDF engine integration by aligning the engine implementation/tests/docs with WeasyPrint’s actual CLI capabilities, and by updating CI to ensure the binary is available when running the test suite.
Changes:
- Renames/fixes the WeasyPrint engine class naming (
WeasyPrintEngine) and simplifies command generation to avoid wkhtmltopdf-only flags. - Replaces the WeasyPrint engine test file and updates command expectations.
- Updates README messaging/examples and installs
weasyprintin CI.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Pdf/Engine/WeasyPrintEngine.php |
Renames engine class and updates command/options parsing and error handling. |
tests/TestCase/Pdf/Engine/WeasyPrintEngineTest.php |
Updates test coverage/expectations for the new command generation behavior. |
README.md |
Updates engine recommendations and configuration docs/examples (currently contains inconsistencies). |
.github/workflows/ci.yml |
Installs weasyprint in CI to support engine tests. |
Comments suppressed due to low confidence (5)
src/Pdf/Engine/WeasyPrintEngine.php:127
- In
_getCommand(), theif (!$value) { continue; }check will skip valid option values like0/'0'(e.g.--zoom 0), even thoughparseOptions()supportsint. Consider only skippingnull/false/empty string (and possibly empty arrays), while allowing0to be emitted as an argument.
src/Pdf/Engine/WeasyPrintEngine.php:46 - The
output()docblock has duplicate@throwstags for the same exception type (one generic and one specific). Consolidate into a single@throws \Cake\Core\Exception\CakeExceptionentry with an appropriate description to avoid contradictory/duplicated API docs.
src/Pdf/Engine/WeasyPrintEngine.php:147 - The
parseOptions()doc comment says it was created to reuse logic forcover/toc, but that special handling has been removed. Also, the annotated typearray<string, mixed>suggests associative arrays, yet the implementation ignores keys and emits only the values. Update the comment/type to reflect the current behavior (e.g. list of values for repeated flags).
README.md:108 - This README example config sets
enginetoCakePdf.WeasyPrintbut still demonstratesmarginandorientation. SinceWeasyPrintEngineno longer translates these settings to CLI args (and WeasyPrint generally expects page layout via CSS@page), this example is likely misleading. Either adjust the example to a wkhtmltopdf engine, or update the example text to show the CSS approach for margins/orientation when using WeasyPrint.
Configure::write('CakePdf', [
'engine' => 'CakePdf.WeasyPrint',
'margin' => [
'bottom' => 15,
'left' => 50,
'right' => 30,
'top' => 45,
],
'orientation' => 'landscape',
'download' => true,
README.md:172
- The engine-options example is configured for
CakePdf.WeasyPrint, but it still includes wkhtmltopdf-specific options (cover,toc, andenable-smart-shrinking) and even the commentedwkhtmltopdfbinary paths. These options won’t apply to WeasyPrint and will confuse users. Consider either switching this example back toCakePdf.WkHtmlToPdfor rewriting it with actual WeasyPrint CLI options (and adjusting the binary-path comments accordingly).
```php
Configure::write('CakePdf', [
'engine' => [
'className' => 'CakePdf.WeasyPrint',
// Options usable depend on the engine used.
'options' => [
'dpi' => 96,
'cover' => [
'url' => 'cover.html',
'enable-smart-shrinking' => true,
],
'toc' => true,
],
/**
* For Mac OS X / Linux by default the `wkhtmltopdf` binary should
* be available through environment path or you can specify location as:
*/
// 'binary' => '/usr/local/bin/wkhtmltopdf',
/**
* On Windows the engine uses the path shown below as default.
* You NEED to use the path like old fashioned MS-DOS Paths,
* otherwise you will get error like:
* "WKHTMLTOPDF didn't return any data"
*/
// 'binary' => 'C:\\Progra~1\\wkhtmltopdf\\bin\\wkhtmltopdf.exe',
],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to clean up and correct the WeasyPrint PDF engine integration by aligning naming, simplifying CLI option handling, and updating docs/CI to reflect WeasyPrint usage.
Changes:
- Renames the engine/test naming from
Weasyprint*toWeasyPrint*and updates the engine command generation to use encoding-based defaults. - Updates README guidance/examples to recommend WeasyPrint and adjusts configuration notes.
- Updates CI to install
weasyprintso engine-related tests can run in GitHub Actions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Pdf/Engine/WeasyPrintEngine.php |
Renames the engine class and changes how CLI options are assembled for weasyprint. |
tests/TestCase/Pdf/Engine/WeasyPrintEngineTest.php |
Replaces the prior test with updated expectations for the new command builder behavior. |
README.md |
Updates engine recommendations and configuration examples/documentation for WeasyPrint. |
.github/workflows/ci.yml |
Ensures weasyprint is installed in CI. |
Comments suppressed due to low confidence (5)
src/Pdf/Engine/WeasyPrintEngine.php:10
- Renaming the public engine class from
WeasyprintEnginetoWeasyPrintEngineis a backwards-incompatible change for consumers referencing the old class name (and for configs usingCakePdf.Weasyprint). Consider adding a backwards-compatible alias (e.g., keepingWeasyprintEngineas a thin subclass orclass_alias) with a deprecation path.
src/Pdf/Engine/WeasyPrintEngine.php:47 - The
output()docblock now lists@throws CakeExceptiontwice (one generic and one for the "no output" case). Since both throws are the same type, consider consolidating into a single@throwsentry (and keep@returnafter the@throwstags) to avoid redundancy.
src/Pdf/Engine/WeasyPrintEngine.php:137 parseOptions()no longer contains any special handling forcover/toc, but the docblock/comment still says it was created to reuse logic for those options. Please update the comment/docblock to reflect the current behavior to avoid misleading future changes.
README.md:171- The README now uses
CakePdf.WeasyPrintin this example, but the shown options (dpi,cover,toc) and the subsequent binary-path comments arewkhtmltopdf-specific and don’t match howWeasyPrintEnginebuilds its CLI command. Either switch this example back toCakePdf.WkHtmlToPdfor update the options/comments to valid WeasyPrint CLI flags + weasyprint binary paths.
'className' => 'CakePdf.WeasyPrint',
// Options usable depend on the engine used.
'options' => [
'dpi' => 96,
'cover' => [
'url' => 'cover.html',
'enable-smart-shrinking' => true,
],
'toc' => true,
],
/**
* For Mac OS X / Linux by default the `wkhtmltopdf` binary should
* be available through environment path or you can specify location as:
*/
// 'binary' => '/usr/local/bin/wkhtmltopdf',
/**
* On Windows the engine uses the path shown below as default.
* You NEED to use the path like old fashioned MS-DOS Paths,
* otherwise you will get error like:
* "WKHTMLTOPDF didn't return any data"
*/
// 'binary' => 'C:\\Progra~1\\wkhtmltopdf\\bin\\wkhtmltopdf.exe',
README.md:108
- With
engineset toCakePdf.WeasyPrint, this example config still setsmarginandorientation, butWeasyPrintEnginedoes not translate these CakePdf settings into CLI flags (and the README already notes some settings require CSS for WeasyPrint). Consider either removing these settings from the WeasyPrint example or adding a note that margin/orientation must be controlled via CSS@pagewhen using WeasyPrint.
Configure::write('CakePdf', [
'engine' => 'CakePdf.WeasyPrint',
'margin' => [
'bottom' => 15,
'left' => 50,
'right' => 30,
'top' => 45,
],
'orientation' => 'landscape',
'download' => true,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR cleans up and updates the WeasyPrint PDF engine implementation and its surrounding developer experience (tests, docs, CI), aligning command generation with the updated engine behavior.
Changes:
- Renames the WeasyPrint engine class/test and updates command-generation expectations.
- Simplifies WeasyPrint CLI option parsing and defaults (notably
--encoding). - Updates README guidance and CI packages to include WeasyPrint in the test environment.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Pdf/Engine/WeasyPrintEngine.php |
Renames engine class and rewrites option parsing/command construction. |
tests/TestCase/Pdf/Engine/WeasyPrintEngineTest.php |
Replaces the old test file with updated expectations and naming. |
tests/TestCase/Pdf/Engine/WeasyprintEngineTest.php |
Removes the legacy-cased test file. |
README.md |
Updates engine recommendations and configuration examples. |
.github/workflows/ci.yml |
Installs WeasyPrint in CI to enable running the WeasyPrint tests. |
Comments suppressed due to low confidence (3)
src/Pdf/Engine/WeasyPrintEngine.php:10
- Renaming the engine class from
WeasyprintEnginetoWeasyPrintEngineis a backwards-incompatible change for both direct class references and for configs likeCakePdf.Weasyprint(CakePHP will resolve that toWeasyprintEngineunder PSR-4). Consider adding a deprecated shim class/file (WeasyprintEngineextendingWeasyPrintEngine) or mapping the old engine alias to the new class to avoid breaking existing users.
src/Pdf/Engine/WeasyPrintEngine.php:146 parseOptions()treats any array option as a list of values and ignores array keys. The PHPDoc currently advertisesarray<string, mixed>(associative) which is misleading, and callers passing associative arrays (like the README example forcover) will now generate incorrect CLI args silently. Either validate/reject non-list arrays (throwing a clear exception) and document the expectedlist<scalar>shape, or restore explicit support for associative key/value arrays.
README.md:150- This configuration example shows
cover/tocand other wkhtmltopdf-style options underCakePdf.WeasyPrint, butWeasyPrintEngine::parseOptions()no longer supports those semantics (and WeasyPrint CLI doesn’t usecover/tocsubcommands). Please update the README example to options actually supported by WeasyPrint, or clearly label this block as wkhtmltopdf-specific.
'engine' => [
'className' => 'CakePdf.WeasyPrint',
// Options usable depend on the engine used.
'options' => [
'dpi' => 96,
'cover' => [
'url' => 'cover.html',
'enable-smart-shrinking' => true,
],
'toc' => true,
],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
No description provided.