[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:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  6.1
Component:  Database                             |     Version:  1.5
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-testing needs-unit-  |     Focuses:
  tests 2nd-opinion early commit                 |
-------------------------------------------------+-------------------------

Comment (by costdev):

 @audrasjb @peterwilsoncc I have reviewed the patch and the tests, and ran
 a coverage report.

 **TL;DR** - The tests cover numeric strings and absent ports. The regex
 removes the need to facilitate non-numeric strings, but this doesn't
 appear to be covered in the datasets to protect BC. This isn't
 ''necessarily'' in the scope of this ticket, but could be added in this
 patch if desired. If not, I think the `needs-unit-tests` can be safely
 removed and the patch can go forward for `commit` consideration.

 -----

 - `wpdb::parse_db_host()` parses using `preg_match()`. On successful
 match, the port will be a string.
 - The `port` group of regex pattern is `(?P<port>[\d]+)`. Non-digits
 should never be matched.
 - `$port = $port ? absint( $port ) : null;` will `absint()` the `string`,
 returning either a positive `int`, or `0`.
 - For non-digits, `$port` will already be `null` as `preg_match()` won't
 match these. The ternary will continue to treat this as `null`.

 The current functionality seems safe to me. However, the tests should
 enforce BC. The one test I can't see is one in which the port is a non-
 numeric string.

 That is:

 {{{#!php
 <?php
 array(
         '127.0.0.1:port_as_string:/tmp/mysql.sock',
         false,
         '127.0.0.1',
         null, // Parsed port.
         '/tmp/mysql.sock',
         false,
 ),
 }}}

 This helps ensure that the regex is never changed to accidentally match
 non-digits. The coverage report screenshot below shows this dataset
 correctly returns `$port` as `null`.

 Granted, `port_as_string` seems a nonsensical port value, but it fulfils
 the purpose of testing a non-numeric port. Any alternative non-numeric
 value could be used here if preferred.

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