@wilsonge Do you have an idea why the unit tests fail? It seems it is related to the statements "$queries1 = $this->db->splitSql($fileContents1);" and "$queries2 = $this->db->splitSql($fileContents2);".

Can it be because file contents are empty (should not be the case)?

Or is it because I initialized the variables "$queries1" and "$queries2" with "$queries1 = array();" and $queries1 = array();" before the statements mentioned above?

The database driver is mocked in this test case. JDatabaseDriver::splitSql() is a static method and cannot be mocked. So to make the test work with this change, essentially JSchemaChangesetTest would have to be rewritten as 5 test classes (one per driver) extending from the various TestCaseDatabase<Driver> classes so that JFactory::$database has a reference to a real JDatabaseDriver instead of a mock.

@mbabker Oh my god, I am absolutely not familiar with the unit tests. Would it maybe be possible and easier just to add the splitSql function to the mock? it is same for all drivers in driver.

Can't. A static method cannot be mocked.

@mbabker I mean implement it locally in the mock.

The unit test case mocks JDatabaseDriver. PHPUnit cannot mock any method declared static. So unless you're going to duplicate and inline JDatabaseDriver::splitSql() in an OOP manner in JSchemaChangeset you can't work around that behavior. If this change requires that JDatabaseDriver::splitSql() is used then the test case for JSchemaChangeset has to be rewritten in a way that uses a real JDatabaseDriver object, not a mock.

@mbabker Should thast be part of this PR here? Or could we test that and reqite the unit test's mock driver with another one? And if so, could someone else than me do that or at leats advise me?

If this PR merges with the unit tests failing then it's going to cause a chain reaction until the tests are rewritten. So no, this shouldn't be merged in a failing state for any reason short of a 0-day security issue.

@wilsonge @mbabker Can someone advise me further with that unit test stuff? Or make a PR against my repo/branch for this PR here? I need the splitSql there to get the checksums of the statements (without comments) from the utf8mb4 conversion scripts when initializing the check query in the changeset for the schema manager so the check can see if conversion to be done or not.

An alternative would be to checksum the complete file content, but this would include also changes in comments only when there are no changes in the SQL statements themselves.

I can deal with the tests. But I don't want to go through the hassle unless this is going to be the way forward. Rarely if ever should changes require massively rewriting a test class, unfortunately you refactored JSchemaChangeset in a way that totally breaks the existing setup (not your fault either just to be clear, it's one of those technology problems).

@mbabker

But I don't want to go through the hassle unless this is going to be the way forward.

Well, if you read the summary of changes and the test instructions, it should show that it is the way forward if it works, and this can be found out by testing.

I get that part. But one issue has evolved into at least 3 PRs so far which have all changed just a little bit. So if this is "the way" then we're good and I can get the tests updated. I'm just a little hesitant at first to put time in something that's been a moving target.

@mbabker No, this one should solve all issues with having problems to use the database fix, and I have combined this together with the one to update conversion by the missing stuff because it helps with testing (would have needed some changes anyway). So now just updating to 3.5.1 would solve the issues if this PR is in 3.5.1, regardless if Update Component of file copy + database fixer method is used.

The only issues which are not solved by this pr are such (mainly reported for com_finder) where duplicate finder terms have caused problems, which then could be fixed with drop and rebuild of the index.

This is a completely different thing and so not makes sense to be solved with this PR.

When this PR is merged, the structure of the utf8_conersion table is ready to handle extensions, too.

What then remains to be done is to make extensions add 2 attributes "utf8mb4conversion1" and "utf8mb4conversion2" (or so) to their XMLs, and make the extension installer handle for these 2 scrips conversion in the same way as we already do for the core, and voila: All done.

But also this is a future PR then, not needed for 3.5.1 urgently.

Tis one here is needed for 3.5.1 if it solves what it promises to solve.

Added Steps 11 and 12 to verify that after a new installation, the conversion status and checksums are correctly set in database.

@mbabker Another was to solve the unit tests would be to move this creating of the special change item to check for done conversion to somewhere outside the changeset.php.

But this would need to be done when the changeset is newly created, so the item can be appended to the end before the check query is executed. If you have any idea where this could be done, let me know. I will check meanwhile what I can find out.

@mbabker I could build the query outside of changeset.php wherever the changeset is instantiated, and pass it as new parameter to the constructor and the getInstance functions of the changeset, so the changeset appends just this query string to the end in the constructor. For the unit tests we would use an empty string as parameter so the check function will not be added, and later with a future PR extend that then by pasing the real thing in the unit tests, too.

I wouldn't change it like that. If the intent is for the schema change
checking API to detect this stuff, you shouldn't be passing in things it is
supposed to be checking (otherwise it kind of defeats the point).

