[wp-trac] [WordPress Trac] #54877: Occasional PHP exception being thrown on WPDB/MySQLi connections

[WordPress Trac]

#54877: Occasional PHP exception being thrown on WPDB/MySQLi connections
-----------------------------+---------------------
 Reporter:  johnjamesjacoby  |       Owner:  (none)
     Type:  enhancement      |      Status:  new
 Priority:  normal           |   Milestone:  6.0
Component:  Database         |     Version:  1.5
 Severity:  normal           |  Resolution:
 Keywords:  2nd-opinion      |     Focuses:
-----------------------------+---------------------
Description changed by johnjamesjacoby:

Old description:

> ** PHP Debug Notice **
> {{{
> mysqli::real_connect() expects parameter 5 to be integer, string given -
> wpdb::_do_query /wp-includes/wp-db.php:2056
> }}}
>
> The above notice appears in logs once for approximately every 250k
> requests. I have a hunch it's happening on reconnection attempts (using
> `$this->dbh`) but don't have any hard evidence to back up that claim.
>
> My `DB_HOST` is `localhost`, and I've also seen this happen with RDS
> (`xxx.rds.amazonaws.com`).
>
> ----
>
> I've spent a few hours researching this, and am including that research
> below:
>
> **WordPress DB_HOST Documentation**
> * https://wordpress.org/support/article/editing-wp-config-php/#set-
> database-host
>
> The above docs confirm that `localhost:3306` is the recommended way of
> adding a port number.
>
> **CodeIgniter example & fix**
> * https://github.com/bcit-ci/CodeIgniter/issues/5627
> * https://github.com/bcit-
> ci/CodeIgniter/commit/41bba2fea6fafa315db0a21093722f920455e3b1
>
> The above links confirm that CodeIgniter was defaulting to a string
> (causing the same issue) instead of null or int, and patched it
> internally.
>
> **Facebook, HHVM, HACK**
> * https://github.com/facebook/hhvm/issues/2482
> *
> https://github.com/facebook/hhvm/commit/f03f434e2eacda604b32bc7e4aa3334ed956a83a
>
> The above issue confirms that Facebook saw the same issue, and decided to
> patch it inside of HHVM.
>
> **Tangential WordPress tickets**
> * #41722 - Adding IPv6 support
> * #42496 - Improving MySQL 8.0 support
> * #21663 - Adding mysqli support
>
> The above tickets show an abbreviated timeline of surrounding changes to
> wpdb. No regressions. No bug introductions. Simply some related research.
>
> **PHP MySQL unit tests**
> * https://github.com/php/pecl-database-
> mysql/blob/6ca4fa4b509819ecc4c8ff2b28080d60ad64acc6/tests/mysql_connect.phpt
>
> The above link shows how PHP itself tests MySQL (not MySQLi) database
> connections.
>
> **PHP MySQLi Docs**
> * https://www.php.net/manual/en/class.mysqli.php
>
> The above link is my favorite. Specifically:
>
> {{{
> class mysqli {
>     (...)
>     public __construct(
>         string $hostname = ini_get("mysqli.default_host"),
>         string $username = ini_get("mysqli.default_user"),
>         string $password = ini_get("mysqli.default_pw"),
>         string $database = "",
>         int $port = ini_get("mysqli.default_port"),
>         string $socket = ini_get("mysqli.default_socket")
>     )
>     (...)
>     public real_connect(
>         string $host = ?,
>         string $username = ?,
>         string $passwd = ?,
>         string $dbname = ?,
>         int $port = ?,
>         string $socket = ?,
>         int $flags = ?
>     ): bool
> }
> }}}
>
> `ini_get()` always returns a string, and PHP's mysqli class constructs &
> defaults its `$port` using it. In the PHP source, it does not appear to
> be type cast into an integer.
>
> ----
>
> **Hypothesis**
>
> I believe this small and random-feeling annoyance can be fixed quite
> easily in WordPress in much the same way it's been fixed in HHVM &
> CodeIgniter.
>
> I have a hunch that: web hosts silence or ignore these notices; they've
> been happening for a very long time; the problem is small enough where it
> wasn't worth anyone else digging too deep into.
>
> ----
>
> **Conclusion**
>
> * `wpdb::parse_db_host()` parses `$this->dbhost` looking for IPv4/IPv6,
> port number, sockets, etc...
> * `$this->dbhost` is gotten from `DB_HOST`
> * Casting `$post` to `(int)` when non-null will ensure
> `mysqli_real_connect()` will only ever see `null` or `int` type values
>
> Something like:
> {{{
> $port = $port ? absint( $port ) : null;
> }}}

