Prevent string * int multiplification issue in PHP 8.0+

[lamskoy]

Description (*)

I faced with fatal errors while upgrading native magento 1.9.x to PHP8.0 locally:

After debugging I found the issue 1:

echo '' * 1;

gives:

Fatal error: Uncaught TypeError: Unsupported operand types: string * int

Issue 2:

round('')

gives:

Fatal error: Uncaught TypeError: round(): Argument #1 ($num) must be of type int|float, string given

So patched core files with missing methods which should be casted to float.

Wanna share fix with you. Hope this helps to make whole code better.

Manual testing scenarios (*)

This is potential issue with PHP8, to be checked with auto tests

Questions or comments

This stuff gives small performance improvement coz we get rid of slow preg_replace, 2 substr and 1 strtolower calls after changes. Coz we use _getData() instead of magic method __call now

Varien_Object:

   protected function _underscore($name)
    {
...
        $result = strtolower(preg_replace('/(.)([A-Z])/', "$1_$2", $name));
  ...
    }

    public function __call($method, $args)
    {
        switch (substr($method, 0, 3)) {
            case 'get' :
...
                $key = $this->_underscore(substr($method,3));
                $data = $this->getData($key, isset($args[0]) ? $args[0] : null);

So on quote calculation we can save much calls to preg_match (even if count that _underscore caches calls)
If there are N calls to different methods getBlabla during quote recalculation, we save
N * preg_replace, N*2 substr, N strtolower calls
Not bad if N > 20 :)

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

Should we use type hinting for newly introduced methods?

[lamskoy]

Should we use type hinging for newly introduced methods?

You mean this
public function setSomething(float $value)
public function getSomething(): float

instead of this?

public function setSomething($value)
public function getSomething()

If yes, then we can have issues with 3rd party extensions overriding getSomething()

We'll get error like "Uncompatible method declaration"

Sample here: https://3v4l.org/V8r2e

I don't mind of course if we don't need any compatibility :)

[luigifab]

Interesting changes!

I'm sorry, but you have forgotten a new empty line here before the comment:

[lamskoy]

Interesting changes!

Here you go with updated empty lines. Just pushed commit

[lamskoy]

Does it make sense to fix the type at the source, make PDO returns numeric types from numeric columns? Ref https://write.corbpie.com/how-to-return-integers-and-floats-from-mysql-with-php-pdo/ and https://stackoverflow.com/questions/1197005/how-to-get-numeric-types-from-mysql-using-pdo

Some of fields are set/get dynamically by observers/tax calculators, think there's no big sense to make PDO returns in this case

[Flyingmana]

with this amount of new methods I wonder if its not easier, to extend the getData, setData logic to force types for specific fields instead of overwriting the methods. So in the end we just have a configurable array.

[tmotyl]

Dedicated methods have a big advantage of being static analisys friendly. So im against of putting more logic in magic get data

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 110e3d0. ± Comparison against base commit 3486d5a.

[fballiano]

PHPStan is telling us that the modification to app/code/core/Mage/Wishlist/controllers/IndexController.php has some problem:

Call to an undefined method Mage_Wishlist_IndexController::_getData().

we've to fix it before release

[constancyGmbH]

Hi @fballiano , after applying this release, all of my prices drop to zero in cart and Checkout. In Catalog and Product view everything is fine.
Any Idea why?

[constancyGmbH]

I broke it down to the File /app/code/core/Mage/Sales/Model/Quote/Item/Abstract.php and here the function

/**
     * @return $this
     */
    public function setCalculationPrice($value)
    {
        return $this->setData('calculation_price', (float) $value);
    }

If I comment this function out, it works and Prices are there. I'm not sure why.

[fballiano]

Thank you very much @constancyGmbH, I'll try to take a look at this ASAP

[constancyGmbH]

I think it has something to do with setting this value to NULL.
with this workaround it is working, but I'm not sure if this will work in PHP 8:

/**
     * @return $this
     */
    public function setCalculationPrice($value)
    {

        if(is_null($value)){
            return $this->setData('calculation_price', $value);
        }else{
            return $this->setData('calculation_price', (float) $value);
        }
    }

[fballiano]

mmmm it doesn't happen to me on a vanilla openmage installation :-\

