dbDelta() should not change display width for integer data types on MySQL 8.0.17+

[SergeyBiryukov]

Background: #49344

​MySQL 8.0.17 removed support for the display width attribute for integer data types:

As of MySQL 8.0.17, the ZEROFILL attribute is deprecated for numeric data types, as is the display width attribute for integer data types. Support for ZEROFILL and display widths for integer data types will be removed in a future MySQL version. Consider using an alternative means of producing the effect of these attributes.

With this change, when creating a table, BIGINT(20) becomes just BIGINT, causing quite a few failures in tests/dbdelta.php (25 in total). They all look similar to this:

2) Tests_dbDelta::test_column_type_change
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 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)'
 )

tests/phpunit/tests/dbdelta.php:151

Most of these failures will be addressed in #49344.

This ticket is for addressing the remaining failure caused by the same BIGINT(20)/BIGINT discrepancy coming from wp_get_db_schema():

1) Tests_dbDelta::test_wp_get_db_schema_does_no_alter_queries_on_existing_install                                                                           
Failed asserting that an array is empty.                                                                                                                                                                                                                                                                                
tests/phpunit/tests/dbdelta.php:693  

Currently, when running on MySQL 8.0.17+, dbDelta() tries to convert BIGINT back to BIGINT(20), INT back to INT(11), etc. This does not have any effect on the database, but is pointless and should not happen.

[SergeyBiryukov]

In 47184:

Tests: Allow dbDelta() tests to (mostly) run on MySQL 8.0.11+.

• MySQL 8.0.11 changed the GeometryCollection data type name to GeomCollection, with the latter being the preferred name.
• MySQL 8.0.17 removed support for the display width attribute for integer data types. Previously, default display width of 20 digits was used: BIGINT(20).

The affected tests now check the MySQL server version and use the appropriate data types.

This leaves one unresolved failure on MySQL 8.0.17+ to be addressed in the future, caused by the same BIGINT display width discrepancy coming from wp_get_db_schema().

Props kaggdesign, ottok, jeremyfelt, SergeyBiryukov.
Fixes #44384, #49344. See #49364.

[SergeyBiryukov]

Related: #51740, #52496.

[leewillis77]

The original ticket states:

Currently, when running on MySQL 8.0.17+, dbDelta() tries to convert BIGINT back to BIGINT(20), INT back to INT(11), etc. This does not have any effect on the database, but is pointless and should not happen.

However, this isn't always a silent failure. It can cause failures where an INT with a display width is also a primary key. Consider the plugin below:

<?php
/*
 * Plugin Name: dbDelta Display Width Issue
 * Description: dbDelta Display Width Issue
 * Version: 1.0.0
*/

if ( ! defined( 'ABSPATH' ) ) {
        exit; // Exit if accessed directly
}