New description:

 ** PHP Debug Notice **
 {{{
 mysqli::real_connect() expects parameter 5 to be integer, string given -
 wpdb::_do_query /wp-includes/wp-db.php:2056
 }}}

 The above notice appears in logs once for approximately every 250k
 requests. I have a hunch it's happening on reconnection attempts (using
 `$this->dbh`) but don't have any hard evidence to back up that claim.

 My `DB_HOST` is `localhost`, and I've also seen this happen with RDS
 (`xxx.rds.amazonaws.com`).

 ----

 I've spent a few hours researching this, and am including that research
 below:

 **WordPress DB_HOST Documentation**
 * https://wordpress.org/support/article/editing-wp-config-php/#set-
 database-host

 The above docs confirm that `localhost:3306` is the recommended way of
 adding a port number.

 **CodeIgniter example & fix**
 * https://github.com/bcit-ci/CodeIgniter/issues/5627
 * https://github.com/bcit-
 ci/CodeIgniter/commit/41bba2fea6fafa315db0a21093722f920455e3b1

 The above links confirm that CodeIgniter was defaulting to a string
 (causing the same issue) instead of null or int, and patched it
 internally.

 **Facebook, HHVM, HACK**
 * https://github.com/facebook/hhvm/issues/2482
 *
 https://github.com/facebook/hhvm/commit/f03f434e2eacda604b32bc7e4aa3334ed956a83a

 The above issue confirms that Facebook saw the same issue, and decided to
 patch it inside of HHVM.

 **Tangential WordPress tickets**
 * #41722 - Adding IPv6 support
 * #42496 - Improving MySQL 8.0 support
 * #21663 - Adding mysqli support

 The above tickets show an abbreviated timeline of surrounding changes to
 wpdb. No regressions. No bug introductions. Simply some related research.

 **PHP MySQL unit tests**
 * https://github.com/php/pecl-database-
 mysql/blob/6ca4fa4b509819ecc4c8ff2b28080d60ad64acc6/tests/mysql_connect.phpt

 The above link shows how PHP itself tests MySQL (not MySQLi) database
 connections.

 **PHP MySQLi Docs**
 * https://www.php.net/manual/en/class.mysqli.php

 The above link is my favorite. Specifically:

 {{{
 class mysqli {
     (...)
     public __construct(
         string $hostname = ini_get("mysqli.default_host"),
         string $username = ini_get("mysqli.default_user"),
         string $password = ini_get("mysqli.default_pw"),
         string $database = "",
         int $port = ini_get("mysqli.default_port"),
         string $socket = ini_get("mysqli.default_socket")
     )
     (...)
     public real_connect(
         string $host = ?,
         string $username = ?,
         string $passwd = ?,
         string $dbname = ?,
         int $port = ?,
         string $socket = ?,
         int $flags = ?
     ): bool
 }
 }}}

 `ini_get()` always returns a string, and PHP's mysqli class constructs &
 defaults its `$port` using it. In the PHP source, it does not appear to be
 type cast into an integer.

 ----

 **Hypothesis**

 I believe this small and random-feeling annoyance can be fixed quite
 easily in WordPress in much the same way it's been fixed in HHVM &
 CodeIgniter.

 I have a hunch that: web hosts silence or ignore these notices; they've
 been happening for a very long time; the problem is small enough where it
 wasn't worth anyone else digging too deep into.

 ----

 **Conclusion**

 * `wpdb::parse_db_host()` parses `$this->dbhost` looking for IPv4/IPv6,
 port number, sockets, etc...
 * `$this->dbhost` is gotten from `DB_HOST`
 * Casting `$port` to `(int)` when non-null will ensure
 `mysqli_real_connect()` will only ever see `null` or `int` type values

 Something like:
 {{{
 $port = $port ? absint( $port ) : null;
 }}}

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54877#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform