Page MenuHomePhabricator

Convert Wikimedia production HHVM instances to have hhvm.php7.all set true
Closed, DeclinedPublic

Description

Before we insist other users of MediaWiki can only use PHP 7+, we should probably be using it ourselves.

That said, no doubt there are gotchas and issues we've not spotted yet, so this would have to be carefully done (start with unit tests, then one machine, then…).

Probably want to do some more digging into the code before enabling...
https://docs.hhvm.com/hhvm/configuration/INI-settings#php-7-settings

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 21 2017, 9:49 PM

In principle, nothing prevents us from switching deployment-prep right now by editing https://wikitech.wikimedia.org/wiki/Hiera:Deployment-prep I think that would be something like

"hhvm::extra::cli":
    hhvm:
        php7:
            all: 1
"hhvm::extra::fcgi":
    hhvm:
        php7:
            all: 1
aaron triaged this task as Low priority.Aug 23 2017, 6:54 PM
Joe added a subscriber: Joe.Aug 28 2017, 8:21 AM

Beware that, as HHVM developers declared themselves, the php 7 implementation in HHVM will never be 100% compatible with the PHP one.

So we will need to support php 7 and hhvm-with-php7-compat anyways, and treat the two as separated platforms.

As a minimum, we need to make mediawiki-config php7-compatible.

This probably needs to be closed due to T176370: Migrate to PHP 7 in WMF production - unless someone thinks that HHVM's PHP7 mode can be a convenient way to test stuff before PHP7 migration proper (which I strongly doubt).

Duped in or sub-tree'ed, yes.

tstarling closed this task as Declined.Sep 21 2017, 12:44 AM
tstarling added a subscriber: tstarling.

I don't think it's a duplicate, we could theoretically do both. But like Max says, there's not really a rationale for it anymore.

Paladox removed a subscriber: Paladox.Sep 21 2017, 12:54 AM

It is likely that we will need to implement T172165: Require either PHP 7.0+ or HHVM in MW 1.31 for the MediaWiki 1.31 release before Wikimedia moves over to PHP 7. Should we reconsider opening this?

Anomie added a subscriber: Anomie.EditedOct 2 2017, 3:07 PM

The HHVM documentation for the setting states that it controls the following:

  1. Disallow and warn when using old PHP 4 constructors
  2. Enable throwing the new Error heirarchy of exceptions
  3. Change some edge-case int and float behavior, such as divide/mod by zero
    • Specifically, that seems to be casts of NAN/INF to int and behavior of large bitshifts.
  4. Make order of assignment in list() lvalues consistent
  5. Don't consider hex strings to be numeric
    • e.g. is_numeric( "0x1a" ) now results in false, $a += "0x1a" adds 0 rather than 26, and so on. But (int)"0x1a" isn't changing because PHP5 was inconsistent and returned 0 here.
  6. Enable PHP 7-style scalar type annotations (NB: not the same as Hack's)
  7. Fix some odd precedence and order of evaluation issues
    • Interpretation of stuff like $foo->$bar['baz'] and support for stuff like (...)['foo'] and (...)->foo() for more values of (...).

Other PHP7 features implemented in HHVM are allowed even if the ini setting isn't set, it's just these where PHP5 and PHP7 behavior differs that are affected.

We could potentially keep the PHP5 compat code for #2 and #7 (and maybe #5 if there is any) and hold off on depending on #3, #4, #5, and #6 until WMF switches to PHP7, while still allowing the use of other PHP7 features in 1.31.

Mentioned in SAL (#wikimedia-releng) [2017-10-14T00:47:29Z] <MaxSem> Trying PHP7 mode on depoyment-prep with https://wikitech.wikimedia.org/w/index.php?diff=1772791 (ping T173786)

So I tried briefly enabling the PHP7 mode on deployment-prep, it just immediately exploded with PHP fatal error /srv/mediawiki/php-master/vendor/monolog/monolog/src/Monolog/Logger.php line 318: __construct() expects parameter 1 to be string, null given

The line in question is

$ts = new \DateTime(null, static::$timezone);

From which we can tell that HHVM goes bonkers with type strictness in this mode and that it's probably not worth our time to try getting PHP7 mode out because it works too differently.

It looks like that's upstream bug https://github.com/facebook/hhvm/issues/7198. In short, PHP7 allows passing null to (some?) built-in functions with scalar type hints but not user-defined functions with the same hints, while HHVM prohibited both. It looks like that difference in behavior was fixed in HHVM 3.21 and backported to 3.20. We're on 3.18.

Another option would be to enable all the php7 compat options except the buggy "hhvm.php7.scalar_types".