Improve WordPress Compatibility with HHVM

[wjywbs]

When running the WordPress unit tests with HHVM, there are 2 failures and 1 error.

1. Do not use empty db handle to avoid catchable fatal error.
2. Do not trim the cdata because CDATA in xml is parsed differently in hhvm and php. (Upstream bug report is at ​https://github.com/facebook/hhvm/issues/5844)

Attached are the fixes for the 2 failures.

The 1 error is:

1) Tests_Auth::test_empty_user_activation_key_fails_key_check
crypt(): No salt parameter was specified. You must use a randomly generated salt and a strong hash function to produce a secure hash.

src/wp-includes/class-phpass.php:272
src/wp-includes/user-functions.php:1879
tests/phpunit/tests/auth.php:257

[wonderboymusic]

@pento, can you review the first part?

[pento]

For 33926-db.patch​:

• When we're using mysqli, we need to be checking $this->dbh instanceof mysqli, not empty( $this->dbh ).
• In ::query(), why is HHVM throwing a fatal error if the connection has gone away? If the query fails due to a 2006 error (MySQL server has gone away), then it'll reconnect and retry the query. We do it this way so that we're not doing a mysqli_ping() for every query, just the ones that fail due to the server going away - HHVM should be able to handle this.

[wjywbs]

Replying to pento:

For 33926-db.patch​:

• When we're using mysqli, we need to be checking $this->dbh instanceof mysqli, not empty( $this->dbh ).

Thanks for your review. I tested that using $this->dbh instanceof mysqli would cause the error of Undefined property: wpdb::$dbh because dbh is unset.

• In ::query(), why is HHVM throwing a fatal error if the connection has gone away? If the query fails due to a 2006 error (MySQL server has gone away), then it'll reconnect and retry the query. We do it this way so that we're not doing a mysqli_ping() for every query, just the ones that fail due to the server going away - HHVM should be able to handle this.

In tests/phpunit/tests/db.php, test_db_reconnect() calls unset( $wpdb->dbh ), so @mysqli_query( $this->dbh, $query ) runs in _do_query(). Since $this->dbh is empty, HHVM will throw a catchable fatal error. The empty db handle must be checked before running _do_query.

[pento]

Ah, I see. I didn't know that about HHVM behaviour. test_db_reconnect() is kind of hacky in the way it works, could you try 33926.diff​ as a tidied up version?

[wjywbs]

Replying to pento:

Ah, I see. I didn't know that about HHVM behaviour. test_db_reconnect() is kind of hacky in the way it works, could you try 33926.diff​ as a tidied up version?

HHVM still throws the fatal error after I applied 33926.diff and reverted 33926-db.patch, so setting dbh to null is similar to unsetting. With 33926-db.patch, the test passes.

[pento]

I've just done some more testing - with HHVM 3.9.1, I was unable to produce the same error that you're seeing. What version of HHVM are you using? Is there anything else I need to do to reproduce it?

[ocean90]

I can confirm that test_db_reconnect() is broken. Sadly I can't get HHVM to show the error message… 33926.diff​ seems not to fix it.

HipHop VM 3.9.1 (rel)
Compiler: tags/HHVM-3.9.1-0-g0f72cfc2f0a01fdfeb72fbcfeb247b72998a66db
Repo schema: 6416960c150a4a4726fda74b659105ded8819d9c

[wjywbs]

Sorry for the delay! Previously I used a self-compiled one, and now I retested with stock HHVM 3.9.1 on Ubuntu. I got a minimal repro for the test_db_reconnect(), but the bug is probably in either phpunit or HHVM.

<?php
class DBTest extends PHPUnit_Framework_TestCase {
  public function testDBQuery() {
    @mysqli_query(null, "SELECT CONNECTION_ID()");
  }
}

Save the file above as DBTest.php and run it as:

$ php phpunit.phar DBTest
PHPUnit 4.8.9 by Sebastian Bergmann and contributors.

.

Time: 273 ms, Memory: 13.50Mb

OK (1 test, 0 assertions)

$ hhvm phpunit.phar DBTest
PHPUnit 4.8.9 by Sebastian Bergmann and contributors.

$

@ocean90 To see the error message, remove the "@" and run the test again:

hhvm ../phpunit.phar DBTest
PHPUnit 4.8.9 by Sebastian Bergmann and contributors.

E

Time: 1.41 seconds, Memory: 16.65Mb

There was 1 error:

1) DBTest::testDBQuery
Argument 1 passed to mysqli_query() must be an instance of mysqli, null given

DBTest.php:4

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.

I still need to test if a WordPress site would function properly in HHVM if I put the following code in the theme, such as header.php (it should work fine with 33926-db.patch):

global $wpdb;
$var = $wpdb->get_var( "SELECT ID FROM $wpdb->users LIMIT 1" );
var_dump($var);

unset( $wpdb->dbh );

$var = $wpdb->get_var( "SELECT ID FROM $wpdb->users LIMIT 1" );
var_dump($var);

[DrewAPicture]

@pento Are we still at a place where this should go in with 4.4 or punt?

[pento]

In 36084:

User: Don't continue checking a password reset key, if the hash is empty.

An empty reset key hash will never be valid, so we can skip seeing if it can be used to validate the given key, and return a failure early.

This fixes a warning in the unit tests under HHVM.

See #33926.

[pento]

Can't reproduce the mysqli bug in trunk, with hhvm 3.11 and phpunit 4.3.5.

There's a new failure occurring on Travis:

1) Tests_Import_Parser::test_escaped_cdata_closing_sequence
Parser WXR_Parser_XML
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Content with nested <![CDATA[ tags ]]> :)'
+'Content with nested <![CDATA[ tags]]> :)'
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/import/parser.php:161

But Travis uses hhvm 3.6.6, I can't reproduce it with the newer version.

That only leaves the crypt() error, which was fixed in [36084].