This issue is automatically created based on existing pull request: #41160: Fix division by zero in Weee item price renderer when qty_ordered is 0
Description (*)
Magento\Weee\Block\Item\Price\Renderer computes a per-unit base price by dividing the row total by the ordered quantity before checking whether Weee/FPT is even enabled:
$qty = $orderItem->getQtyOrdered();
$basePriceExclTax = $orderItem->getBaseRowTotal() / $qty;
if (!$this->weeeHelper->isEnabled($this->getStoreId())) {
return $basePriceExclTax;
}This appears identically in getBaseUnitDisplayPriceExclTax() (line 246) and getBaseFinalUnitDisplayPriceExclTax() (line 467).
If a sales_order_item row has qty_ordered = 0 while base_row_total is non-zero, the division throws DivisionByZeroError. Because it happens before the isEnabled() check, the admin order, invoice and credit memo views return HTTP 500 even on stores where Weee/FPT is switched off entirely, and since the order view will not open at all, the offending line cannot be corrected from the admin UI.
On reachability, to be straight about it: I could not find a default flow that creates this data shape. Storefront checkout and admin order create both enforce a quantity of at least 1, and order create renders through the cart-zone block rather than this one, so a stock install driven through the UI will not hit it. We hit it because a third-party admin order-editing extension recalculated a fully refunded line to qty_ordered = 0 while leaving base_row_total untouched. That extension bug is ours to fix.
What makes this worth fixing in core anyway is that the class is inconsistent with itself. It already guards this exact division in eight other places (lines 109, 133, 225, 255, 353, 373, 451, 475); these two are the only unguarded ones left, and both sit before the isEnabled() check, so a merchant with FPT switched off entirely still loses the page. Magento\Sales\Model\OrderRepository::save() applies no quantity validation, so core will persist the shape for any API consumer, and then core cannot render it.
Note that the natural-looking guard $qty ?: 1 does not fix this. getQtyOrdered() returns a numeric string such as '0.0000', which PHP treats as truthy:
$ php -r 'var_dump((bool)"0.0000");'
bool(true)
so $qty ?: 1 still evaluates to '0.0000' and still divides by zero. The guard has to be numeric.
This PR fixes both methods:
$qty = (float)$orderItem->getQtyOrdered();
$basePriceExclTax = $qty > 0
? $orderItem->getBaseRowTotal() / $qty
: (float)$orderItem->getBaseRowTotal();Why not just call getItemQtyForUnitPriceCalculation()?
That helper already exists in this class and already ends return $qty > 0 ? $qty : 1.0;, and both of these methods already use it, guarded, a few lines further down. Swapping it in for the unguarded division would be a smaller diff, but it would also change behaviour beyond the crash: the helper reads $item->getQty() for InvoiceItem and CreditMemoItem, whereas the code being fixed deliberately unwraps to the order item and reads getQtyOrdered(). On a partially invoiced order those are different numbers, so using the helper here would change displayed unit prices on invoice and credit memo views. This PR keeps the existing quantity semantics and changes only the divide-by-zero case.
The fallback value is chosen to agree with that helper's convention: when the quantity is zero the helper yields 1.0, and dividing the base row total by 1.0 gives the base row total, which is exactly what this patch returns.
For context on the surrounding convention, the same class already guards divisions this way in eight other places (lines 109, 133, 225, 255, 353, 373, 451, 475). These two are the only unguarded divisions left in it.
Related Pull Requests
Same fix submitted to Mage-OS: mage-os/mageos-magento2#329
Fixed Issues (if relevant)
N/A. No existing issue was found. Searched magento/magento2 issues and PRs for DivisionByZeroError Weee, getBaseUnitDisplayPriceExclTax, getQtyOrdered division and weee division by zero; every division-by-zero result was for an unrelated file. Search coverage is not proof of absence, so please close this as a duplicate if a prior report exists.
Manual testing scenarios (*)
- Take an order whose
sales_order_item row has qty_ordered = 0 and a non-zero base_row_total. This can be set directly for the purposes of reproducing:
UPDATE sales_order_item SET qty_ordered = 0 WHERE item_id = <a row with base_row_total > 0>;
- Open that order in the admin panel (Sales > Orders > View), which renders
Magento\Weee\Block\Item\Price\Renderer.
- Before: the page returns HTTP 500 and
var/log/exception.log records DivisionByZeroError: Division by zero at Renderer.php:247 (or :468 for the final-price method). This reproduces whether or not Weee/FPT is enabled for the store, since the division precedes the isEnabled() check.
- After: the page renders, showing the base row total as that line's unit price instead of throwing.
- Repeat on the invoice and credit memo views for the same order, which render the same block.
- Regression: run
Magento\Weee\Test\Unit\Block\Item\Price\RendererTest. The existing cases (non-zero quantity, Weee enabled and disabled, include-Weee flag on and off) are unchanged.
Questions or comments
Two things I would welcome direction on.
First, the fallback. Returning the base row total keeps the class's own : 1.0 convention, but returning 0.0 is arguably more honest for a line with no quantity. I went with the former for consistency; happy to change it.
Second, whether the maintainers would prefer the two methods to be refactored onto getItemQtyForUnitPriceCalculation() as a follow-up, accepting the invoice and credit memo quantity change described above. That felt out of scope for a crash fix, but it would leave the class with one quantity source instead of two.
Contribution checklist (*)
This issue is automatically created based on existing pull request: #41160: Fix division by zero in Weee item price renderer when qty_ordered is 0
Description (*)
Magento\Weee\Block\Item\Price\Renderercomputes a per-unit base price by dividing the row total by the ordered quantity before checking whether Weee/FPT is even enabled:This appears identically in
getBaseUnitDisplayPriceExclTax()(line 246) andgetBaseFinalUnitDisplayPriceExclTax()(line 467).If a
sales_order_itemrow hasqty_ordered = 0whilebase_row_totalis non-zero, the division throwsDivisionByZeroError. Because it happens before theisEnabled()check, the admin order, invoice and credit memo views return HTTP 500 even on stores where Weee/FPT is switched off entirely, and since the order view will not open at all, the offending line cannot be corrected from the admin UI.On reachability, to be straight about it: I could not find a default flow that creates this data shape. Storefront checkout and admin order create both enforce a quantity of at least 1, and order create renders through the cart-zone block rather than this one, so a stock install driven through the UI will not hit it. We hit it because a third-party admin order-editing extension recalculated a fully refunded line to
qty_ordered = 0while leavingbase_row_totaluntouched. That extension bug is ours to fix.What makes this worth fixing in core anyway is that the class is inconsistent with itself. It already guards this exact division in eight other places (lines 109, 133, 225, 255, 353, 373, 451, 475); these two are the only unguarded ones left, and both sit before the
isEnabled()check, so a merchant with FPT switched off entirely still loses the page.Magento\Sales\Model\OrderRepository::save()applies no quantity validation, so core will persist the shape for any API consumer, and then core cannot render it.Note that the natural-looking guard
$qty ?: 1does not fix this.getQtyOrdered()returns a numeric string such as'0.0000', which PHP treats as truthy:so
$qty ?: 1still evaluates to'0.0000'and still divides by zero. The guard has to be numeric.This PR fixes both methods:
Why not just call
getItemQtyForUnitPriceCalculation()?That helper already exists in this class and already ends
return $qty > 0 ? $qty : 1.0;, and both of these methods already use it, guarded, a few lines further down. Swapping it in for the unguarded division would be a smaller diff, but it would also change behaviour beyond the crash: the helper reads$item->getQty()forInvoiceItemandCreditMemoItem, whereas the code being fixed deliberately unwraps to the order item and readsgetQtyOrdered(). On a partially invoiced order those are different numbers, so using the helper here would change displayed unit prices on invoice and credit memo views. This PR keeps the existing quantity semantics and changes only the divide-by-zero case.The fallback value is chosen to agree with that helper's convention: when the quantity is zero the helper yields
1.0, and dividing the base row total by1.0gives the base row total, which is exactly what this patch returns.For context on the surrounding convention, the same class already guards divisions this way in eight other places (lines 109, 133, 225, 255, 353, 373, 451, 475). These two are the only unguarded divisions left in it.
Related Pull Requests
Same fix submitted to Mage-OS: mage-os/mageos-magento2#329
Fixed Issues (if relevant)
N/A. No existing issue was found. Searched
magento/magento2issues and PRs forDivisionByZeroError Weee,getBaseUnitDisplayPriceExclTax,getQtyOrdered divisionandweee division by zero; every division-by-zero result was for an unrelated file. Search coverage is not proof of absence, so please close this as a duplicate if a prior report exists.Manual testing scenarios (*)
sales_order_itemrow hasqty_ordered = 0and a non-zerobase_row_total. This can be set directly for the purposes of reproducing:Magento\Weee\Block\Item\Price\Renderer.var/log/exception.logrecordsDivisionByZeroError: Division by zeroatRenderer.php:247(or:468for the final-price method). This reproduces whether or not Weee/FPT is enabled for the store, since the division precedes theisEnabled()check.Magento\Weee\Test\Unit\Block\Item\Price\RendererTest. The existing cases (non-zero quantity, Weee enabled and disabled, include-Weee flag on and off) are unchanged.Questions or comments
Two things I would welcome direction on.
First, the fallback. Returning the base row total keeps the class's own
: 1.0convention, but returning0.0is arguably more honest for a line with no quantity. I went with the former for consistency; happy to change it.Second, whether the maintainers would prefer the two methods to be refactored onto
getItemQtyForUnitPriceCalculation()as a follow-up, accepting the invoice and credit memo quantity change described above. That felt out of scope for a crash fix, but it would leave the class with one quantity source instead of two.Contribution checklist (*)
testGetBaseUnitDisplayPriceExclTaxWithZeroQtyOrderedandtestGetBaseFinalUnitDisplayPriceExclTaxWithZeroQtyOrderedtoRendererTest, each covering both an integer0and the numeric-string'0.0000'thatgetQtyOrdered()actually returns. Each asserts a value the unpatched code cannot return, because the same line throws first.php -lclean and the new tests were written against the existing suite's mocks and#[DataProvider]style, but I have not executed the suite locally. Relying on CI here.