Skip to content

Conversation

@richard67
Copy link
Member

@richard67 richard67 commented Jul 20, 2019

Pull Request for Issue #25452 for staging.

Summary of Changes

Fix case in column type check of database schema checker for MySQL 8.

When you enter e.g. the SQL statement

SHOW COLUMNS IN #__extensions WHERE field = 'enabled' AND type = 'TINYINT(3)';

(using your db prefix of course and not #__)

in PhpMyAdmin on a Joomla 3.9.10 or staging database, you will get a result in MySQL 5.7 but not in MySQL 8.

But when you enter

SHOW COLUMNS IN #__extensions WHERE field = 'enabled' AND UPPER(type) = 'TINYINT(3)';

you get a result on both MySQL versions.

In both cases type names are lowercase in the result, but on MySQL 8 the search seems to be case-sensitive, while it is case-insensitive e.g. on MySQL 5.7.

Testing Instructions

  1. Install current staging using a MySQL db of version 8.0.something. After successsful finish, login on backend and go to "Extensions -> Manage -> Database".

Result: See section "Actual result" below.

  1. Apply this patch, navigate to somwhere else in backend and then back to "Extensions -> Manage -> Database".

Result: See section "Expected result" below.

Expected result

No database errors.

Actual result

Database errors shown as described in issue #25452 , but when you check in the database and the schema update SQL mentioned in the error message, you will not see any reason because the data types match in all details those set by the schema update and also those in joomla.sql.

Documentation Changes Required

None.

Fix case in column type check of database schema checker for MySQL 8.
@richard67 richard67 changed the title Update MysqlChangeItem.php Fix database schema checker for MySQL 8 on staging branch Jul 20, 2019
@richard67
Copy link
Member Author

@wilsonge Question: Shall I do a PR for 4.0-dev, too, or shall we wait until this is merged and then 3.9.11 will be merged to 4.0-dev?

@richard67
Copy link
Member Author

@MonkeyTrainer12 @breisig Please test.

@ReLater Please spread in forum people shall test this PR.

@mbabker Please review or at least notice desciption above about different behavior of "SHOW COLUMNS".

@richard67
Copy link
Member Author

Drone failure is javascript and so not related to this PR.

@ReLater
Copy link
Contributor

ReLater commented Jul 20, 2019

@ReLater Please spread in forum people shall test this PR.

Done. Let's see how lazy they are.

And thank you for your efforts. I "bombed" my LInux with upgrading to MySQL 8 ;-)

@richard67
Copy link
Member Author

yes i had struggle too

thanks god i bombed only a clone vm ;-)

@richard67
Copy link
Member Author

Sorry, had forgotten most important part. Fixed now.

@richard67
Copy link
Member Author

Grr, again mistake. Now it is really correct.

@breisig
Copy link

breisig commented Jul 20, 2019

Will test out the patch very soon.

@richard67
Copy link
Member Author

@breisig Did you already find time to test? If so, please mark your test result in the Joomla CMS issue tracker at https://issues.joomla.org/tracker/joomla-cms/25658. Just use the button "Test this", then select your test result and then submitt that with the corresponding button.

@frostmakk
Copy link
Contributor

frostmakk commented Jul 21, 2019

I have tested this item ✅ successfully on 30cd879

Tested on localhost wamp J 3.9.10 , PHP 7.3.7 , MySQL 8.0.15


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25658.

@richard67
Copy link
Member Author

@MonkeyTrainer12 @breisig One more tester only needed. Thanks @frostmakk for testing.

@breisig
Copy link

breisig commented Jul 22, 2019

@richard67 It looks like it works.

@ghost
Copy link

ghost commented Jul 22, 2019

@breisig please mark your test as successfully (how to: https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results)

@richard67
Copy link
Member Author

@MonkeyTrainer12 We still need one more tester because @breisig is either not willing or not able to mark his test result in the issue tracker like described e.g. here #25658 (comment) or here https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results .

@richard67
Copy link
Member Author

@breisig Thanks for your test, but please also mark your test result in the Joomla CMS issue tracker at https://issues.joomla.org/tracker/joomla-cms/25658. Just use the button "Test this", then select your test result and then submitt that with the corresponding button.

As long as not marked, it is not counted, and as long as no 2 test, this PR will not be ready for merge.

@breisig
Copy link

breisig commented Jul 25, 2019

I have tested this item ✅ successfully on 30cd879

It worked for me.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25658.

@Quy
Copy link
Contributor

Quy commented Jul 26, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25658.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 26, 2019
@wilsonge wilsonge merged commit 45e4304 into joomla:staging Jul 26, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 26, 2019
@wilsonge wilsonge added this to the Joomla! 3.9.11 milestone Jul 26, 2019
@richard67 richard67 deleted the staging-fix-db-schema-check-type-case branch July 26, 2019 10:56
@richard67
Copy link
Member Author

Thanks to all testers and to reporters for your patience, and thanks for merging of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants