Remove all default values for type 'text' or 'mediumtext' - redo of PR #17860 by richard67 · Pull Request #27937 · joomla/joomla-cms

richard67 · 2020-02-15T13:06:21Z

Redo of Pull Request (PR) #17860 .

Important for maintainers: When merging, please don't do a squash commit. I don't want the commit history and so the credits for the author of the original PR being lost.

Summary of Changes

Removed all default values for SQL type text or in case of MySQL also mediumtext for databases MySQL and PostgreSQL. For MS SQL Server nothing to be done since there VARCHAR columns are used in these cases.

Thanks to @eXsiLe95 for the original PR.

There were a few things missing and to be changed, see comments in the original PR.

This PR here takes over the work and completes it. Commit history is preserved so some credits still go to the original PR's author. In addition, a few inconsistencies between PostgreSQL and MySQL are fixed regarding such database columns of SQL type text or in case of MySQL also mediumtext.

Testing Instructions

Important: Both of the following 2 tests should be tested on both kinds of database MySQL and PostgreSQL. If you have both kinds of database available, test both. In any case please report back with which kind(s) of database you have tested, and in case or errors also the version(s).

Test 1 - New installation

Step 1: Apply the patch of this PR to a clean, current staging branch or nightly build or a 3.9.15.

Step 2: Make a new installation.

Step 3: Login to backend.

Step 4. Go to Extensions -> Manage -> Database.

Result: No database problems shown. Database schema version is 3.9.16-2020-02-15.

Step 5: Follow the testing instructions of PR #17860 , i.e.:

