Keep sales PDF item rows from splitting across a page break

[TuVanDev]

Description (*)

AbstractPdf::drawLineBlocks() has a block-level page-break check that is meant to move a whole item block to the next page when it no longer fits in the space left on the current one:

if ($this->y - $itemsProp['shift'] < 15) {
    $page = $this->newPage($pageSettings);
}

shift is documented on the method as "full line height (optional)", and when it is absent drawLineBlocks() computes it from the block's own lines. Every core item renderer, however, passes a hardcoded 'shift' => 5:

• Magento\Sales\Model\Order\Pdf\Items\Invoice\DefaultInvoice
• Magento\Sales\Model\Order\Pdf\Items\Shipment\DefaultShipment
• Magento\Bundle\Model\Sales\Order\Pdf\Items\Invoice
• Magento\Bundle\Model\Sales\Order\Pdf\Items\Shipment
• Magento\Bundle\Model\Sales\Order\Pdf\Items\Creditmemo

With shift = 5 the check is effectively dead: it only fires in the last 20pt of a page. An item block that does not fit in the remaining space is therefore started anyway and broken mid-block by correctText(), so a single item row is split across the page boundary. The product-name and option lines end up on one page while that same item's SKU, qty, price, tax and subtotal columns are drawn on the next one, where the numeric columns then line up against the wrong name fragment.

Magento\Sales\Model\Order\Pdf\Items\Creditmemo\DefaultCreditmemo no longer has the hardcoded value: it was removed as part of ACP2E-4630 (commit 545981c), which fixed the related overlapping-text defect by returning the new page out of correctText() / correctLines(). This pull request finishes that work for the remaining five renderers.

Removing 'shift' => 5 on its own is not enough, because a block taller than one usable page then makes the check true even at the top of a fresh page: newPage() starts drawing at y=800 and the guard is < 15, so a block over ~785pt (less the table-header row) can never satisfy it. The break fires, the page that was just created is left with nothing but its table header, and the block overflows across the boundary anyway. Two additions prevent that:

• fitsOnEmptyPage() suppresses the break when the block cannot fit on an empty page. Breaking gains nothing there, it only leaves the rest of the current page empty.
• createPage() records the Y coordinate each new page starts drawing at, so fitsOnEmptyPage() compares against the real top of the current page (a table header lowers it) rather than an assumed one. For a page created elsewhere, such as the first page of each document built in getPdf(), only the un-lowered coordinate newPage() sets is known, and that is used instead.

Together these make an empty page structurally impossible: on a page that has nothing drawn on it yet $this->y equals that page's recorded top, so $this->y - $shift < 15 and $shift <= $pageTop - 15 are exact negations of each other and the break cannot fire.

That also removes a latent empty-page case in the credit memo renderer, which has had the computed shift since ACP2E-4630 but no such guard: an item whose option block is taller than a page currently gets pushed to a fresh page, leaving the credit memo's first page empty below its table header, and still overflows. Measured here at 3 pages before this change and 2 after.

The two literals the check depends on (800 from newPage() and 15) are now named constants, since the new guard has to agree with both.

Related Pull Requests

This is the same change I opened upstream as magento/magento2#41061 (base 2.4-develop), where it is still open and unreviewed.

To be straightforward about that: Mage-OS syncs magento/magento2 daily through .github/workflows/merge-upstream-changes.yml, so if Adobe merges #41061 this fix will arrive here on its own and this pull request would be redundant. It is offered now only because that outcome is not in my hands and Adobe's queue is long. Nothing here is novel relative to #41061 - the diff is the same change re-applied to Mage-OS main file content and independently re-verified against it. Maintainers should feel free to close this in favour of the upstream sync if that is the preference; taking it now only buys the fix earlier and, if Adobe later merges #41061, the sync of that commit should be a no-op or a trivial conflict since the content is identical.

It also extends ACP2E-4630 (commit 545981c), already present on main, which fixed the overlapping-text half of this code path and removed the hardcoded shift from the credit memo renderer only.

Fixed Issues (if relevant)

None. I searched mage-os/mageos-magento2 issues and pull requests (open and closed) for a matching report and found none. Found while investigating multi-page invoice PDFs.

Manual testing scenarios (*)

1. Create an order with several products whose custom options render a long option list (about 10-12 options per item is enough), so that a single item block is a few hundred points tall.
2. Invoice the order and open Sales > Invoices > View > Print. Repeat for a shipment (packing slip), a credit memo, and the Print Invoices mass action over several orders.
3. Before the change: at each page boundary an item is cut in half. Its option lines continue at the top of the next page with no product name, SKU, qty, price or subtotal row, and those columns for that item are back on the previous page.
4. After the change: every page boundary falls between items. A page begins with a complete item row (name, SKU, qty, price, tax, subtotal) and no item's option lines are separated from its own columns.
5. Add one item with an option list long enough to exceed a whole page (about 25 options). The page count must not increase versus before the change, and no page may be produced that contains only the item-table header row.
6. Print an invoice, packing slip and credit memo that already fit on a single page. They must be unchanged.

Verification

