Corrected typo in testing instructions, code snippet for test 5 used $sql instead of $buffer in call to splitSql.

Please find me some testers.

Not every tester has to perform all tests, but then he/she should not in coment to test result what was tested and what not, e.g. "Tested all except Test 5" or so.

@richard67 I am happy to test this afternoon if it hasn't been completed.

Great, thanks in advance.

I have tested this item unsuccessfully on 46fdca1

Having a bit of a run around here.

Test 1 Pass

Test 2 Problem - Joomla 2.5.28 upgrade to J3.5-richard-version
With this test, there seems to be issues with saving items, the examples I can use are installed kunena but the save and save and close wouldn't work when creating a module. Also having installed the patch tester entered my github parameters and save failed to work.
Test 2 Pass Joomla 3.4.8 upgrade to J3.5-richard-version

Test 3 I am getting an error when importing the database error is below

"CREATE TABLE h057e_assets (
id int(10) UNSIGNED NOT NULL COMMENT 'Primary Key',
parent_id int(11) NOT NULL DEFAULT '0'COMMENT AS Nested set parent.,
lft int(11) NOT NULL DEFAULT '0'COMMENT AS Nested set lft.,
rgt int(11) NOT NULL DEFAULT '0'COMMENT AS Nested set rgt.,
level int(10) UNSIGNED NOT NULL COMMENT 'The cached level in the nested tree.',
name varchar(50) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'The unique name for the asset.\n',
title varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'The descriptive title for the asset.',
rules varchar(5120) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'JSON encoded access control.'
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
MySQL said: Documentation

