magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.39k stars 9.29k forks source link

Wrong Price Calculation with HHVM #966

Closed gewaechshaus closed 8 years ago

gewaechshaus commented 9 years ago

There seems to be a bug with the price calculation MCE 1.9.1.0 when using HHVM 3.4.2 in fcgi mode, even with enabled

hhvm.enable_zend_sorting=1

See the related issue https://github.com/facebook/hhvm/issues/4630

_Tracked down -> Mage_Core_Helper_Data _

/**
 * Convert and format price value for specified store
 *
 * @param   float $value
 * @param   int|Mage_Core_Model_Store $store
 * @param   bool $format
 * @param   bool $includeContainer
 * @return  mixed
 */
public static function currencyByStore($value, $store = null, $format = true, $includeContainer = true)
{
    try {
        if (!($store instanceof Mage_Core_Model_Store)) {
            $store = Mage::app()->getStore($store);
        }
        echo $value.'<br />';
        $value = $store->convertPrice($value, $format, $includeContainer);
        die($value);
    }
    catch (Exception $e){
        $value = $e->getMessage();
    }

    return $value;
}

Output

1.4800
14.800.000,00 €

Prices are high atm :)

Will track it down deeper and inspect the convertPrice method.

gewaechshaus commented 9 years ago

Deeper trackdown -> Mage_Core_Model_Store

/**
 * Format price with currency filter (taking rate into consideration)
 *
 * @param   double $price
 * @param   bool $includeContainer
 * @return  string
 */
public function formatPrice($price, $includeContainer = true)
{
    if ($this->getCurrentCurrency()) {

          Zend_Debug::dump(Mage::app()->getLocale()); 
          Zend_Debug::dump($price); 
          Zend_Debug::dump($this->getCurrentCurrency()->format($price, array(), $includeContainer));

          die('somewhere over the rainbow');

          return $this->getCurrentCurrency()->format($price, array(), $includeContainer);
    }
    return $price;
}

Output

object(Mage_Core_Model_Locale)#77772 (4) {
 ["_defaultLocale":protected] => string(5) "de_DE"
 ["_locale":protected] => object(Zend_Locale)#79137 (1) {
   ["_locale":protected] => string(5) "de_DE"
 }
 ["_localeCode":protected] => string(5) "de_DE"
 ["_emulatedLocales":protected] => array(0) {
 }

}

float(1,48)
string(45) "<span class="price">14.800.000,00 €</span>"
gewaechshaus commented 9 years ago

It appears system-wide, so it has to be deeper in the core... Call it the 10mio € bug.

Native number_format works as expected.

Zend_Debug::dump(number_format($price,2));

string(4) "1.48"
gewaechshaus commented 9 years ago

Damn, this looks like an issue with sprintf in https://github.com/facebook/hhvm/issues/3725

Mage_Directory_Model_Currency

  /**
 * Returns the formatted price
 *
 * @param float $price
 * @param null|array $options
 * @return string
 */
public function formatTxt($price, $options = array())
{
    if (!is_numeric($price)) {
        $price = Mage::app()->getLocale()->getNumber($price);
    }
    /**
     * Fix problem with 12 000 000, 1 200 000
     *
     * %f - the argument is treated as a float, and presented as a floating-point number (locale aware).
     * %F - the argument is treated as a float, and presented as a floating-point number (non-locale aware).
     */

     Zend_Debug::dump($price);
     $price = sprintf("%F", $price);
     Zend_Debug::dump('After sprintf- '.$price);

    if ($price == -0) {
        $price = 0;
    }
    return Mage::app()->getLocale()->currency($this->getCode())->toCurrency($price, $options);
}

Output float(1,48) string(23) "After sprintf- 1.480000"

gewaechshaus commented 9 years ago
Zend_Debug::dump(Mage::app()->getLocale()->currency());

object(Zend_Currency)#89761 (1) {
 ["_options":protected] => array(12) {
 ["position"] => int(8)
 ["script"] => NULL
 ["format"] => NULL
 ["display"] => int(2)
 ["precision"] => int(2)
 ["name"] => string(4) "Euro"
 ["currency"] => string(3) "EUR"
 ["symbol"] => string(3) "€"
 ["locale"] => string(5) "de_DE"
 ["value"] => int(0)
 ["service"] => NULL
 ["tag"] => string(11) "Zend_Locale"
  }
}

Options array The following options are available

Will continue tomorow! Any related ideas, hints etc. would be nice...

orlangur commented 9 years ago

Is this bug reproducible in Magento 2? It seems to me that relevant place for Magento 1.x bug reports is http://www.magentocommerce.com/bug-tracking.

As for Magento 2, it is not fully compatible with HHVM currently, but should definitely become compatible some day.

gewaechshaus commented 9 years ago

Hi Vlad,

you are right. I posted it here cause i'm unsure if this bug is related to ZF and i know about the future plans with Magento 2 and HHVM. I also think that Alan said that there are no plans to move away from ZF. Another reason, i like github more than the official bug tracker at magentocommerce.com as it seems faster and more productive ;) Found that would be a better place than the hhvm repo.

But to keep things in order, i will also post it in the official bugtracker when things are more clear and reproducable to us. I would like to continue inspecting this and post my results here if there are no problems with that.

Regards, Jan

verklov commented 9 years ago

We created the ticket int he internal tracker with ID: MAGETWO-32945. the team will work on it according to the priorities. Thank you!

gewaechshaus commented 9 years ago

Hey Sergey,