Verified against Mage-OS main file content, not just the upstream branch. The seven target files on main are byte-identical to the 2.4-develop revision the change was originally written against, so the patch applies with no offset, but the render verification was re-run from scratch with the Mage-OS copies of the classes overlaid over a real store, over real invoices, packing slips, credit memos, order prints and a 5-invoice mass action, plus synthetic option blocks sized just under and just over one page.

Raw PDF bytes are not reproducible run to run, so comparison is on extracted per-page draw runs (page, y, x, string) rather than file hashes.

scenario	pages before > after	item rows split across a page break, before > after	pages holding only the table header, before > after
invoice, 41 items	9 > 11	8 > 0	0 > 0
packing slip, 21 items	9 > 11	7 > 0	0 > 0
credit memo, 18 items	2 > 2	0 > 0	0 > 0
order print, 23 items	3 > 3	1 > 0	0 > 0
mass action, 5 invoices	17 > 19	10 > 0	0 > 0
synthetic block that fits a fresh page, invoice	16 > 22	15 > 0	0 > 0
synthetic block that fits a fresh page, packing slip	15 > 22	13 > 0	0 > 0
synthetic block that fits a fresh page, credit memo	12 > 12	0 > 0	0 > 0
synthetic block that fits a fresh page, order print	17 > 24	14 > 0	0 > 0
block taller than a page, invoice	2 > 2	1 > 1	0 > 0
block taller than a page, packing slip	2 > 2	1 > 1	0 > 0
block taller than a page, credit memo	3 > 2	1 > 1	0 > 0
block taller than a page, order print	2 > 2	1 > 1	0 > 0
two consecutive blocks taller than a page, invoice	17 > 22	15 > 3	0 > 0
single-item invoice / packing slip / credit memo / order print	1 > 1	0 > 0	0 > 0

Also checked across every case above: zero text runs drawn on top of a page's document-header block, before or after.

The remaining split on a block taller than a page is unavoidable: it cannot fit anywhere, so it is drawn from where it stands instead of costing an extra page. Single-page documents draw exactly the same text at exactly the same coordinates as before, verified as an identical draw-run dump. Page counts rise on documents whose items were previously being cut in half, which is what the check is for: the extra pages buy back the item rows that were being broken.

Order print rows are a project subclass of Magento\Sales\Model\Order\Pdf\Invoice that routes through the same core DefaultInvoice renderer and drawLineBlocks(); they are an extra data point on the same code path, not a separate core document type.

Not render-verified: the three Magento\Bundle\Model\Sales\Order\Pdf\Items\* edits. The catalog available for verification has no bundle products, so those renderers were never instantiated in any of the runs above. Each of the three is the identical single-line removal of 'shift' => 5 from the $drawItems[] / $draw[] array that was verified on the non-bundle renderers, and they call the same drawLineBlocks(). Worth a look from anyone with a bundle catalog to hand.

Questions or comments

fitsOnEmptyPage() and createPage() are private, which keeps them out of the public surface but also means a third-party subclass of AbstractPdf that creates pages by calling newPage() directly gets the PAGE_TOP_Y fallback rather than a recorded coordinate. That is the conservative direction (it can only under-estimate how much room a page has, never over-estimate) but if a protected seam is preferred here I am happy to change it.

Unit coverage: Magento\Sales\Test\Unit\Model\Order\Pdf\AbstractTest::testDrawLineBlocksBreaksPageOnlyForABlockThatFits covers both halves of the new guard. Against unmodified main content it fails with Zend_Pdf::newPage('595:842:', null) was not expected to be called. on the "block taller than an empty page stays where it is" data set, and passes with this change. The three pre-existing tests in that file still pass.

phpcs --standard=Magento2 --warning-severity=1 reports 0 errors and 0 warnings on all seven changed files.

CI status

Coding Standard, Unit Tests and Integration Tests are all sitting at action_required on this pull request: as a fork PR they need a maintainer to approve the workflow runs. Happy to push again if anything comes back red once they do.

The Sansec eComscan check reports failure, but that is not this change. It is a pull_request_target workflow whose actions/checkout step refuses to check out fork code (Refusing to check out fork pull request code from a 'pull_request_target' workflow), so it fails on every fork PR - including recently merged ones such as #297.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update - none of the modified modules have a README section affected by this change
• All automated tests passed successfully (all builds are green) - the three test/standard workflows are awaiting maintainer approval to run, see the CI status section above. Locally against Mage-OS main file content: phpcs --standard=Magento2 --warning-severity=1 is 0/0 on all seven files, and the full Magento\Sales\Test\Unit\Model\Order\Pdf\AbstractTest suite passes (5 tests, 24 assertions) on PHPUnit 9.6.

[rhoerr]

In the future, can you keep notes shorter/more concise? Appreciate all the context and test data, but it's a lot to parse. Thanks

[rhoerr]

Thanks for the thorough PR. One issue: Mage-OS main pins phpunit/phpunit: ^12.0, and PHPUnit 10+ dropped docblock annotation support. That means the new test's @dataProvider is ignored and it errors with ArgumentCountError: Too few arguments ... 0 passed.

1. Replace the @dataProvider docblock line with #[DataProvider('blockHeightDataProvider')]
2. Make blockHeightDataProvider() static (required in PHPUnit 10+)

The rest of Magento\Sales\Test\Unit already uses attributes. Everything else in the PR checked out on review.