[4.0] Fix errors about int columns shown in database schema check on MySQL 8#28370
[4.0] Fix errors about int columns shown in database schema check on MySQL 8#28370richard67 wants to merge 5 commits intojoomla:4.0-devfrom
Conversation
|
@wilsonge This PR makes the schema checker ignore any display size values for columns of type |
|
I think it's probably fine without being more specific if it genuinely has no effect in mysql 5.6/5.7 |
|
Well the effect would be that if you change ONLY the display size attribute of an integer column in an update script, the schema checker would ignore that. |
|
When being more specific, it would behave different depending on db version, so now without being more specific it is even more consistent. |
|
The clean solution would be to query the MySQL schema table instead of using |
|
P.S.: And of course I've verified that using |
|
i can confirm that this pr worked on mysql 8 |
|
I have tested this item ✅ successfully on 985192a This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28370. |
|
@wilsonge I think this PR here has deserved at least the Release Blocker label, if not even the Beta Blocker label, since it solves the database checker showing false errors on MySQL 8 after a clean, new installation. |
|
Tagged as release blocker - it's not a structural API thing so it's not a beta blocker |
|
Strange ... me and @alikon could reproduce it. I have googled a lot to find if there has been some change in MySQL8, but wasn't able to find something. I would be happy if it could be influenced by some session variable so it would not need the change of this PR. |
|
Can be that the change came with 8.0.19. I found this: https://mysqlserverteam.com/the-mysql-8-0-19-maintenance-release-is-generally-available/. At the end in section "Deprecation and removal" I find: It is about @alikon What was the exact version of MySQL 8 you have used? I have to look up my version at home, am in the office now. |
|
I am doing the upgrade now and will report back |
|
@brianteeman What happens if you create a new db on your MySQL 8.0.19 server and make a new Joomla 4 installation into this new db? I ask because I found following statement here https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-19.html in section "Deprecation and Removal Notes":
I.e. if you have updated e.g. a 8.0.14 to an 8.0.19, previously present databases still will show the display width in their integer data types. |
|
now I can confirm the issue |
|
I have tested this item ✅ successfully on 985192a This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28370. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28370. |
|
@wilsonge As long as there are no other data types with names starting with "int" except "int" and the synonym "integer", this PR will be fine. But if you think it is too "hacky" I have to develop a better solution. We will have to remove all display width usage for integer columns in our sql scripts, because MySQL clearly say in the docs I've linked in previous comments that it will be removed in a future version. Question is: When shall we do that? If you say "now", I can make a PR on weekend. To be honest, I am shocked how MySQL can do such breaking changes with a patch version! It is obvious they don't follow Semver practice. And this is the second time we have such breaking change, the other one I remember for which I had to fix the db checker was about upper and lower case for their data types, and as far as I remember it was also a patch version change. |
|
I think this is OK. What concerns me is on the same version of MySQL an upgraded db is different to a new one then there's something practically different in the DB rather than it being a UI thing. Which is somehow more concerning. Is there any practical difference? |
@wilsonge Yes, for the use case you can see in this PR: Someone doing a show tables or a select from the data dictionary and suddently gets different results. Beside this, the display width never had any meaning regarding data value range or storage space, it always was just for zero padding stuff. In our SQL scripts the display width is widely used, and it looks as if people just misunderstood it and thought it has some effect on value range or storage space. Problem is: Even if you leave it away in a create table or modify column statement, i.e. use just "int", the data type returned e.g. by show tables will still be "int(11)" on MySQL 8.0.18 or an old migrated DB in 8.0.19 and be "int" on a new 8.0.19 DB, and if just using "int unsigned" or "integer unsigned", show tables will still return "int(10) unsigned" on an "old" db and "int unsigned" on a new 8.0.19 db. Since 8.0.19, our statements using e.g. "int(11)" will result in a deprecated warning in the SQL log, and in a future version which is not named yet, it will result in an SQL error. Isn't that crazy? And that with the change from 8.0.18 to 8.0.19. |
|
I've updated description and testing instructions by the exact version numbers we have found out here to be relevant for the change in MySQL 8 behavior, and I've added section "Additional information" at the end of the description to document the behavior for updated databases. This doesn't change anything on the tests, they are still valid. |
|
P.S.: This PR here will still be needed when we change our SQL scripts for MySQL to not using display width for integer data types anymore, because 1. the different result of a show table statement depending on db version still remains, and 2. there might be update sql scripts by 3rd party extensions still using display width, and the db schema checker has to check these, too. |
|
my test was on 8.0.19-0ubuntu0.19.10.3 |
|
@wilsonge Silly question: Should this issue be fixed in J3, too? |
|
@HLeithner What do you think, should we fix this in J3, too, like we did it with the other issue for the database checker with MySQL8, #25658 ? |
|
@richard67 yes sure |
@HLeithner @wilsonge What about this one here then? Shall I close it when having made the PR for J3 because it will be merged up anyway? But this might take a while, and I wanna have it fixed soon in J4. |
|
Hmm, it seems on J3 we have that with tinyint, too. |
|
@HLeithner I have made PR for J3: #28501 . When I've tested on J3, the issue applied not only for int but also for tinyint columns. This fits to the documentation of MySQL I've linked here in the comments. For some reason these errors for tinyint are not shown on my J4 database, but this might just be the case in my testing environment but not for others, so I think it should be done in the same way for J4. @wilsonge Shall I update this PR here, or shall I close it because nor there is one for J3, which can be merged up to J4 later as it is? |
|
Ah the error just doesn't happen in J4 with tinyint because there is no 4.0 update sql modifying a tinyint column, like we have them in J3. So it definitely needs that change here, too, in the same way as proposed with #28501 for staging. |
|
Closing in favour of PR #28501 for staging. It can be later merged up to J4 as it is with the regular 3.9.x to 4.0-dev merges. |