On Friday, March 25, 2016, Richard Fath notifications@github.com wrote:

@mbabker https://github.com/mbabker I could build the query outside of
changeset.php wherever the changeset is instantiated, and pass it as new
parameter to the constructor and the getInstance functions of the
changeset, so the changeset appends just this query string to the end in
the constructor. For the unit tests we would use an empty string as
parameter so the check function will not be added, and later with a future

PR extend that then by pasing the real thing in the unit tests, too.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/9590
https://issues.joomla.org/tracker/joomla-cms/9590.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9590 (comment)

No, I do not pass in things to it which it shall check. I only pass in the check query, which is built outside. But still the schema change api will process the check query.

shrug I still say the schema management layers need to me moved to
/dev/null and recreated but as with so much of Joomla, if the hack works...

On Friday, March 25, 2016, Richard Fath notifications@github.com wrote:

Bo, I do not pass in things to it which it shall check. I only pass in the
check query, which is built outside. But still the schema change api will
process the check query.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9590 (comment)

@mbabker I fully agree.

One thing with which you maybe can help me:

Should I add "@since 3.5.1" tags to the new parameters for the constructor and the getInstance of the changeset? I have not quickly found now an example on how to correctly use "@since" tags for single function parameters.

Fuck, I hope nobody has the nick name "since" on GitHub. Sorry for disturbing, if so.

Please do not test yet, I have to fix 1 little thing. The test with detecting a change in the conversion sql statements will not work yet.

I have to roll back commit c0c6470, and so the unit tests will fail. Have to find another solution for that.

please don't disturb SInCE ... https://github.com/since

Still not ready for testing, please be patient.

@mbabker @wilsonge I tried a few things but could not find a way to get the thing work without making the check query in the constructor of the changeset. So I rolled back the pervious change for this, and now again have the problem with the unit test.

Can anyone help?

@mbabker @wilsonge P.S.: When I tested here, it did all work in the way which I restored now, only the unit tests fail now again.

Give me a little bit and I'll get the tests converted over.

Oh, I give you as much as you need, and cannot say how thankful I am.

I know it's Easter, so I will be patient.

Have a nice Easter fest.

ok, but his can be tested anyway right?

Yes.

@andrepereiradasilva Hmm, no, wait a bit please. Seems I have to fix something.

@andrepereiradasilva @mbabker Now it is ready for testing.

So, now this should have been the last commit (except later for the unit tests). Was working already, just had to change an important misleading comment.

Ready for testing now, definitely, just tested everything myself againwith success.

ok will test whne i have time

Thanks in advance.

Added Tests for detecting changes in the 2nd conversion script.

Added test for updating with Joomla! Update component.

I had to add also an update component test to the testing instructions to make sure all changes made with this PR are tested.

Now it is (except of the unit test) complete.

Sorry, last change for now.

Corrected testing instructions: Update test must be done with a 3.5.0 to proof the conversionwill be done when updaing, even if it was done before (with a pre-3.5.0 this cannot be tested because no conversion done before).

TEST 1 done with success on a clean staging install

only difference in step 2 results got and extra line.

But all ok.

Will now reset everything for test 2.

@andrepereiradasilva That fits to the testing instructions. The top line is always added.

But I noticed that Test 2 with updating does not work yet. After update, the database fixer shows the utf8mb4 conversion to be done. I try to reproduce this on another test system I have, but maybe I have to add something to script.php to update the conversion status table to the new structure, similar as I did in database.php.

so i skip to test 3? or stop the test?

You can do Test 3, that one should work, but when I have to do a commit, any test result will be reset anyway. So maybe better wait if you do not want to waste time.

ok i will wait

Added details about the update zip to Test 3.

@andrepereiradasilva Now you can test. The recent commit did only change script.php, which is only relevant for Test 2, and I already updated the update zip container behind the custom URL, so you can continue to test.

Test 3 success

i can't make test 2 when i put your custom URL the updater doesn't find an update:

You are on the "Custom URL" update channel. This is not an official Joomla! update channel.
You already have the latest Joomla version, 3.5.1-dev.

@andrepereiradasilva Well if you used the same system for the test before, it will not work. You have to fall back to 3.5.0 before.

ok ...

Is also the subject of the test, that a 3.5.0 can be updated to this, because on a staging + this patch, there will not be any changes anymore to do for the conversion.