Go to administrator > Modules and set it to Administrator.
Try to create a new Administrator Menu (for example), so that there is no editor tool in the creation window (because that's where the content comes from).
Save this module.

Result: No SQL error, module is saved.

Step 6: Verify that following works as before and there are no SQL errors shown which are related to invalid default values for columns of type text or in case of MySQL also mediumtext:

Creating and editing categories
Creating and editing fields and field groups
Smart Search
Action logs
Content history

Result: No SQL errors related to invalid default values for columns of type text or in case of MySQL also mediumtext.

Step 7: Using a tool like e.g. PhpMyAdmin or PhpPgAdmin, verify in Joomla database tables that none of the table columns of SQL type text or in case of MySQL also mediumtext has a default value.

Result: No database table columns of SQL type text or in case of MySQL also mediumtext with a default value.

Test 2 - Update SQL scripts

Step 1: On a clean, current staging branch or nightly build or a 3.9.15 without the patch of this PR applied, or on an older Joomla version, make a new installation.

Step 2: Apply the patch of this PR.

Step 3: Login to backend.

Step 4: Go to Extensions -> Manage -> Database.

Result: Depends on the databasase type and the Joomla version.

MySQL: On current staging or nightly, only "1 Problem: Database schema version not up to date" is shown. For older Joomla versions there might be some database errors shown similar to those mentioned for PostgreSQL. These errors then refer to wrong database columns of type text or mediumtext.
PostgreSQL: On current staging or nightly, 34 database problems are found, which all belong to update sql script 3.9.16-2020-02-15.sql. The problems refer to wrong database columns of type text. For older Joomla versions there might be shown more database errors of this type.

Step 5: Use the "Fix" button.

Result: No database problems shown. Database schema version is 3.9.16-2020-02-15.

Step 6: Follow the testing instructions of PR #17860 , i.e.:

Go to administrator > Modules and set it to Administrator.
Try to create a new Administrator Menu (for example), so that there is no editor tool in the creation window (because that's where the content comes from).
Save this module.

Result: No SQL error, module is saved.

Step 7: Verify that following works as before and there are no SQL errors shown which are related to invalid default values for columns of type text or in case of MySQL also mediumtext:

Creating and editing categories
Creating and editing fields and field groups
Smart Search
Action logs
Content history

Result: No SQL errors related to invalid default values for columns of type text or in case of MySQL also mediumtext.

Step 8: Using a tool like e.g. PhpMyAdmin or PhpPgAdmin, verify in Joomla database tables that none of the table columns of SQL type text or in case of MySQL also mediumtext has a default value.

Result: No database table columns of SQL type text or in case of MySQL also mediumtext with a default value.

Test 3 - Update to patched nightly build of today plus this PR applied

Step 1: On a Joomla 3.9.15 or any other 3.9.x but not current staging or nightly, go to the Joomla Update Component, change update channel to "Custom URL" and enter following custom URL:
https://test5.richard-fath.de/test-pr-27937_list.xml.
Or download the patched update comtainer from here
https://test5.richard-fath.de/Joomla_3.9.16-dev-Development-Update_Package_2020-02-15_pr-27937.zip
and use the "Upload & Update" functionality of the Joomla Update component, then you can also update a current staging or nightly.

Step 2: Start the update.

Result: Update finishs with success.

Step 3. Go to Extensions -> Manage -> Database.

Result: No database problems shown. Database schema version is 3.9.16-2020-02-15.

Step 4: Follow the testing instructions of PR #17860 , i.e.:

Go to administrator > Modules and set it to Administrator.
Try to create a new Administrator Menu (for example), so that there is no editor tool in the creation window (because that's where the content comes from).
Save this module.

Result: No SQL error, module is saved.

Step 5: Verify that following works as before and there are no SQL errors shown which are related to invalid default values for columns of type text or in case of MySQL also mediumtext:

Creating and editing categories
Creating and editing fields and field groups
Smart Search
Action logs
Content history

Result: No SQL errors related to invalid default values for columns of type text or in case of MySQL also mediumtext.

Step 6: Using a tool like e.g. PhpMyAdmin or PhpPgAdmin, verify in Joomla database tables that none of the table columns of SQL type text or in case of MySQL also mediumtext has a default value.

Result: No database table columns of SQL type text or in case of MySQL also mediumtext with a default value.

Expected result

No database table columns of SQL type text or in case of MySQL also mediumtext has a default value.
No SQL error when creating an admin module as descibed in the testing instructions of PR Removed all default values for type 'TEXT' #17860 .
No SQL errors related to invalid default values for columns of type text or in case of MySQL also mediumtext in other cases.

Actual result

There are database table columns of SQL type text or in case of MySQL also mediumtext which have a default value.
You might get SQL error when creating an admin module as descibed in the testing instructions of PR Removed all default values for type 'TEXT' #17860 .
Depending on your Joomla version you might such SQL errors in other cases, too.

Documentation Changes Required

None.

Continuing PR joomla#17860

richard67 · 2020-02-15T15:07:19Z

@alikon Ready for test.

richard67 · 2020-02-15T15:17:58Z

@SharkyKZ @Quy @jwaisner and whoever else has time: Please test ... the more testers for MySQL and PostgreSQL. the better. I really want to be sure that it doesn't break anything.

brianteeman · 2020-02-15T16:28:01Z

Why are you changing existing sql update files? they should be immutable

richard67 · 2020-02-15T16:30:51Z

@brianteeman No they aren't. We have to do that in the way I did in order not to let the database checker/fixer report errors. I had explained that in past, and I also explained in a comment here: #17860 (comment). Trust me, I know what I'm doing here.

richard67 · 2020-02-15T16:32:58Z

@rdeutz @wilsonge @HLeithner If one of you has time: Please confirm my comment above. It has been explained to Brian several times but he doesn't want to understand it.

brianteeman · 2020-02-15T16:37:21Z

You are making the changes required in this file 3.9.16-2020-02-15.sql so why would you need to make them in a previous file

richard67 · 2020-02-15T16:40:04Z

You are making the changes required in this file 3.9.16-2020-02-15.sql so why would you need to make them in a previous file

@brianteeman Because the database checker/fixer will report errors for these old files if I don't change them. Then you fix it, and then in the next step it will repost errors for the same columns of the new update sql, and if you fix them it will again report errors for the old script and so on and so on, so you end in an endless fixing loop.

P.S.: The checker goes through all present update sql files from old version to new version and checks if column definition in database matches what he finds in that particular sql file. It always starts e.g. with the 2.5 file, if present. So you can do a change for a column only in 1 file. If you later have to change the same column in a later file, you either have to comment out the change in the old file, or adapt it if it is in a create table statement.

richard67 · 2020-02-15T16:43:55Z

@brianteeman P.P.S.: If we would throw away the (almost) useless db ckecker and fixer it would all as you say.

brianteeman · 2020-02-15T16:48:33Z

I am sure you are wrong because the only time these changes will be seen is after you have upgraded the site and therefore already have the correct database.

Anyway this PR cannot be truly tested without testing an upgrade using the joomlaupdate component

richard67 · 2020-02-15T16:50:43Z

@brianteeman You are wrong, I think. Asfar as I know, the db checker "Extensions -> Manage -> Database" checks all the old files again, regardless if after new installation or after an update.

P.S.: But of course I can build an update container and provide a custom update URL to check updating with the Joomla Update Component. Will do so in a few minutes.

brianteeman · 2020-02-15T16:58:24Z

jwaisner · 2020-02-15T17:06:02Z

I have tested this item ✅ successfully on 993254e

New install works with no issues and database is up to date. All table updating functions work as well.

Update function works on an existent install then applying the PR. All table updating functions work as well.

This was tested on MySQL database.

richard67 · 2020-02-15T17:17:01Z

@jwaisner Thanks for testing. I have added a third test with Joomla Update Component. Could you test that, too, and report back the result?

richard67 · 2020-02-15T17:18:48Z

@brianteeman I have added a 3rd test for updating the with the Joomla Update component, as you suggested. Regarding what you show in your video: Maybe something is wrong on MySQL then with the db checker? On PostgreSQL you can see in my 2nd test that it is necessary to update the old scripts. Otherwise if not updated you would end in the endless fixer loop as I described before.

brianteeman · 2020-02-15T17:19:16Z

Steps to reproduce why the old updates do not need to be changed

Add just this file 3.9.16-2020-02-15.sql
Go to DB checker
It should look like this
Apply db fix
It will now look like this

So please show me why you need to change the old files

richard67 · 2020-02-15T17:24:49Z

@brianteeman Do following: On a clean staging apply from this PR only the changed files joomla.sql for nex installations and only add the new update SQL files 3.9.16-2020-02-15.sql. Then make a new installation. Then login to backend and check "Extensions -> Manage -> Database". It will show what happens when making a new installation with the joomla.sql modified by this PR and not having the old sql update files modified.

brianteeman · 2020-02-15T17:34:19Z

So I can now confirm the issue on new installs only and NOT on upgrades. To me thats an issue in the installer that should be fixed. You shouldn't even have the update sql files on a new install.

richard67 · 2020-02-15T17:36:40Z

So I can now confirm the issue on new installs only and NOT on upgrades. To me thats an issue in the installer that should be fixed. You shouldn't even have the update sql files on a new install.

Agree, but thats a larger, conceptional issue not touched by this PR. I agree with you about how it should be. And I hope I ever have the time and the courage to fix that.

@brianteeman and @jwaisner Please waith with testing the 3rd test with the update component .. I have to fix the xml so Joomla Update finds an update.

brianteeman · 2020-02-15T17:40:24Z

I stand by my comment that old update sql files should be immutable. The fact you are changing them is because of a bug elsewhere. Fix the problem, don't use a band aid.

richard67 · 2020-02-15T17:40:27Z

@jwaisner @brianteeman Update custom URL XML is fixed, you can text updating from any 3.9 version, but not from current staging or nightly because these already have same version as my update container. But you can test with any 3.9.x

richard67 · 2020-02-15T18:19:15Z

I stand by my comment that old update sql files should be immutable. The fact you are changing them is because of a bug elsewhere. Fix the problem, don't use a band aid.

@brianteeman I promise you now to fix that during our both lifetime, but it's not a small thing so it really needs a bit time, and the issue solved with this PR (and the preceeding one) has to be fixed before.

chmst · 2020-02-15T20:57:07Z

I have tested this item ✅ successfully on 993254e

Tested only MySQL.

Following exactly the test instructions, everything works as described.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27937.}

HLeithner · 2020-02-15T21:31:59Z

Since our db fix can't handle it correctly we don't have another option.

alikon · 2020-02-16T08:29:38Z

I have tested this item ✅ successfully on 993254e

tested on postgresql 11.5

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27937.}

alikon · 2020-02-16T08:30:22Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27937.}