Pull Request for Issue #28367 .
Summary of Changes
This Pull Request (PR) changes the database schema checker for MySQL so that any display size is ignored for integer (
int) columns when checking these columns.The reason for this is that beginning with MySQL 8.0.19, the
typein aSHOW COLUMNSstatement doesn't include anymore the display size if the database has been created in that version, while on previous versions it is included. E.g. on MySQL 5.7 or 8.0.18 thetypevalue is "int(11)" or "int(10) unsigned" while on mySQL 8.0.19 it is just "int" or "int unsigned".The display size doesn't have any impact on value range or storage size for an integer column, it only determines how a value would be left-padded with zeros if that would be done, but this padding is deprecated anyway. See https://dev.mysql.com/doc/refman/8.0/en/numeric-type-attributes.html for details.
Testing Instructions
Requirements
Have a MySQL database with version 8.0.19 and another one with an older version, e.g. 5.7 or 8.0.18.
Instructions
Install a clean 4.0-dev without the patch of this PR applied on MySQL 8.0.19.
Then go to the database checker.
Result: You see errors about columns of type
int(10)andint(5)and similar. See section "Actual result" below.Now apply the patch of this PR and go again to the database checker or reload the page if still there.
Result: No errors are shown, see section "Expected result" below.
Now delete configuration.php and install the 4.0-dev with the patch of this PR applied on a MySQL version prior to 8.0.19, e.g. 5.7 or 8.0.18.
Then go to the database checker.
Result: With this PR applied it works as before for MySQL prior to 8.0.19, i.e. there are no database errors after a clean install.
Expected result
All database table structures are up to date.
No problems were found.
Actual result
Documentation Changes Required
None.
Additional information
I found following statement here https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-19.html in section "Deprecation and Removal Notes":
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.I.e. if you have updated e.g. a 8.0.18 to an 8.0.19, previously present databases still will show the display width in their integer data types.