[constancyGmbH]

ok, so maybe this is a Probleme with a custom module or an adjustment on our Magento installation....

[constancyGmbH]

Have you tried your Code in PHP 7 oder 8 (or Both)?

[fballiano]

ok, so maybe this is a Probleme with a custom module or an adjustment on our Magento installation....

that's what i was thinking, I tried it on php7.4

[kiatng]

I haven't tested the finding of @constancyGmbH, but I highly suspect it is related to

magento-lts/app/code/core/Mage/Sales/Model/Quote/Item/Abstract.php

Lines 609 to 613 in dfb6fe8

	public function setConvertedPrice($value)
	{
	$this->setCalculationPrice(null);
	$this->setData('converted_price', $value);
	return $this;

.

This PR added a bunch of setters:

magento-lts/app/code/core/Mage/Sales/Model/Quote/Item/Abstract.php

Lines 1114 to 1477 in dfb6fe8

	public function setBaseOriginalDiscountAmount($value)
	{
	return $this->setData('base_original_discount_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseCalculationPrice($value)
	{
	return $this->setData('base_calculation_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseCustomPrice($value)
	{
	return $this->setData('base_custom_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseDiscountAmount($value)
	{
	return $this->setData('base_discount_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseDiscountCalculationPrice($value)
	{
	return $this->setData('base_discount_calculation_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseExtraRowTaxableAmount($value)
	{
	return $this->setData('base_extra_row_taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseExtraTaxableAmount($value)
	{
	return $this->setData('base_extra_taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseHiddenTaxAmount($value)
	{
	return $this->setData('base_hidden_tax_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseOriginalPrice($value)
	{
	return $this->setData('base_original_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBasePriceInclTax($value)
	{
	return $this->setData('base_price_incl_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseRowTotal($value)
	{
	return $this->setData('base_row_total', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseRowTotalInclTax($value)
	{
	return $this->setData('base_row_total_incl_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseRowTotalWithDiscount($value)
	{
	return $this->setData('base_row_total_with_discount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseTaxableAmount($value)
	{
	return $this->setData('base_taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseTaxAmount($value)
	{
	return $this->setData('base_tax_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseTaxBeforeDiscount($value)
	{
	return $this->setData('base_tax_before_discount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseTaxCalcPrice($value)
	{
	return $this->setData('base_tax_calc_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseTaxCalcRowTotal($value)
	{
	return $this->setData('base_tax_calc_row_total', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBasePrice($value)
	{
	return $this->setData('base_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseRowTax($value)
	{
	return $this->setData('base_row_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseShippingAmount($value)
	{
	return $this->setData('base_shipping_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseWeeeTaxAppliedAmount($value)
	{
	return $this->setData('base_weee_tax_applied_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setBaseWeeeTaxAppliedRowAmount($value)
	{
	return $this->setData('base_weee_tax_applied_row_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setCalculationPrice($value)
	{
	return $this->setData('calculation_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setDiscountAmount($value)
	{
	return $this->setData('discount_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setDiscountCalculationPrice($value)
	{
	return $this->setData('discount_calculation_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setDiscountPercent($value)
	{
	return $this->setData('discount_percent', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setDiscountTaxCompensation($value)
	{
	return $this->setData('discount_tax_compensation', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setExtraRowTaxableAmount($value)
	{
	return $this->setData('extra_row_taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setExtraTaxableAmount($value)
	{
	return $this->setData('extra_taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setHiddenTaxAmount($value)
	{
	return $this->setData('hidden_tax_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setOriginalDiscountAmount($value)
	{
	return $this->setData('original_discount_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setPriceInclTax($value)
	{
	return $this->setData('price_incl_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setQty($value)
	{
	return $this->setData('qty', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setRowTotal($value)
	{
	return $this->setData('row_total', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setRowTotalExcTax($value)
	{
	return $this->setData('row_total_exc_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setRowTotalInclTax($value)
	{
	return $this->setData('row_total_incl_tax', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setRowTotalWithDiscount($value)
	{
	return $this->setData('row_total_with_discount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxableAmount($value)
	{
	return $this->setData('taxable_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxCalcPrice($value)
	{
	return $this->setData('tax_calc_price', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxCalcRowTotal($value)
	{
	return $this->setData('tax_calc_row_total', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxAmount($value)
	{
	return $this->setData('tax_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxBeforeDiscount($value)
	{
	return $this->setData('tax_before_discount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setTaxPercent($value)
	{
	return $this->setData('tax_percent', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setWeeeTaxAppliedAmount($value)
	{
	return $this->setData('weee_tax_applied_amount', (float) $value);
	}
	/**
	* @return $this
	*/
	public function setWeeeTaxAppliedRowAmount($value)
	{
	return $this->setData('weee_tax_applied_row_amount', (float) $value);
	}

Just eyeballing without testing, we know that setField(null) is another way to unset the field unsField(), which is not the same as setField(0), which is what the bunch of methods do above.

I do not know how best to address the issue, but may be something like:

// Field is generic field name.
function setField($value)
{
   $this->setData('field', $value === null ? null : (float) $value);
   return $this;
}

[edit] I'm not sure if the above is the correct fix. Does PHP 8 happy with null * 1?
[edit] I tested 1/null in PHP8.0, it throws PHP Fatal error: Uncaught DivisionByZeroError: Division by zero, which is exactly what is required (PHP converts null to zero in maths). So, it seems the setter above is an OK fix.

[kiatng]

I think this is the reason the price is zero in the cart:

magento-lts/app/code/core/Mage/Sales/Model/Quote/Item/Abstract.php

Lines 434 to 446 in dfb6fe8

	public function getCalculationPriceOriginal()
	{
	$price = $this->_getData('calculation_price');
	if (is_null($price)) {
	if ($this->hasOriginalCustomPrice()) {
	$price = $this->getOriginalCustomPrice();
	} else {
	$price = $this->getConvertedPrice();
	}
	$this->setData('calculation_price', $price);
	}
	return $price;
	}

With this PR, $price above is always zero when the setter value is null. So, line 437 if (is_null($price)) { is false, another fix is change it to if (empty($price)) {.

[edit] If we use if (empty($price)) {, I'm not sure if it will work for promotion rule that set the price to zero, or any side effect on product with price zero. I think it is safer to fix it in the setters. I'm totally unsure how best to fix this.

[fballiano]

yes, right, it's not only the final price but also the unit price

and who know how many more things, I think we've to revert this patch completely or I don't know.

[kiatng]

+1 to revert this PR.

[edited]
PHP 8: The fatal error is only on empty string but not numeric string

echo '1' + 1; // OK, result int(2)
echo null + 1; //OK, result int(1)
echo '1.1' + 1; //OK, result float(2.1)
echo round('1.1'); //OK, result float(1)
echo '' + 1; // Not OK, PHP Fatal error:  Uncaught TypeError: Unsupported operand types: string + int 
echo round(''); // Not OK, PHP Fatal error:  Uncaught TypeError: round(): Argument #1 ($num) must be of type int|float, string given

When retrieving type float|int from DB, getter returns null or numeric string but never empty string when tested in OM, so far so good.

But what if we set empty string to float|int field? I tested it in OM, setField(''), will save in DB as int(0) or float(0). So, saving empty string is OK, it's converted to the right type. It means saving a form with float|int input types are OK. That 's good. However, if we do a getField() later on, it returns empty string, not good. Therefore, for float|int type, to avoid empty string in arithmetic, we can do

setField((int) $var); // OK for arithmetic, but not OK when testing for null.
setField((float) $var); // OK for arithmetic, but not OK when testing for null.
setField($var ?: null); // OK for arithmetic and testing for null, but not OK when testing === 0

Without knowing the intent of the original developer and how the field is being used, it is hard to know which of the above can be applied.

But for float|int field, why would $var be an empty string in the first place? May be it was inherited from a form or params from AJAX. Irregardless, this seems like a mistake to me and should be corrected at the source. The correction can be data validation prior to setter, or being lazy, reload the model if it has been saved. So, throwing a fatal error is a good thing.

I am not 100% sure, but prior to this PR, OM seems to work just fine with PHP 8. My conclusion is that this PR is unnecessary.

[fballiano]

I've created a PR to revert this one, altough I can't merge it without 2 other maintainers to approve