On a clean 3.5.0 using the nightly build update package for 3.5.1-dev, the update would succeed, but the changes in the conversion scripts (e.g. TABLE #__menu, KEY idx_path limit to 100) would not have been applied.

With my patched package behind my custom URL it will also end with success, but the conversion has been executed again at the end of the update, and so the changes have been applied.

That's why testing instructions for Test 2 sais "Update a Joomla! 3.5.0 to ...".

ups, didn't read properly

Test 2 ok too.

I have tested this item successfully on 3ba979d

Excluding the unit tests followd test instructions and all worked as described.

Test server:

• Database Version: 5.5.5-10.0.24-MariaDB
• Database Collation: utf8_general_ci
• Database Connection Collation: utf8mb4_general_ci
• PHP Version: 7.0.4
• Web Server: nginx/1.9.12

  ---

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

BTW, not related to the PR, but noticed now in upgrading from 3.5.0 to your 3.5.1 that the updater look is way better!

@andrepereiradasilva Super, the tests look really good, also here

Now only the unit tests have to be fixed.

And maybe com_finder issues with duplicate search terms in another PR (there were still issues in the forum reported, making the term column binary sorted could maybe help).

Ah, and my other PR #9504 with the bugfix for the query downgrade would also be a good thing, even if it does not solve a current issue, but will make things safer.

Then we solved most of the issues seen after updating to 3.5.0., except maybe server related stuff like timeouts too short.

So 3.5.1 could come then.

And then another PR making the core doing the same for the extensions, check and if necessary run conversion scripts (would be specified with 2 attibutes, one for each script, in the extension's XML), and then all is done for 3.5.2 (if not ready for 3.5.1).

My other PR for having scripts for different charsets for the extensions, which you used for your PR for the localise component, those would be not needed anymore then. It would be done like for the core then.

Ah, what did you mean with the updater looking better? I did not see big differences on the Joomla! Update component's page, but maybe was too fast, and now as all my test systems are on this 3.5.1-dev I cannot just have a look. Or did you mean extensions updater?

More precise instructions regarding start conditions for Tests 1 and 2.

@richard67 did you test from an older version (ex: 2.5.28) upgrad too? i can't test this old upgrades now since i'm on php 7.

And maybe com_finder issues with duplicate search terms in another PR (there were still issues in the forum reported, making the term column binary sorted could maybe help).

Regarding com_finder, maybe another PR is needed, also those tables are still in general_ci. But IMHO that can go for 3.5.2.

Ah, and my other PR #9504 with the bugfix for the query downgrade would also be a good thing, even if it does not solve a current issue, but will make things safer.

That one is still for test? Doesn't conflict with this one? Can be safely tested too? Probably i will not have time to test that until monday.

And then another PR making the core doing the same for the extensions, check and if necessary run conversion scripts (would be specified with 2 attibutes, one for each script, in the extension's XML), and then all is done for 3.5.2 (if not ready for 3.5.1).

Ok. Will wait for that PR, but IMHO it's not for 3.5.1.

My other PR for having scripts for different charsets for the extensions, which you used for your PR for the localise component, those would be not needed anymore then. It would be done like for the core then.

Do you mean this one? #9468
If it's not needed please close it. It's RTC...

When all utf8mb4 stuff is working fine we look at the com_localize PoC. Until them it stays on hold.

Ah, what did you mean with the updater looking better? I did not see big differences on the Joomla! Update component's page, but maybe was too fast, and now as all my test systems are on this 3.5.1-dev I cannot just have a look. Or did you mean extensions updater?

The progress bar has a better look.

@andrepereiradasilva No, I did not test upgrading older versions. But it should work, too, as it did before.

Regarding #9504 : Yes, that one maybe could have conflicts after this one here is merged (or applied with patch tester). But as it is now it should be testable.

Regarding #9468 : I will close that right now, thanks for the hint.

@andrepereiradasilva Just right now I finished the worst case test with success.

Updated a clean 2.5.28 with testing data on a database not supporting utf8mb4 using my custom URL: Worked like a charm, zero errors or warnings, all new and running smooth from beginning on.

Then played back the database with the data of the clean 2.5.28. Result: Looked like a mess and had SQL error not related to this PR but to new templaes and extensions not being installed. So I installed all discovered extensions, and then used the database Fix button to solve reported database problems. I had to use it 2 times because after 1st problems were solved, some were remaining, but this can happen with this method on an old 2.5.28, and none of the problems was related to the utf8mb4 changes, only to the schema changes in past, and there were no further SQL errors shown. Then set Isis as default template, and voila: Also all fine.

In both cases, I successfully checked with phpMyAdmin that conversion to utf8_unicode has been made for all core tables except of the finder tables.

So it seems this PR here works (except of the unit test not working yet).

This PR has received new commits.

CC: @andrepereiradasilva

I have tested this item successfully on 6c36012

the latest change has just resolve conflicts.

See richard67#4

@mbabker Wow, am really impressed. Seems you like "WRIWYG" (what you read is what you get): It is named refactoring, and it is a refactoring. 23 changed files. Quiet a lot of work. Sorry for having caused this.

When you spend time with the guy who wrote PHPUnit you learn a thing or five about decent test structures

This PR has received new commits.

CC: @andrepereiradasilva

I have tested this item successfully on 50a139a

only test units changed. So no need to test again.

great work @mbabker!

@lal12 I ping you because you had the issue #9526 .

Do you have time to test this PR here?

If so, do you have a backup of your database where you had the problem, either before update, or after, and have a test Joomla! installation with an own database for testing purposes?

If this is the case, you can skip the testing instructions and instead of this test one or both of the following possbile scenarios, depending on what all you have available as backup.

The below tests would also be very welcome from any other user who had problems with SQL errors or not fixable database problems after updating to 3.5.0.

If this works for you, you can mark the test result as success in the issue tracker here: https://issues.joomla.org/tracker/joomla-cms/9590, using the "Test this" button on the top left area of the page.

But you should do these tests not on your production site, only on a test system!

Scenario 1

You have meanwhile a Joomla! 3.5.0 test system which is running well, and have a database dump (either before or after update) of the database (test or real system) where you had the problems.

In this case

1. Apply the patch for this Pull Request either with patchtester component, if you have that, or by fetching the files added or changed by this PR.
2. Save your good test database so you can import this back later in case of probems.
3. Delete all tables in phpMyAdmin and then import the dump of the db which made problems.
4. Login at the backend (admin area) and go to "Extensions -> Manager -> Database".
5. Depending on what the status of the omported database was (pre or post update) you see more or less database problems to be fixed, and at the end as last one the undone UTF8-Multibyte or UTF8 conversion.
6. Use the "Fix" button, if necessary 2 or 3 times, until all database problems are fixed.

The result should look then as shown below the next scenario.

Scenario 2

You do not only have a backup of the old database but also of the old file system of the Joomla! where you had the problems, both from before the update.

In this you can simply

1. Save your good test database so you can import this back later in case of probems.
2. Delete all tables in phpMyAdmin and then import the dump of the db which made problems.
3. Rename the Joomla root folder of your test system so you can later use it again after the test, if you want.
4. Restore the backuped file system from the Joomla! where you had the problems to a folder named like the joomla root of your test system you just renamed before, and copy the configuration.php from tha backuped test system to the new one so it looks at the test database.
5. Update this system using following custom URL for the Joomla! Update component: http://test5.richard-fath.de/list_test1.xml.
6. Go to "Extensions -> Manager -> Database".

Result: See below.

Result at the end in both cases

Database table structure is up to date.
Other Information

• Database schema version (in #__schemas): 3.5.1-2016-03-25.
• Update version (in #__extensions): 3.5.0.
• Database driver: mysqli.
• 97 database changes were checked successfully.
• 145 database changes did not alter table structure and were skipped.

This PR has received new commits.

CC: @andrepereiradasilva

I have tested this item successfully on f72fc57

the latest change has just remove a duplicated query in the utf8mb4 conversion script.

@andrepereiradasilva It may happen that I will replace this PR later tonight by a new one, which does only the necessary parts of this one to fix update errors, but not the automatic conversion reprocessing, because this introduces changes maybe not be usable for a final good solution when also handling conversion of extensions. Have to wait for @wilsonge feedback.

Closing in favour of PR #9652 .

@andrepereiradasilva Sorry for the additional work. Could you test the new one, too? Testing is simpler than it was here.

@mbabker Sorry if I made you work for nothing with reengineering the unit tests for this PR. If you think we should keep your work, then let me know and I will not delete the branch of this PR so we can use it then, and your work will not be lost. @wilsonge Please check that, too, and let me know if I shall keep my branch.

I think you need to keep it. A lot of this is useful and should be merged. Just some the "easy" fixes needs to come into 3.5.1 and the rest kept back for 3.5.2 with further evaluation :)

We could keep Michael's changes, but the other stuff is either in my new PR, or I would do it differently in a new one. The approach here was not good.

Michael's test fixes should be ported across into core

@wilsonge just cherry pick ff136d6 into core for the tests, it should be fine without this PR anyway. It actually makes the tests a bit more structurally sound here and gives us a chance to ID what parts of the overall API don't have explicit test coverage (only a small part of it is explicitly tested and some of that with mocked behavior and some dependent on the core schemas).

Merged

Hmm it seems Michael's changes for making the tests use real db drivers now make unit tests fail with error on sending data from the statisctics plugin, see my still open PR #9504 , which I just rebased.

That looked more like a Travis failure than a Joomla failure (especially since it was only one job out of the set that failed). I retriggered it.

Ok, thanks.

Please stop to use any of the links I provided here for testing purpose. They will not work anymore.