Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HHVM 3.6.0 ini_get not working #4993

Closed
sandeepone opened this issue Mar 10, 2015 · 106 comments

Comments

@sandeepone
Copy link

sandeepone commented Mar 10, 2015

I'm unable to get ini_get('post_max_size') return empty string, which worked in 3.5.0.

echo ini_get('post_max_size');  // output ""
echo ini_get('max_post_size');  // output ""

echo ini_get('hhvm.server.max_post_size');  //output 104857600

Note: i'm not sure which one to use in HHVM so tried both, in Zend php it is 'post_max_size'
Thank you

@jeroenvermeulen

This comment has been minimized.

Copy link

jeroenvermeulen commented Mar 11, 2015

This bug is probably what breaks image management in te backend of Magento on HHVM 3.6.0.
This is a serious problem.

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 11, 2015

even with ini_set('post_max_size', '25000');
ini_get('post_max_size') returns empty string

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 11, 2015

the problem occurs with upload_max_size too

@sandeepone

This comment has been minimized.

Copy link
Author

sandeepone commented Mar 11, 2015

I don't think we can set the post_max_size with ini_set, IMO it can only be set from ini file. Yes same issue with upload_max_size. A qucik ugly fix is use this ini_get('hhvm.server.max_post_size')

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 11, 2015

ini_get('hhvm.server.max_post_size') returns empty string too

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 11, 2015

well, actually it returns a boolean value (false)

@fredemmott

This comment has been minimized.

Copy link
Contributor

fredemmott commented Mar 11, 2015

@rebeld

This comment has been minimized.

Copy link

rebeld commented Mar 11, 2015

FWIW, I'm only seeing this behaviour when vending through a webserver (nginx via fastcgi in my case). If I call ini_get through hhvn via the command line, it returns the values I would expect for 'upload_max_filesize' and 'post_max_size'.

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 11, 2015

exactly.

@BradStephens

This comment has been minimized.

Copy link

BradStephens commented Mar 13, 2015

For post_max_size, use ini_get('hhvm.server.max_post_size').
For upload_max_filesize, use ini_get('hhvm.server.upload.upload_max_file_size').

Here's code for Magento users. app/code/core/Mage/Adminhtml/Block/Media/Uploader.php

public function getPostMaxSize()
{
    $post_max_size = ini_get('post_max_size');
    return $post_max_size ? $post_max_size : ini_get('hhvm.server.max_post_size');
}

public function getUploadMaxSize()
{
    $upload_max_filesize = ini_get('upload_max_filesize');
    return $upload_max_filesize ? $upload_max_filesize : ini_get('hhvm.server.upload.upload_max_file_size');
}
@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Mar 13, 2015

Does not work for me.

P.S.: sorry, it works.

@sandeepone

This comment has been minimized.

Copy link
Author

sandeepone commented Mar 13, 2015

The code should work as i'm using slightly different variant is working for me. Since I custom built the HHVM 3.6.0 on Centos 7 its working, I don't why it's not working for others.

@rebeld

This comment has been minimized.

Copy link

rebeld commented Mar 13, 2015

Sandeep, interesting, you are saying that hhvm 3.6 built from source is not exhibiting this behaviour for you? I'll have to give that a try.

@sandeepone

This comment has been minimized.

Copy link
Author

sandeepone commented Mar 13, 2015

I mean some people saying ini_get('hhvm.server.max_post_size') this not working

@rebeld

This comment has been minimized.

Copy link

rebeld commented Mar 13, 2015

Ah, got it, thanks for the clarification.

@BradStephens

This comment has been minimized.

Copy link

BradStephens commented Mar 13, 2015

They're probably trying post_max_size instead of max_post_size. The hhvm team reversed it from what PHP calls it.

@jeroenvermeulen

This comment has been minimized.

Copy link

jeroenvermeulen commented Mar 13, 2015

Will ini_get('post_max_size') and ini_get('max_post_size') work in the next version of HHVM again?
A lot of existing upload scripts break under HHVM 3.6.0.

@JoelMarcey JoelMarcey added the ini label Mar 13, 2015
@JoelMarcey JoelMarcey self-assigned this Mar 13, 2015
@sandeepone

This comment has been minimized.

Copy link
Author

sandeepone commented Mar 13, 2015

Hopefully a minor version will be released with the fixes

@craigcarnell

This comment has been minimized.

Copy link
Contributor

craigcarnell commented Mar 16, 2015

Any updates on identifying the code change that broke this?

@danslo

This comment has been minimized.

Copy link
Contributor

danslo commented Mar 16, 2015

Looking into this.

@danslo

This comment has been minimized.

Copy link
Contributor

danslo commented Mar 17, 2015

After a ~5 hour long bisect, 4bfeda9 is the culprit.

/cc @rrh

@rrh

This comment has been minimized.

Copy link
Contributor

rrh commented Mar 17, 2015

Sorry for the issues here. I'll look into them on Mar 17.

@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Mar 17, 2015

My initial hunch is that it is having to do with settings that are set as PHP_INI_PERDIR...

@craigcarnell

This comment has been minimized.

Copy link
Contributor

craigcarnell commented Mar 17, 2015

Any luck @rrh ?

@rrh

This comment has been minimized.

Copy link
Contributor

rrh commented Mar 17, 2015

I've just started looking at it now. Gotta wait for a debug build to finish. Then will wonder why there aren't any tests for this. Then will write some tests and look at the data flow through the PHP_INI_PERDIR

@danslo

This comment has been minimized.

Copy link
Contributor

danslo commented Mar 17, 2015

@rrh it's probably covered by a test, but it seems to happen in server / fastcgi mode and not on the command line. @JoelMarcey confirms that it's not specific to FastCGI though, so my initial hunch was threading, but PHP_INI_PERDIR sounds more likely.

In any case, hphp/test/server/fastcgi/tests will probably be a good place to place your new tests in.

@Larceniii

This comment has been minimized.

Copy link

Larceniii commented Jun 11, 2015

@JoelMarcey Thank you for understanding our position on this and taking away something positive from it. To me, that's a big win for hhvm.

@oliver-graetz I was in the same boat on the 3.5 > 3.3 release and it wasn't nearly as stable as 3.5 in my custom wordpress environment.

I tried building hhvm 3.5 from source but the builds were doing very strange things like growing in size to 100s of megs on disk. I probably made some noobie mistake compiling but I spent so many hours on it only to finally give up and go back to 3.3 + fpm

I mean all this is possible because hhvm exists and I'm pushing the boundaries like I never dreamed possible because of the advancements in php due to hhvm. It's really an amazing piece of work, and I can't wait to get back on board.

@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Jun 11, 2015

@Larceniii Thank you. I am really sorry about all this heartache.

All -- let me give you status of our current thinking. I want to keep everyone in the loop.

We are looking to hotfix the diff that enabled this issue to begin with. I am looking into that as we speak. I have an idea that could work.

4bfeda9

We don't want to revert the diff because people are depending on the functionality. But we can possibly work around the diff to provide an option not to use its functionality so that we can have a fix for the 3.6 LTS (for the upcoming 3.6.2).

Then we can let https://reviews.facebook.net/D38907 bake in master for a while to make sure it is stable. If it is, then we can remove the hotfix and have these two diffs live harmoniously in an upcoming point release.

cc: @jwatzman

JoelMarcey added a commit that referenced this issue Jun 12, 2015
Summary: D1797805 enabled us to have better compatibility between ini and our
zend compatibility layer, but it broke certain per request type
of settings like `upload_max_filesize`. You could `ini_get()` these
values running a CLI script, but server requests gave you a `bool(false)`
signifying that the settings were not being bound.

This is a hotfix for our LTS releases so that we can keep the current
functionality but unbreak people as well. Basically provide a
rutnime option to disable the zend ini compatibility.