thanks. I'll keep you updated. Actually we tracked it down so deep, i has to be related to ZF and the some of the currency formating methods. Another thing in the background, it could be related to hhvm's parsing of php money_format. One thing we could clearly track: If this bug appends, the format of the each currency gets broken, indepent of which currency you choose. For example, if you choose EUR and Zend::dump a helper formated zero price in €, it will become 00,00€ instead of it's default 0,00€, the same shizzle happens with $ and so on. With this fact in the background, i guess sprintf or some other methods will fail and multiply the result with 10mio. We checked some other stuff, the localization formats xml from the ZF locale are included correctly. So what the f*\ is happening here?

It happens in the back- and in frontend (you can clearly see this in DEV systems dashboard as the stats are mostly at zero or switch the currency programmaticaly in the FE)... All orders are multiplied with 10mio while sales_flat_order is still ok. If you dump out the var, without any formating, it's ok 2. So it's in fact related to localisation/currency converting.

Kind regards, Jan

We created the ticket int he internal tracker with ID: MAGETWO-32945. the team will work on it according to the priorities. Thank you!

— Reply to this email directly or view it on GitHub https://github.com/magento/magento2/issues/966#issuecomment-71048054.

verklov commented 9 years ago

Thanks a lot for your input, Jan! Unfortunately, I cannot promise you issue handling - you know that we do not officially support hhvm. However, the ticket is in the product backlog and will be handled according to our priorities. Have a good day! :-)

AGalico commented 9 years ago

Hi Jan, have you found any conditions that trigger this behavior? we've had similar issues on a similar stack, (nginx(1.6.2)->hhvm(3.5.0)->magento(1.9.1)) but i've not been able to replicate it or find any evidence of it occuring other than a screenshot that was sent to me.

gewaechshaus commented 9 years ago

Hi @AGalico, not really! We're only able to reproduce this after a bigger bunch of transactions and during traffic peaks... So all you can do actually is bomb the system with orders and traffic. We were able to bring it up with a 5 man team.

Can u post the screen?

AGalico commented 9 years ago

price issue

not the most descriptive image, the rrp is coming from the magento database, the customer price is generated by our ERP but goes through the same methods for formatting.

gewaechshaus commented 9 years ago

Hey AGalico,

ok, that looks and sounds exactly like in our case and my test cases...

Could you bring it up again? Maybe we should concentrate our ressources and try to bring it up in one of our or your dev systems.

AGalico commented 9 years ago

i've tried simulating a load on the dev environment and haven't been able to get it to occur, if i get some time in the near future i might switch live back to hhvm with a fallback to php-fpm, and build something into view.phtml to trigger it to silently fallback when this issue arises, in turn triggering something to image hhvm and magento cache.

gewaechshaus commented 9 years ago

Short version: the bug isn't only affecting the view.phtml, neither one template. You'll have to fix it globally in the related method(s). Otherwise transactions will fail[f.e. the paypal ipn procedure], wrong values get stored[f.e. in sales_flat_order tbl] and so on.

jjone commented 9 years ago

hhvm3 6bug

Shopping Cart Price Rules failed to apply in the grand total.

server: HHVM 3.6, Magento 1.9.1 and nginx 1.6.2

AGalico commented 9 years ago

@jjone i believe this is a different issue, and may be rectifiable by adding the line: hhvm.enable_zend_sorting= 1 to hhvm's server.ini

gewaechshaus commented 9 years ago

@AGalico that is absolutely correct!

Daniel Sloof figured this out a while ago, everybody who wants to dig deep, take a look at this write-up http://de.slideshare.net/meetmagento/daniel-sloof-magento-on-hhvm https://github.com/facebook/hhvm/issues/3202

jjone commented 9 years ago

fixed. Thank you very much AGalico and gewaechshaus !

gewaechshaus commented 9 years ago

Another Idea @verklov @orlangur @AGalico

I didn't check it, but maybe we're just running into a case-insensitive class naming issue http://docs.hhvm.com/manual/en/hack.unsupported.php

Case-insensitive function calls (the Hack type checker is case-sensitive). 

or something similar, simple stupid, naturaly not in horizon cause functionable in 99% of all setups issue. Maybe one of you can check the list 2...

AGalico commented 9 years ago

http://docs.hhvm.com/manual/en/hack.unsupported.php

Note: These are features that the Hack language does not support (i.e., the Hack type checker will give you an error if you try to use them). However, HHVM supports these features just fine when using PHP code.

gewaechshaus commented 9 years ago

Damn, i didn't read this good enough @AGalico .

But as the fate want, we switched to HHVM 3.6.1 today and gave it a short try. Finaly we got this ahhhhhhhhhhhhhhhhhhhh

39,80€

and something like waaaaaaaaaaaaaaaaahhhhhhhhhhhh

1,98€

My hot candidates causing this numfmt_format numfmt_format_currency numfmt_parse_currency Zend Currency

http://docs.hhvm.com/manual/en/numberformatter.formatcurrency.php http://php.net/manual/de/numberformatter.formatcurrency.php http://framework.zend.com/manual/1.12/en/zend.currency.usage.html

AGalico commented 9 years ago

are you able to see what happens in Zend_Currency toCurrency()?

gewaechshaus commented 9 years ago

I stopped hhvm to let the fallback be active[php5-fpm], we just moved our dev to live... Have to clone it into a new dev, will start tomorow. Still working 20hrs.

  sleep

Quick note, test this snip in index.php

 $locale = new Zend_Locale('de_DE');
 Zend_Registry::set('Zend_Locale', $locale);
Krapulat commented 8 years ago

I confirm the bug.

With HHVM active it displays wrong prices.

When using PHP-FPM it works fine.

vzabaznov commented 8 years ago

Hi @gewaechshaus seems like it was fixed in hhvm version 3.9, please check it, and if you think that not, feel free to write here or to open new one