alikon · 2020-02-16T08:35:25Z

the main point here is:

to remove wrong DDL a field type like TEXT cannot have default

plus we need to do the same on J4 as an example
https://github.com/joomla/joomla-cms/blob/4.0-dev/installation/sql/mysql/joomla.sql#L747

richard67 · 2020-02-16T09:42:03Z

@brianteeman One more explanation for you, not because I want to argue who is right or wrong but because I want you and other readers understand. Regarding your gif: The db checker checks only database stuff which appears in any update sql script. Furthermore, for create table statements or create index it doesn't check the table or index details, it checks only if the table or index with that name exists or not. That means when you change in db the type of a table column which doesn't appear in any update sql script in any "alter table ... modify ..." or "alter table ... change ..." statement, the db checker will not notice it. I think this is what your animated gif shows. This just for your and others' understanding.

richard67 · 2020-02-16T09:45:06Z

plus we need to do the same on J4 as an example
https://github.com/joomla/joomla-cms/blob/4.0-dev/installation/sql/mysql/joomla.sql#L747

Yes, this here has to be merged up into 3.10-dev and 4.0-dev after merged into staging, and it might have to be completed in J4 then by handling columns having been added in J4. I will keep an eye on it and if no bus hits me before will do that work then.

richard67 · 2020-02-16T10:12:02Z