function ddwi_install() {
        require_once ABSPATH . 'wp-admin/includes/upgrade.php';

        dbDelta( <<<SQL
CREATE TABLE `wp_ddwi_test` (
        `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY,
 )
SQL
        );
}

register_activation_hook( __FILE__, 'ddwi_install' );

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

and gives an error on WP-CLI , or through online activation about excess output produced during activation.

[desrosj]

I'd like to solve the MySQL 8 test failures for 5.9 to that we can look at expanding our testing combinations.

Add regx to dbDelta() function so any calls in the old form are handled

Trac ticket: https://core.trac.wordpress.org/ticket/49364

Add regex to dbDelta() function so any calls in the old form are handled
and included the corrected schema

Trac ticket: https://core.trac.wordpress.org/ticket/49364

[pbearne]

Hi All

I dug into this and checked the testing

It seems to me that we will have issues with current code/plugins still trying to use the old format
So I added a regx_replace to fix the SQL on the fly

Maybe we should look at adding a do_it_wrong notice but I didn't do that it would increase the overhead

The new patch is in pull request #1781 and the combined patch is in #1782

I hope this helps

Paul

[pbearne]

also tested on mariadb:10.5

[hellofromTonya]

5.9 Beta 1 is in 4 days. Like the other MySQL 8.0 tickets, moving to 6.0.

[JavierCasares]

I'm having this problems with MariaDB 10.5.

​https://make.wordpress.org/hosting/test-results/r52797/wpsabot-r52797/

I think this may be related with

@@SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION

Mainly: STRICT_TRANS_TABLES. MariaDB 10.2.4+ has the STRICT_TRANS_TABLES by default...

​https://mariadb.com/kb/en/sql-mode/#strict_trans_tables

Strict mode. Statements with invalid or missing data are aborted and rolled back, except that for non-transactional storage engines and statements affecting multiple rows where the invalid or missing data is not the first row, MariaDB will convert the invalid value to the closest valid value, or, if a value is missing, insert the column default value. Default since MariaDB 10.2.4.

[SergeyBiryukov]

Replying to JavierCasares:

I'm having this problems with MariaDB 10.5.

​https://make.wordpress.org/hosting/test-results/r52797/wpsabot-r52797/

I think this may be related with

@@SQL_MODE: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION

Mainly: STRICT_TRANS_TABLES. MariaDB 10.2.4+ has the STRICT_TRANS_TABLES by default...

Thanks! I'm not sure it's related to STRICT_TRANS_TABLES though, as WordPress specifically disables this and other incompatible modes as of [27072] / #26847. Perhaps the disabling fails for some reason?

It's interesting that the failure messages are the exact opposite of what I had in the ticket description.

MySQL 8.0.17 changes from bigint(20) to bigint:

Tests_dbDelta::test_column_type_change
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 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.
--- Expected
+++ Actual
@@ @@
 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)'
 )

It also looks like the same tests do successfully pass on another host:
​https://make.wordpress.org/hosting/test-results/r52797/cloudingbot-r52797/

I think the test changes in [47184] may need some adjustment, specifically this check:

if ( version_compare( $db_version, '8.0.17', '<' ) ) {
	// Prior to MySQL 8.0.17, default width of 20 digits was used: BIGINT(20).
	$this->bigint_display_width = '(20)';
}

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. It might be helpful to check whether STRICT_TRANS_TABLES is related or not, and make sure that the disabling actually works.

[JavierCasares]

I can try to change the SQL_MODE in my machine (I don't have anything in this machine, is only to do tests). So I can try and check changing this option.

I'll do a manual test and leave the report here.

[JavierCasares]

Well... after testing it without the STRICT_TRANS_TABLES it gave me the same errors... so, no, it seems not to be that. I'm going to do some more testing and configurations.

[costdev]

Per the discussion in the bug scrub, I'm moving this to the 6.1 milestone and adding the early keyword.

There are merge conflicts on the PRs and this needs more testing.

[costdev]

This ticket was discussed in the recent bug scrub.

@pbearne, do you have time to refresh the PR to resolve merge conflicts?

[costdev]

@pbearne has refreshed ​PR 1782.

This could use more testing. Can anyone perform this testing and include some testing instructions so that the Test team can pick this one up?

Thanks!

[johnbillion]

@pbearne Would you be kind enough to add testing instructions for this change?

[pbearne]

This is not easy to test

What I did was do a fresh install on an old DB (per 8.0.17) and a new DB (after 8.0.17)

I then checked the DB Schema to make sure they were set correctly.

[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: ​PHP 8.0.21 + MariaDB 10.4.25
• Passes: ​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 $wpdb->db_version() reports MariaDB version 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:

• ​php-src: #7972: MariaDB version prefix 5.5.5- is not stripped
• ​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 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 49364.2.diff​ for the suggested solution, which reverts most of [47184] as no longer needed.

This change is covered by existing unit tests:

• 20+ dbDelta() tests which currently fail on PHP 8.0.x + MariaDB and pass with this patch.
• One test from the ticket description which currently fails on MySQL 8.0.17+ and passes 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

[SergeyBiryukov]

In 53897:

Database: Ignore display width for integer data types in dbDelta() on MySQL 8.0.17 or later.

MySQL 8.0.17 deprecated the display width attribute for integer data types:

As of MySQL 8.0.17, the ZEROFILL attribute is deprecated for numeric data types, as is the display width attribute for integer data types. You should expect support for ZEROFILL and display widths for integer data types to be removed in a future version of MySQL. Consider using an alternative means of producing the effect of these attributes. For example, applications can use the LPAD() function to zero-pad numbers up to the desired width, or they can store the formatted numbers in CHAR columns.

In practice, this means that display width is removed for integer types when creating a table:

• BIGINT(20) → BIGINT
• INT(11) → INT
• MEDIUMINT(9) → MEDIUMINT
• SMALLINT(6) → SMALLINT
• TINYINT(4) → TINYINT

Note: This only applies specifically to MySQL 8.0.17 or later. In MariaDB, display width for integer types is still available and expected.

This commit ensures that dbDelta(), which relies on the DESCRIBE SQL command to get the existing table structure and field types, when running on MySQL 8.0.17 or later, does not unnecessarily attempt to convert BIGINT fields back to BIGINT(20), INT back to INT(11), etc. When comparing the field type in the query with the existing field type, if display width is the only difference, it can be safely ignored to match MySQL behavior.

The change is covered by existing dbDelta() unit tests:

• A test for not altering wp_get_db_schema() queries on an existing install using MySQL 8.0.17+ now passes.
• More than twenty tests which previously failed on PHP 8.0.x + MariaDB due to incorrect expectations, caused by MariaDB version reporting not being consistent between PHP versions, now pass.

References:

• ​MySQL: Nymeric Type Attributes
• ​MariaDB: Numeric Data Types

Follow-up to [1575], [18899], [37525], [47183], [47184].

Props SergeyBiryukov, pbearne, leewillis77, JavierCasares, desrosj, costdev, johnbillion.
Fixes #49364. See #51740.

Thanks for the PR! This is now addressed in r53897.

Thanks for the PR! This is now addressed in r53897.