[wp-trac] [WordPress Trac] #49364: dbDelta() should not change display width for integer data types on MySQL 8.0.17+

[WordPress Trac]

#49364: dbDelta() should not change display width for integer data types on MySQL
8.0.17+
-------------------------------------------------+-------------------------
 Reporter:  SergeyBiryukov                       |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  6.1
Component:  Database                             |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-testing early        |     Focuses:
  needs-testing-info                             |
-------------------------------------------------+-------------------------

Comment (by SergeyBiryukov):

 Spent some more time looking into this.

 I was able to reproduce the observations from comment:13:
 > MySQL 8.0.17 changes from `bigint(20)` to `bigint`:
 > {{{
 > Tests_dbDelta::test_column_type_change
 > Failed asserting that two arrays are equal.
 >  Array (
 > -    'wptests_dbdelta_test.id' => 'Changed type of
 wptests_dbdelta_test.id from bigint(20) to int(11)'
 > +    'wptests_dbdelta_test.id' => 'Changed type of
 wptests_dbdelta_test.id from bigint to int(11)'
 >  )
 > }}}
 >
 > MariaDB 10.5.15 changes from `bigint` to `bigint(20)`:
 > {{{
 > Tests_dbDelta::test_column_type_change
 > Failed asserting that two arrays are identical.
 >  Array &0 (
 > -    'wptests_dbdelta_test.id' => 'Changed type of
 wptests_dbdelta_test.id from bigint to int(11)'
 > +    'wptests_dbdelta_test.id' => 'Changed type of
 wptests_dbdelta_test.id from bigint(20) to int(11)'
 >  )
 > }}}
 > ...
 > However, it's not quite clear to me why this works for me locally with
 MariaDB 10.4.12, as well as on some hosts with MariaDB 10.5.15, but not on
 the one with the failures.

 See these more recent test runs for example:
 * Fails: [https://make.wordpress.org/hosting/test-
 results/r53889/kdawsbot-r53889/ PHP 8.0.21 + MariaDB 10.4.25]
 * Passes: [https://make.wordpress.org/hosting/test-
 results/r53889/tuonettibot-r53889/ PHP 7.4.30 + MariaDB 10.4.25]

 MariaDB version is the same, the only notable difference is PHP 8.0.x vs.
 PHP 7.4.x. That is indeed the reason for different results: when running
 the tests with the same versions locally, they fail on PHP 8.0 and pass on
 PHP 7.4.

 The reason is that MariaDB version is reported differently between PHP
 versions:
 * PHP 8.0.21: `10.6.8-MariaDB`
 * PHP 7.4.30: `5.5.5-10.6.5-MariaDB`

 This was previously noted in comment:7:ticket:47738 and appears to be
 addressed in PHP 8.0.16 or later:
 * [https://github.com/php/php-src/issues/7972 php-src: #7972: MariaDB
 version prefix 5.5.5- is not stripped]
 * [https://github.com/php/php-src/pull/7963 php-src: PR #7963 Fix GH-7932:
 MariaDB version prefix not always stripped]

 This leads to the `version_compare( $db_version, '8.0.17', '<' )` checks
 added in [47184] working as expected on PHP 7.4.x and earlier, but not on
 PHP 8.0.x or later. We erroneously set `$bigint_display_width` to an empty
 string for MariaDB, as the 10.6.8 version is greater than 8.0.17. However,
 display width support was only removed for integer data types specifically
 in MySQL 8.0.17+, it is still available and expected in MariaDB.

 Thinking about this some more, I believe [attachment:"49364.diff"] (and
 the refreshed PR) is not quite in the right direction: changing the schema
 would not really solve the issue. Instead, `dbDelta()` should match the
 database server behavior: keep display width for integer data types on
 MariaDB and older MySQL versions, but ignore it on MySQL 8.0.17 or later,
 which would mean treating `BIGINT(20)` same as `BIGINT`, `INT(11)` same as
 `INT`, etc.

 See [attachment:"49364.2.diff"] for the suggested solution, which reverts
 most of [47184] as no longer needed.

 This change is covered by 20+ existing `dbDelta()` tests which currently
 fail on PHP 8.0.x + MariaDB, and pass with this patch. It also resolves an
 issue with plugin activation described in comment:5:
 > The first activation will go without issues. However deactivating and
 re-activating the plugin attempts to run the following (unnecessary) alter
 statement:
 >
 > {{{
 > ALTER TABLE wp_ddwi_test CHANGE COLUMN `id` `id` bigint(20) unsigned NOT
 NULL AUTO_INCREMENT PRIMARY KEY
 > }}}
 >
 > This yields the following error:
 > {{{
 > WordPress database error Multiple primary key defined for query ALTER
 TABLE wp_ddwi_test CHANGE COLUMN `id` `id` bigint(20) unsigned NOT NULL
 AUTO_INCREMENT PRIMARY KEY
 > }}}

 Tested on:
 * PHP 8.0.22 + MariaDB 10.6.8
 * PHP 7.4.30 + MariaDB 10.6.8
 * PHP 5.6.40 + MariaDB 10.6.8
 * PHP 8.0.22 + MySQL 8.0.30
 * PHP 7.4.30 + MySQL 8.0.30
 * PHP 5.6.40 + MySQL 8.0.30

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