Important for maintainers: When merging, please don't do a squash commit. I don't want the commit history and so the credits for the author of the original PR being lost.

richard67 · 2020-02-17T21:14:07Z

Last commit was only a grammar change in a comment, so reviews and tests and RTC should still be valid.

wilsonge · 2020-02-18T11:02:41Z

I think we all agree the db fixer tool needs to be adapted to an approach that makes it immutable. But given we don't have that agree modification the old files is an unfortunate reality :(

eXsiLe95 and others added 8 commits September 4, 2017 10:26

Removed all default values for type 'TEXT'

170e10a

Removed all defaults for SQL types text from update files

914283e

Removed default value for SQL type mediumtext

9894caa

Merge branch 'staging' into invalid-sql-joomla3

32d5f59

Merge eXsiLe95:invalid-sql-joomla3 into richard67:staging-redo-pr-17860

ce9346d

Continuing PR joomla#17860

Fix joomla.sql

c129993

Add missing new update sql scripts

d88af31

Fix old schema updates for postgresql

993254e

joomla-cms-bot added the PR-staging label Feb 15, 2020

richard67 mentioned this pull request Feb 15, 2020

Removed all default values for type 'TEXT' #17860

Closed

richard67 marked this pull request as ready for review February 15, 2020 14:53

richard67 requested a review from HLeithner February 15, 2020 15:33

joomla-cms-bot removed the PR-staging label Feb 16, 2020

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 16, 2020

alikon self-requested a review February 16, 2020 08:42

alikon approved these changes Feb 16, 2020

View reviewed changes

joomla-cms-bot added the PR-staging label Feb 16, 2020

wilsonge approved these changes Feb 17, 2020

View reviewed changes

Corrected English grammar in comment

4583899

wilsonge merged commit 90736a8 into joomla:staging Feb 18, 2020

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 18, 2020

wilsonge added this to the Joomla! 3.9.16 milestone Feb 18, 2020

richard67 deleted the staging-redo-pr-17860 branch February 18, 2020 11:26

This was referenced Aug 8, 2021

[4.0] Mail template API fails creating mail templates #35071

Closed

[4.0] Fix mail template API failing with SQL error when creating mail templates programmatically #35074

Merged

richard67 mentioned this pull request Nov 5, 2024

[5.2] Error on saving nested Subform #44168

Closed

richard67 mentioned this pull request Nov 13, 2024

Can not save transition of workflow for extension without workflow plugins #44447

Open

Uh oh!

Conversation

richard67 commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Test 1 - New installation

Test 2 - Update SQL scripts

Test 3 - Update to patched nightly build of today plus this PR applied

Expected result

Actual result

Documentation Changes Required

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

jwaisner commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

brianteeman commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

richard67 commented Feb 15, 2020

Uh oh!

chmst commented Feb 15, 2020

Uh oh!

HLeithner commented Feb 15, 2020

Uh oh!

alikon commented Feb 16, 2020

Uh oh!

alikon commented Feb 16, 2020

Uh oh!

alikon commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Feb 16, 2020

Uh oh!

richard67 commented Feb 16, 2020

Uh oh!

richard67 commented Feb 17, 2020

Uh oh!

wilsonge commented Feb 18, 2020

richard67 commented Feb 15, 2020 •

edited

Loading

richard67 commented Feb 15, 2020 •

edited

Loading

richard67 commented Feb 15, 2020 •

edited

Loading

jwaisner commented Feb 15, 2020 •

edited

Loading

richard67 commented Feb 15, 2020 •

edited

Loading

alikon commented Feb 16, 2020 •

edited

Loading

richard67 commented Feb 16, 2020 •

edited

Loading