MySQL 8.0.19 Removes the int LENGTH

[williamdes]

1) PhpMyAdmin\Tests\Selenium\Table\StructureTest::testAddColumn
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'int(11)'
+'int'

Using docker image

Ref: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-19.html

Display width specification for integer data types was deprecated in MySQL 8.0.17, and now statements that include data type definitions in their output no longer show the display width for integer types, with these exceptions:

The type is TINYINT(1). MySQL Connectors make the assumption that TINYINT(1) columns originated as BOOLEAN columns; this exception enables them to continue to make that assumption.

The type includes the ZEROFILL attribute.

This change applies to tables, views, and stored routines, and affects the output from SHOW CREATE and DESCRIBE statements, and from INFORMATION_SCHEMA tables.

For DESCRIBE statements and INFORMATION_SCHEMA queries, output is unaffected for objects created in previous MySQL 8.0 versions because information already stored in the data dictionary remains unchanged. This exception does not apply for upgrades from MySQL 5.7 to 8.0, for which all data dictionary information is re-created such that data type definitions do not include display width. (Bug #30556657, Bug #97680)

The demo server has 8.0.18

cc @MauricioFauth @ibennetch

Feel free to double check.
From what I tested it makes either useless changing the length or impossible using the GUI as the field is always empty

[ibennetch]

Okay, good information. We'll have to consider a few instances where this could affect us:

Creating tables

When using the GUI to create or modify a table, we should check the MySQL version and ignore the length for MySQL 8.0.19 and newer. A new feature could disable the length field except for when the appropriate data types are selected (enum, set, varchar, varbinary), but that's beyond the scope of this particular issue. There's currently no error shown so this doesn't seem to be a big deal at the moment.

SQL linter

Do we need to make sql-parser aware of this, so that it properly lints queries? AFAIK, we don't have any way to detect particular SQL flavors (MySQL vs MariaDB, etc) so this might be a large undertaking.

Import

I don't think we do anything to the import engine. If people try to import malformed SQL, that's not something we usually detect and I think this falls in to a similar behavior. We should just pass along the SQL statements to MySQL and let MySQL handle or fail the statements.

[iifawzi]

As I can see, for this to be done, we should only check the MySQL version and ignore the length for MySQL 8.0.19 and above if the type is SMALLINT,MEDIUMINT,BIGINT, or INT, and the attribute isn't ZEROFILL am I right? i'm willing to work on it.

[williamdes]

As I can see, for this to be done, we should only check the MySQL version and ignore the length for MySQL 8.0.19 and above if the type is SMALLINT,MEDIUMINT,BIGINT, and INT, and the attribute isn't ZEROFILL am I right? i'm willing to work on it.

I am not sure about the solution, maybe @ibennetch would have better advice to say

[iifawzi]

My thought for this is that we should check the version, type and the attribute. Which maybe could be done with a separate utility which then will be used in the if condition here:

phpmyadmin/libraries/classes/Table.php

Lines 545 to 550 in 2291814

	if (strlen($length) !== 0 && ! preg_match($pattern, $type)) {
	// Note: The variable $length here can contain several other things
	// besides length - ENUM/SET value or length of DECIMAL (eg. 12,3)
	// so we can't just convert it to integer
	$query .= '(' . $length . ')';
	}

If you don't prefer to use a separate utility for this, we can make it inline in the same file, but i think it will look messy.
cc @williamdes @ibennetch

[ibennetch]

You're on to the right idea. We test for MySQL and MariaDB versions in several places throughout the code, for instance https://github.com/phpmyadmin/phpmyadmin/com#diff-b47bc389d7238e77949201866d47131c8d49e0cccb303558010a28aae233a196 or most notably in the many calls to dbi->getVersion() in https://github.com/phpmyadmin/phpmyadmin/blob/master/libraries/classes/Server/Privileges.php.

The test should be:

• if the server is MySQL, and
• the version is 8.0.18 or higher,
• and if the column type is any of SMALLINT, MEDIUMINT, INT, or BIGINT (not including TINYINT, as you correctly evaluated), and
• if it does not have ZEROFILL.

That's a lot to check, so if we reuse the check anywhere it can be turned in to a utility function in libraries/Util.php, but I think it's fine to do the check in the page when we show the create/edit dialog.

However, I'm not sure what we need to do right now. Including the length causes MySQL to ignore it, not throw an error, so we're doing okay there. The table structure tab displays correctly because we just show what MySQL tells us about the table. What we might want to do is show a warning message (similar to the warnings we show on the Insert page; I don't recall anything we warn about from the structure page), that warning could just say "The column width is ignored in your MySQL version unless defining a TINYINT(1) column or using the ZEROFILL attribute".

I'm not sure if any changes should be made to the linter, that could be discussed further.

[iifawzi]

However, I'm not sure what we need to do right now. Including the length causes MySQL to ignore it, not throw an error, so we're doing okay there. The table structure tab displays correctly because we just show what MySQL tells us about the table. What we might want to do is show a warning message (similar to the warnings we show on the Insert page; I don't recall anything we warn about from the structure page), that warning could just say "The column width is ignored in your MySQL version unless defining a TINYINT(1) column or using the ZEROFILL attribute".

Alright, are you thinking about only showing a warning message, and not ignore it ourselves as I stated in my previous comment?

For the warning message, is it how it should appears?

If this's the intended way, wouldn't it be better to mention that it's only about the integer types?
"The column width is ignored in your MySQL version for the integer types unless defining a TINYINT(1) column or using the ZEROFILL attribute"

And I think as per #16742, the ZEROFILL shouldn't be checked nor mentioned in the warning message.

I'm not sure for this case, if this is the correct way to show a warning message (using the twig file and hide/show using functions.js), point me out if i'm wrong, anyway, I think it gonna work, a warning will be shown if there's any inappropriate integer type column (not TINYINT(1))