HHVM and PHP handle non-static input to call_user_func_array() different
Open, Needs TriagePublic

Description

The hhvm test passed, while the php test gives:

1) EchoDiscussionParserTest::testExtractHeader with data set #0 ('', false)
call_user_func_array() expects parameter 1 to be a valid callback, non-static method LoginNotifyHooks::onUserSaveOptions() should not be called statically

/srv/jenkins-workspace/workspace/mwext-testextension-php55/src/includes/Hooks.php:186

Seen on https://gerrit.wikimedia.org/r/#/c/335994/
Fix is easy https://gerrit.wikimedia.org/r/#/c/336072/

I am wonder, why hhvm and php handle this different. Maybe a upstream bug

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 5 2017, 11:02 AM
Florian added a subscriber: Florian.Feb 5 2017, 9:08 PM

Filed upstream as: https://github.com/facebook/hhvm/issues/7647

PHP does this correctly, see:
https://3v4l.org/irrel

(maybe I miss something and this is the correct behaviour :/)

hashar moved this task from Backlog to Reported Upstream on the Upstream board.

The upstream bug has some information, that this was already fixed in 3.17. The test is using 3.18.2.

It needs error_reporting(E_ALL | E_STRICT) and there is something about hhvm.raise_missing_this or hhvm.force_hh

Yeah, I totally missed that, sorry :(

So, I tested this a little bit locally, and with HHVM 3.19.2 and 3.20.0 the strict standards error appears without explicitly setting any configuration option. It disappears, if I manually set hhvm.raise_missing_this to 0. So, for now I'm unsure what to do, as I can not see, that this settings is explicitly set to 0 in puppet (which should manage the hhvm installation on integration servers). So, I would now propose a change for https://github.com/wikimedia/puppet/blob/fa7b47d3115a470fd14a68ede5b76df353180649/modules/contint/manifests/hhvm.pp to set the option explicitly to 1, however, I would love if someone (maybe @hashar?) to check the existing configuration and probably verify it?

Change 399651 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Move avro-php back to Nodepool HHVM

https://gerrit.wikimedia.org/r/399651

Change 399651 merged by Hashar:
[integration/config@master] Move avro-php back to Nodepool HHVM

https://gerrit.wikimedia.org/r/399651

hashar added a comment.EditedDec 21 2017, 3:59 PM

I had the issue with avro-php. The tests worked fine on the Nodepool instances but failed on the Docker one. They supposedly use the same HHVM version but the ini file is slightly different.

In the Docker container based on Stretch docker-registry.wikimedia.org/releng/composer-package-hhvm:0.1.3 P6495

In Nodepool (Jessie) P6496

The diff:

$ colordiff -u nodepool docker
--- nodepool	2017-12-21 16:54:24.482848878 +0100
+++ docker	2017-12-21 16:54:04.419091345 +0100
@@ -1,31 +1,17 @@
 date.timezone = UTC
-hhvm.debug.core_dump_report_directory = /var/log/hhvm
 hhvm.debug.core_dump_report = false
-hhvm.dynamic_extension_path = /usr/lib/x86_64-linux-gnu/hhvm/extensions/20150212
-hhvm.dynamic_extensions[luasandbox.so] = luasandbox.so
-hhvm.dynamic_extensions[tidy.so] = tidy.so
-hhvm.dynamic_extensions[wikidiff2.so] = wikidiff2.so
 hhvm.enable_obj_destruct_call = true
 hhvm.enable_zend_compat = true
-hhvm.hack.lang.iconv_ignore_correct = true
 hhvm.jit = false
-hhvm.jit_pseudomain = false
-hhvm.log.header = true
-hhvm.log.level = Error
-hhvm.log.native_stack_trace = true
+hhvm.log.always_log_unhandled_exceptions = true
+hhvm.log.level = Warning
+hhvm.log.runtime_error_reporting_level = 8191
 hhvm.log.use_syslog = false
-hhvm.mysql.slow_query_threshold = 10000
+hhvm.mysql.typed_results = false
 hhvm.mysql.typed_results = false
 hhvm.perf_pid_map = false
-hhvm.pid_file = 
-hhvm.repo.central.path = 
+hhvm.repo.allow_fallback_path = false
+hhvm.repo.central.path = /hhvm.hhbc
 hhvm.repo.eval.mode = central
 hhvm.repo.journal = memory
 hhvm.repo.local.mode = --
-hhvm.resource_limit.core_file_size = 8589934592
-hhvm.server.apc.expire_on_sets = true
-hhvm.server.apc.ttl_limit = 172800
-hhvm.server.light_process_count = 5
-hhvm.server.light_process_file_prefix = /var/tmp/hhvm
-hhvm.timeouts_use_wall_time = true
-include_path = .:/usr/share/php

Maybe the HHVM versions are slightly different though:

On Jessie:

HipHop VM 3.18.6-dev (rel)
Compiler: 3.18.5+dfsg-1+wmf1
Repo schema: 3fcc261f421ec3a327e43ddde3db2682b2a76905

On Stretch using docker-registry.wikimedia.org/releng/composer-package-hhvm:0.1.3:

hhvm --version
HipHop VM 3.18.6-dev (rel)
Compiler: 3.18.5+dfsg-1+wmf1+deb9u1
Repo schema: ddab12b5dd4efd2827d6fa4b9a9a871cd28e5975

+deb9u1 indicates it has been rebuild for stretch (and per changelog).

So probably related to some ini setting?

We have HHVM 3.18 which should include the upstream fix for issue #7647.

The hhvm Debian package ships on both Jessie and Stretch with:

/etc/hhvm/php.ini
; php options

; hhvm specific 
hhvm.log.level = Warning
hhvm.log.always_log_unhandled_exceptions = true
hhvm.log.runtime_error_reporting_level = 8191
hhvm.mysql.typed_results = false

https://docs.hhvm.com/hhvm/configuration/INI-settings#traits__logging__error-reporting-levels so it reports anything but deprecated errors. And that includes STRICT.

Hence, by default, the example code does now yield a strict warning.


Now on CI, the HHVM jobs suffixed with -jessie run on Nodepool labs instances which are provisioned using Puppet. Puppet overrides the configuration file entirely to write P6496, hence hhvm.log.runtime_error_reporting_level is no more set and defaults to HPHP_ALL which does not include the STRICT warnings.

Thus the avro-php tests pass on Nodepool since Strict are not reported, but fail on Docker since Strict errors are.

So I guess we will want to revisit the php.ini confguration file being shipped and most probably make sure strict errors are reported.

So I guess we will want to revisit the php.ini confguration file being shipped and most probably make sure strict errors are reported.

+1 I think that seeing strict standards is reasonable; it will bring up problems earlier, not when they already became important (e.g. for that example: WHat happens if php decides, that non-static methods, called statically, will throw a fatal). It's like not taking notices into account, which could result in weird undicovered problems.