Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#33926 closed defect (bug) (fixed)

Improve WordPress Compatibility with HHVM

Reported by: wjywbs's profile wjywbs Owned by: pento's profile pento
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

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

Attachments (3)

33926-db.patch (964 bytes) - added by wjywbs 9 years ago.
33926-wp-importer.patch (460 bytes) - added by wjywbs 9 years ago.
33926.diff (961 bytes) - added by pento 8 years ago.

Download all attachments as: .zip

Change History (17)

@wjywbs
9 years ago

#1 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4

#2 @wonderboymusic
9 years ago

  • Keywords has-patch added
  • Owner set to pento
  • Status changed from new to reviewing

@pento, can you review the first part?

#3 follow-up: @pento
8 years ago

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.

#4 in reply to: ↑ 3 @wjywbs
8 years ago

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
8 years ago

#5 follow-up: @pento
8 years ago

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?

#6 in reply to: ↑ 5 @wjywbs
8 years ago

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.

#7 @pento
8 years ago

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?

#8 @ocean90
8 years ago

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

#9 @wjywbs
8 years ago

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);

#10 @DrewAPicture
8 years ago

  • Keywords dev-feedback added

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

#11 @ocean90
8 years ago

  • Milestone changed from 4.4 to Future Release

#12 @pento
8 years ago

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.

#13 @pento
8 years ago

  • Milestone changed from Future Release to 4.5
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Version 4.4 deleted

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].

This ticket was mentioned in Slack in #core by pento. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.