#1064 - You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS Nested set parent.,
lft int(11) NOT NULL DEFAULT '0'COMMENT AS `Nested ' at line 3"

Test 4 Pass Joomla 2.5 upgraded and 3.4 upgraded (the table is inserted correctly both times)

Test 5 Joomla 3.4.8 upgrade to J3.5-richard-version
I had to install bug tester and install patch #9369 in order to get it to work I didn't expect to need to do this.

If you would like me to run any test again please let me know

Thanks for testing, I'll have a look on it when I have time, later tonight or tomorrow.

@Twincarb Did you upgrade with Joomla! Update component as descibed? Or did you use the extension installer to ugrade Joomla!? In this case please try again with Joomla! Update component.

@richard67 I used the Joomla Update component each time, I am just going to do a job in the garage then I will take another look as well.

@Twincarb How did you import your database? When I use my test code snippet from Test 5 to show the content of installation/sql/mysql/joomla.sql from a 2.5 (where the defaults where '0' and not 0), the statement for creating the assets table is shown by moy code snipped as follows:

CREATE TABLE IF NOT EXISTS #__assets ( id int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'Primary Key', parent_id int(11) NOT NULL DEFAULT '0' COMMENT 'Nested set parent.', lft int(11) NOT NULL DEFAULT '0' COMMENT 'Nested set lft.', rgt int(11) NOT NULL DEFAULT '0' COMMENT 'Nested set rgt.', level int(10) unsigned NOT NULL COMMENT 'The cached level in the nested tree.', name varchar(50) NOT NULL COMMENT 'The unique name for the asset.\n', title varchar(100) NOT NULL COMMENT 'The descriptive title for the asset.', rules varchar(5120) NOT NULL COMMENT 'JSON encoded access control.', PRIMARY KEY (id), UNIQUE KEY idx_asset_name (name), KEY idx_lft_rgt (lft,rgt), KEY idx_parent_id (parent_id) ) DEFAULT CHARSET=utf8;

So as you can see there is not this syntax error with "... DEFAULT '0'COMMENT ...".

How did you export and import your pre-updated 2.5 database? Did you run the joomla.sql? Or did you export / import with phpmyadmin? Or Akeba backup? Maybe they have a problem with old 2.5?

I cannot replicate this here up to now.

@Twincarb If you do the same test for updating with an unpatched 3.5.0 RC. Do you have the same problems? Can you check this? If so, they are not relatef tho this PR.

@Twincarb Hmm, I've just processed the complete joomla.sql of a 2.5.28 with the patched splitSql function as shown in my test code snippet for Test 5 for the test scripts. Then I copied the complete sql from my frontpage into phpmyadmin and run the SQL. It all run without any error, and at the end all core tables of a 2.5.28 were created. So I have no idea what happened in your test. Please try what you did again with unpatched 3.5.0 RC and let me know if ok or same errors. Thanks in advance.

@richard67 Doing a test upgrade to 3.5rc was my next plan, although I am not sure where to find the xml file for it.

With the sql, I did a quick export in phpmyadmin and then used the import to restore the dump. But it gave the error above

@Twincarb Hmm, I also have a problem to update 2.5.28 to 3.5.0 RC using testing channel. Get told I am already at max version.

I prepared a custom URL like for my patched version but with "3" instead of "1" in the file name: http://test5.richard-fath.de/list_test3_j25.xml. This points to an original 3.5.0 RC.

But @wilsonge do you know why 2.5 cannot be updated to 3.5.0 RC via testing channel of Joomla! Update Component? Mistake? Or by purpose?

@Twincarb The problem with syntax error in your export I cannot reproduce here. Did export from a 2.5.28 and it looks as it should, space between "DEFAULT '0'" and "COMMENT". Maybe this is a problem with your export/import?

What I can reproduce is that editing and saving modules does not work after updating the 2.5.28 to my patched 3.5.0 RC, so I want to test if this is also the case with unpatched 3.5.0 RC. I let you know my result, but if you find time, pleas test, too.

Thansk for your help.

It's purposeful because 2.5 and 3.x share the same testing channel. So when we were pushing 2.5 and 3.x releases together, we couldn't put the beta/RC for 2.5 to 3.x upgrades on that testing channel because that would override testing 2.5 to 2.5 updates.

Not as much of an issue now that 2.5's EOL I guess. In theory this change does it with the custom XML file we've got in the repo, or instead of the list_test_25to3x.xml change you change this line in the main testing channel's XML to use the 3.x releases.

@mbabker Well, meanwhile I helpe myself with a separate custom URL on my domain serving the 3.5.0 RF for 2.5.28, http://test5.richard-fath.de/list_test3_j25.xml. If you want and have time you could help with testing this PR here.

@Twincarb I tested with this URL and it serves the unpatched RC, and it has the same problem: Saving changes when editing modules does not work.

So this problem is not related to this PR here. Can you test and confirm?

@richard67 No worries will test with your xml there, I need to look into why I am not able to restore databases on my vps the only change on this server is I set db defaults to utf8mb4 , I have a second one rebuilding at the moment so will be able to test again when that is set up. In the meantime I will do the 2.5.28 upgrade using your xml.

Before upgrading the 2.5.28, export its db again and then check in the export file if for the very first table at the top, assets, you see "DEFAULT '0' COMMENT ..." (= ok) or "DEFAULT '0'COMMENT ..." (=error) for columns lft and rgt, and let me know the result. If it is wrong in your export, the problem is also not related to this PR. Thanks in advance.

@richard67 Just had a look at the sql dump, the first line is missing the space which should be in here "DEFAULT '0'COMMENT ...
the same applies to lft and rgt columns.

@Twincarb So it cannot be related to my PR here, if the sql dump of the 2.5.28 before the upgrade looks like this, right?

@richard67 I agree, it's something my end.

@Twincarb And the menu edit problem? Does it also happen with an unpatched 3.5.0. RC?

@richard67

Test 1 Pass
Test 2 Pass J3.4.8 - patched version
Test 2 Pass J2.5.28 - patched version
Test 3 Unable to run - will try again tomorrow with akebba backup sql dump
Test 4 Pass
Test 5 Pass

J3.5.0.RC menu works on a clean install
J3.4.8 + upgrade - menu works using your upgrade
J2.5.28 + upgrade - menu works following a forced refresh ctrl F5

I tested on 2 seperate vps's and have an sql dump issue on both

@Twincarb just to be sure I understand right: The sql dump issue is with phpmyadmin? or with akeeba?

Beside this it looks like a good test here.

Even if you cannot solve the sql dump problem I would say the test is a success because all the rest worked.

@Twincarb Ah, and could you change your test result in the issue tracker to "not tested" if you not want to mark it already as "success"?

The failed test could keep others from testing because they might think it dows not work and so makes no sense to test.

@richard67 the sql dump isn't working as expected in phpmyadmin.

I am happy to submit it as passed under "Test This" not sure what the normal protocols are. I have time tomorrow morning to see if an akeeba dump would work or if I would have the same issue.

I have tested this item successfully on 46fdca1

Tests passed successfully, unable to run test 3 due to unrelated issues.

@Twincarb Thanks for testing. The problems with step 3, did you have them with the db from pre-upgrade 2.5.x? or a 3.4.8? or both? If with 2.5 only, could you try it with 3.4.8?

@richard67
Test 3 Pass J3.4.8 upgraded then restored original db just needed to fix database as expected

Test 3 Pass J2.5.28 upgraded then restored original db needed to discover and install all found then set default template then fix database as expected.

Original issue I had was with both J2.5 and J3.4 I am unable to restore a db using phpMyAdmin whether exported from phpMyAdmin. An akeeba site backup then site restore works, I used mysqldump and restore with command line to complete the test.

@Twincarb Thank you very much for the detailled testing and information.

Looks good to me regarding my PR, but of course your bug with phpmyadmin sql dumps is annoying.
Maybe you can post your mysql db server and phpmyadmin version numbers, and also which client api with which version number?

Maybe some mysql experienced person will read it and can tell if he/she ever heard of such bugs with a specific version.

I am not very experienced with mysql, but I remember there were certain versions not to be used because buggy.

What is good to hear is that akeeba worked. Does it also work on the 3.5.0 RC + my patch (you called it J3.5-richard-version), do a complete site backup and later restore it? That would be good to hear.

@richard67 Not a problem testing, wasn't wanting to be beaten knowing there are several ways to work with the db.

Your build J3.5.RC +patch works correctly with akeeba backup, I just did a quick install and tested it.

mysql is 5.5.47
phpmyadmin is 4.5.5
mysql server is nginx 1.9.2
server build is centos 6.7
Database client version: libmysql - mysqlnd 5.0.11-dev - 20120503

@wilsonge As you can see by the previous comment, no problems with Akeeba or so when using my PR. I see no reason not to do it.

I have tested this item unsuccessfully on 46fdca1

Test failure. Fore core usage I can't find any issues however I do find issues when using MySQL c-style comments:

For example using an example here: http://dev.mysql.com/doc/refman/5.7/en/comments.html

$db = JFactory::getDbo();
$queries = $db->splitSql('CREATE /*!32302 TEMPORARY */ TABLE temp_table (a INT);');
foreach ($queries as $query)
{
    $db->setQuery($query);
    $db->execute();
    echo $query . ' has been executed';
}

By running this on staging and staging with your patch applied then I get two different results. In staging I get a temporary table created (as expected), with this patch applied I find a real table created.

Obviously this specific example isn't really going to be used by extensions but there might well be others that are. As I understand it's only comments of the format /*! */ that can be executed - so maybe we need a special case for that?

@wilsonge Solution will be to change this PR to not handle the c style comments at all, because this was a goodie I made which was not really needed.

We only need correct handling of "--" and "#" comments, which was not correct before because not checking if within quoted text or not.

C style never was supported, and as we see in your example they are abused by some sql implementations to specify omptimizer hints and other stuff. All this is not standard, but will be ok when I remove handling of c style comments from this PR.

I will prepare that and update description and test sql script.

This PR has received new commits.

CC: @Twincarb, @wilsonge

Update description and test instructions to latest change (not handling of c style comments), updated the update containers for thhe test with custom URLs and updated the test sql scripts to these changes.

@Twincarb @wilsonge Could you test again?

I have changed this PR not to handle c style comments anymore, so those coments which may contain relevant stuff e.g. like optimizer hints will not be stripped off.

The patched update container available via the custom update URLs is already updated.

Sorry guys, I am not 100% sure yet, but I think this PR is not ready yet.

I just saw our schema update files do contain c style comments, but only for single lines, e.g. at top of the file, like "/* Core 3.2 schema updates */" in 3.2.0.sql.

So I might have to deal with c style comments, too, either handle them completely like before the last commit and check if special comments which are not to be stripped off, like optimizer hints and so on, or only strip off c style comments for complete lines or so, so those with optimizer hints and so on will survive.

On the other hand, the c style comments also were stripped off with these old trimSql functions I have removed with this PR, and so the optimizer hints and other specials also would have been stripped off, so before the last commit this PR was not introducing a new mistake regarding these.

This PR has received new commits.

CC: @Twincarb, @wilsonge

Changed description and example sql script output back for c style comment handling, but with exceptions for mysql specials.

richard, a question for the future: wouldn't it be easy to remove the comments with regular expressions? see for instance http://stackoverflow.com/questions/7100127/regex-to-match-mysql-comments/13823363#answer-13823363 (didn't test)

@andrepereiradasilva I have thought about it and exactly had this stackoverflow answer in mind, but I remember from past discussions about comment strippings in other issues (with the "#" comments) that complex regexes are not so welcome for some of the maintainers, and I also find them not easy to maintain, even if well commented, and the solutoon from stackoverflow did not work as it was for our purpose.

And so I decided to do it based on the existing solution.

Regarding preformance I think the one is like the other ... the regex parser also has to loop and check backwards, so i think there is not a big difference in performance.

So at the end it was a matter of taste, and what could be better to understand for others.

PR is ready for test now by the way, with last commit I added back the handling of c style comments but with exceptions for the mysql specials.

ok thanks for the awnser

Would you have preferred the regex way, andre?

always prefer to write less code :)

I see .. well the problem with the stackoverflow solution was that it did not do what we need, split the complete filecontent into statement with borders at semicolons (which are not in quoted text) AND strip off the comments, inline, single line and multi line. This one you linked did not work for the complete file with multiple queries. I found another one for splitting but this then did not handle the comments. An all in one solution I did not find with regex, but found an answer in stackexchange telling that this would not be a trivial thing.

but found an answer in stackexchange telling that this would not be a trivial thing.

yeah, with complex regex it never is simple

but this PR is ready for test and that is what matters

I will be able to test again this evening.
Is it possible for Joomla to set a single standard for comments in sql, and other files too which is perhaps then adopted in J4.0 but encouraged in J3.5+ which gives plenty of time for coders to comment in the single Joomla way.

At the moment we have this single standard for sql (which works somehow for lucky circumstances), and with this PR we will also have same standard for all kinds of databases (but this time working well), because it not makes a difference between the databas kinds.

So I think this PR would be a good step in your direction.

Or did you mean we shall support only 1 kind of comment for all kinds of files?

I think this will not come true because sql guys not wanna miss their "--", while the shell scrip guys prefer their "#" ... not even to talk aout the c programmers.

Thanks in avance for later testing.

I was thinking of having a "standard" way for each of the file types..
but on my drive to work thought again, as it's only sql at the moment that would benefit.

Testing wise do I need to run all 5 tests? I presume you have updated your hosted files?

Yes, I have updated my files, the test sql scripts and also the update container behind the custom url.

I think about doing 1 more commit for making code better without changing functionality, but I can also do this later if time gets too tight. So if you or someone will test this before, I will not do that 1 more commit, in order not to need tests again, so it can go into 3.5.0 as it is now.

Otherwise, if I see nobody tested yet when I have it prepared later, then I will check it in when my files are updated again.

This PR has received new commits.

CC: @Twincarb, @wilsonge

Ready for test. Test update container is already updated, same custom URL. Comment to latest commit describes the changes. The test update container still is based on RC1 and not RC2 because with RC2 I had troubles with the changed handling of the manifest file.

Code is even closer now to code before the PR, so diff (comparison) for the complete PR is better understandable now, even if diff for the last commit might be less clear.

• Test One
• Test Two
• Test Three
• Test Four

I didnt do test five - didnt quite understand

Thanks for testing, Brian. Test 5 is just using a code snippet to read in a test sql file full of comments and stuff and statements, use the splitSql function modified by this PR to split the file content into sql statements array, and then output the extracted sql statements to the web site's page and check if it looks like the shown output, i.e. only contains the queries.

P.S.: The statements in test 5 are of course not executed. It is only to show what is result of reading the test sql file. So you can also do this test with sql file for postgresql when using a mysql.

P.P.S.: @brianteeman Ah, I just see that the "<br />" was messed up in my code snippet here because GutHub treats it as html, maybe that's what you did not understand? Have corrected it in the testing instrucions, so the code snippet is readable now and still can be copied into text editor.

I have tested this item successfully on 02462ed

All Tests passed.

I finished the final test for @brianteeman successfully. So with 2 good tests i'm merging this

Thanks everybody for testing.

Thankyou richard!