Longer term, something around D2099778 will be the anwser to the
problem. But we need to let that review and bake in `master` for
a while.

Ref. #4993

Reviewed By: @jwatzman

Differential Revision: D2148517
JoelMarcey added a commit that referenced this issue Jun 12, 2015
Summary: D1797805 enabled us to have better compatibility between ini and our
zend compatibility layer, but it broke certain per request type
of settings like `upload_max_filesize`. You could `ini_get()` these
values running a CLI script, but server requests gave you a `bool(false)`
signifying that the settings were not being bound.

This is a hotfix for our LTS releases so that we can keep the current
functionality but unbreak people as well. Basically provide a
rutnime option to disable the zend ini compatibility.

Longer term, something around D2099778 will be the anwser to the
problem. But we need to let that review and bake in `master` for
a while.

Ref. #4993

Reviewed By: @jwatzman

Differential Revision: D2148517
JoelMarcey added a commit that referenced this issue Jun 12, 2015
Summary: D1797805 enabled us to have better compatibility between ini and our
zend compatibility layer, but it broke certain per request type
of settings like `upload_max_filesize`. You could `ini_get()` these
values running a CLI script, but server requests gave you a `bool(false)`
signifying that the settings were not being bound.

This is a hotfix for our LTS releases so that we can keep the current
functionality but unbreak people as well. Basically provide a
rutnime option to disable the zend ini compatibility.

Longer term, something around D2099778 will be the anwser to the
problem. But we need to let that review and bake in `master` for
a while.

Ref. #4993

Reviewed By: @jwatzman

Differential Revision: D2148517
@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Jun 12, 2015

A diff just landed into master and will be backported into the 3.6 LTS and 3.7 branches by @jwatzman to //hopefully// help alleviate this issue.

Basically, until a proper fix is accepted and known to work (which is probably https://reviews.facebook.net/D38907 or some variation), you can specify a new ini option in your server.ini (or equivalent config file) to disable the functionality that the offending diff that caused this issue provides.

hhvm.enable_zend_ini_compat=false
@jwatzman

This comment has been minimized.

Copy link
Member

jwatzman commented Jun 12, 2015

(And the reason false isn't the default is so we don't break anyone relying on this behavior, particularly in an LTS. 3.9 should have a proper fix, but notably the upcoming 3.8 will not.)

@craigcarnell

This comment has been minimized.

Copy link
Contributor

craigcarnell commented Jun 15, 2015

@jwatzman Updated to the nightly and tested on my end, seems to read through fine! 👍

With hvm.enable_zend_ini_compat=false

What 3.7 version will it be included in? 3.7.3?

@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Jun 15, 2015

@craigcarnell Great to hear. It should be in 3.6.4 and 3.7.2 (I don't think there is a tag for that yet, but the 3.7 branch has been updated).

Again, everyone, sorry it took so long to get a fix out here. What I thought was the right thing, turned out to be too time consuming and, lesson learned, get a hotfix out as soon as possible.

@Kazanir

This comment has been minimized.

Copy link

Kazanir commented Jun 15, 2015

Thanks guys; various OSS frameworks thank you. :)

@sandeepone

This comment has been minimized.

Copy link
Author

sandeepone commented Jun 15, 2015

Thanks guys

@oliver-graetz

This comment has been minimized.

Copy link

oliver-graetz commented Jun 15, 2015

Thanks!

@MarkGavalda

This comment has been minimized.

Copy link

MarkGavalda commented Jun 15, 2015

Woot! Thanks guys.

@jwatzman

This comment has been minimized.

Copy link
Member

jwatzman commented Jun 15, 2015

3.6.4 is out with the hhvm.enable_zend_ini_compat=false option, and I'm running final tests on 3.7.2 which will also include it. I expect to begin building 3.7.2 in a couple hours, but when it lands for you depends on distro :)

@jwatzman

This comment has been minimized.

Copy link
Member

jwatzman commented Jun 15, 2015

For those on 3.7, the 3.7.2 release is ready but the host for dl.hhvm.com has been having some issues today, will get them up as soon as they are resolved.

@jeroenvermeulen

This comment has been minimized.

Copy link

jeroenvermeulen commented Jun 16, 2015

Great work guys!

@chromafunk

This comment has been minimized.

Copy link

chromafunk commented Jun 18, 2015

I am on 3.7.2 and the magento backend still suffers from the same. The buttons won't show up.

@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Jun 18, 2015

@chromafunk I am a bit unclear. Are you running a magento server? Did you put this option in your server.ini file

hhvm.enable_zend_ini_compat=false
@pepijnblom

This comment has been minimized.

Copy link

pepijnblom commented Jun 19, 2015

Thank you so much, this has solved the problem for us (Magento CE 1.9.1.1 on HHVM 3.7.2 on Ubuntu 14.04.2 LTS) by adding the line to server.ini

sathvikl added a commit to sathvikl/hiphop-php that referenced this issue Jun 20, 2015
Summary: D1797805 enabled us to have better compatibility between ini and our
zend compatibility layer, but it broke certain per request type
of settings like `upload_max_filesize`. You could `ini_get()` these
values running a CLI script, but server requests gave you a `bool(false)`
signifying that the settings were not being bound.

This is a hotfix for our LTS releases so that we can keep the current
functionality but unbreak people as well. Basically provide a
rutnime option to disable the zend ini compatibility.

Longer term, something around D2099778 will be the anwser to the
problem. But we need to let that review and bake in `master` for
a while.

Ref. facebook#4993

Reviewed By: @jwatzman

Differential Revision: D2148517
@grimabe

This comment has been minimized.

Copy link

grimabe commented Oct 16, 2015

I'm under Magento 1.9.1.0 with HHVM 3.8.1 FastCGI and the buttons do not appear even with the

hhvm.enable_zend_ini_compat=false

added to /etc/hhvm/php.ini and /etc/hhvm/server.ini files

any idea @JoelMarcey ?

Thanks

@grimabe

This comment has been minimized.

Copy link

grimabe commented Oct 16, 2015

My bad, it was a flash issue. Indeed on safari it didn't worked because flash plugin was missing. But in chrome it did.

@fcorriga

This comment has been minimized.

Copy link

fcorriga commented Oct 16, 2015

i suggest to upgrade to 3.9.1 LTS if you're using it in a production server

atdt added a commit to wikimedia/mediawiki that referenced this issue Nov 8, 2015
…_max_size'

In HHVM, the settings 'upload_max_filesize' and 'post_max_size' are
not available via ini_get() due to some long-standing bug
(facebook/hhvm#4993). Instead, one can use
'hhvm.server.upload.upload_max_file_size' and 'hhvm.server.max_post_size'
(in a typical PHP fashion, their names are subtly different than the
originals as to increase the potential for confusion).

Added a new method UploadBase::getMaxPhpUploadSize() to handle this.

Additionally:
* 'post_max_size' can be set to 0, which is equivalent to no limit.
  Handle this correctly.
* $wgMaxUploadSize can be an array structure, instead of just a number.
  Handle this correctly by using UploadBase::getMaxUploadSize().
* When no maximum is set, use PHP_INT_MAX rather than 1e100. It should
  be big enough, and the latter is a float, results in 0 when cast to
  int, and doesn't look as pretty when formatted in GB in the interface.

Bug: T116347
Change-Id: Idf707253eeae1b90792a7e26d2ab66d1317e67ae
@ksaltik

This comment has been minimized.

Copy link

ksaltik commented Nov 26, 2015

Merging js cause this error not hhvm. Just merge js in frontend. We solve backend product image upload issue this way

@JoelMarcey

This comment has been minimized.

Copy link
Contributor

JoelMarcey commented Dec 12, 2015

I think this can be closed now, right? If not, please re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.