@brianteeman You haven't asked me for review but I noticed the PR and had a quick look. From code reading I think it should work, and I think it's an improvement, so ok for me.

@richard67 Everyone is welcome to review. I just asked Nik and George specifically as they have the most intimate knowledge of the installer

@brianteeman I'd count myself too to those who have such knowledge, at least to some large part.

It's true that @richard67 has accrued a wealth of knowledge about the internal workings of the installer by fixing everyone else's broken crap (including mine) and working on the UTF8MB4 integration. @brianteeman you can tag him for installer and database issues, he's one of the few people who have this kind of intimate knowledge.

My apologies no slight or offence intended

OK, back to the PR.

I really like what it's doing and I fully agree with the intention and the methodology.

I would add only one more thing: log the $version read from the #__schemas table and log which files are being skipped over because the version_compare() check return <= 0. This would help developers and especially users understand that the wrong file's queries were executed because the schema version is wrong.

Side note: The fact that the #__schemas version is not updated after the full execution of an entire file is, in my opinion, a bug. Think about the following case. You have installed version 1.0.0 of an extension and the schema version is 1.0.0-202110231200. You are now about to install version 1.0.2 which needs to execute the following update files:

• 1.0.1-202111011234.sql
• 1.0.1-202111030654.sql
• 1.0.2-202112152233.sql
  An error occurs while executing the first SQL query of the second file (1.0.1-202111030654). Looking at the new log we figure out that's because a table to be updated was marked as crashed in MySQL.

We fix that and we try the update again. The update fails because the first file (1.0.1-202111011234) will be executed again. However, its changes are already applied and the ALTER TABLE commands it contains fail. There is no way to address this issue unless a. you uninstall the extension (and lose all data in it!) or b. edit the #__schemas table to mark the schema version as 1.0.1-202111011234.

My idea is that as a user you should NOT have to do that. It's not your responsibility to keep track of minutiae like that. Joomla itself should know it has already executed 1.0.1-202111011234 and refrain from executing it again. The simple way to do that is add an update to the #__schemas table at the end of the foreach loop.

If @richard67 and @wilsonge agree I could help @brianteeman do that. It would make it SO MUCH EASIER for us 3PDs to provide support on failed updates without having us to ask for access to the client's database server or provide them with doctored installation ZIP files.

I would add only one more thing: log the $version read from the #__schemas table and log which files are being skipped over because the version_compare() check return <= 0. This would help developers and especially users understand that the wrong file's queries were executed because the schema version is wrong.

I will take a look at adding that

Files and folders

Just seen that the code already exists to log the actual files and folders removed. @PhilETaylor added it #31943 so if we want to add that its simple to do.

Still ignores the bigger issue for me is that if the sql part fails with an error then we go directly to the success message and skip the files/folders altogether

I would add only one more thing: log the $version read from the #__schemas table and log which files are being skipped over because the version_compare() check return <= 0. This would help developers and especially users understand that the wrong file's queries were executed because the schema version is wrong.

Log the schema version at the beginning it ok, but lok all the skipped update SQL because they are older than this schema version is redundant information, I think.

Side note: The fact that the #__schemas version is not updated after the full execution of an entire file is, in my opinion, a bug. Think about the following case. You have installed version 1.0.0 of an extension and the schema version is 1.0.0-202110231200. You are now about to install version 1.0.2 which needs to execute the following update files:

...

The simple way to do that is add an update to the #__schemas table at the end of the foreach loop.

This should indeed be fixed.

updated to show schema version at the start

II am not 100% certain that this is the best approach to this. @nikosdion @wilsonge please advise

I think the general approach of using the log category is fine. Having said that it might need a bit more nuance - I think with this change removing any sort of message shown to a user when updating an extension (as this code is shared there) https://github.com/joomla/joomla-cms/pull/36492/files#diff-b99bfdb7da0a1b88f83ba167cfd61b61b3ee3f3875ba5e252258cb761b9bbfb1L1209

Obviously the end intention is correct to log this stuff when we're updating Joomla. Just need to make sure we don't make the extension update pathway worse in the process.

If @richard67 and @wilsonge agree I could help @brianteeman do that. It would make it SO MUCH EASIER for us 3PDs to provide support on failed updates without having us to ask for access to the client's database server or provide them with doctored installation ZIP files.

Obviously this sounds very logical and useful. I'm happy.

Still ignores the bigger issue for me is that if the sql part fails with an error then we go directly to the success message and skip the files/folders altogether

https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L794-L802 So what should happen in this scenario is we run a full rollback. However because this is core we don't really have a rollback path. I guess I listen to @nikosdion as the expert here but I guess in the case here of upgrading core maybe attempting a rollback is probably not such a great idea as it is for extension as we won't rollback the files (and even if we did we'd have no way to remove any new files that had been added in the new release).

when updating an extension (as this code is shared there)

Which is the main reason I asked for input as I didnt even know that AND now that I do it doesn't really help as I have almost zero experience with developing extensions

@richard67 You said

Log the schema version at the beginning it ok, but lok all the skipped update SQL because they are older than this schema version is redundant information, I think.

No, it's not. When dealing with unstable versions (dev, alpha, beta, ...) PHP has a very weird and specific way of handling version_compare(). It's easy to mess it up. I did it, it took me 10' to realise that my problem was that I didn't think through how version_compare() actually works. If I had that in a log it would have taken me 1' instead 😜

@wilsonge

Obviously the end intention is correct to log this stuff when we're updating Joomla. Just need to make sure we don't make the extension update pathway worse in the process.

IMHO we should improve the message logged to the user (instead of removing it), saying which file and SQL statement failed. Otherwise it's as good as not saying anything. This has frustrated me to no end trying to debug my own extension updates now that I am back into using the core database schema updates.

Obviously this sounds very logical and useful. I'm happy.

I'll do a PR to Brian's PR then! Awesome! This thing has bugged me for years.

I guess I listen to @nikosdion as the expert here but I guess in the case here of upgrading core maybe attempting a rollback is probably not such a great idea as it is for extension as we won't rollback the files (and even if we did we'd have no way to remove any new files that had been added in the new release).

Oof... I'll try to be as concise as possible but you'll have to bear with me. The TL;DR is that indeed it's not a good idea attempting a full rollback. Restoring from a backup will always yield better results which is why we suggest that. The only thing we can improve is communicating WHY the upgrade failed so that next time the user doesn't run into the exact same problem.

And now, to the technical nitty–gritty.

Updating Joomla is unlike every other extension. First of all, while Joomla registers itself as a files extension (so that SQL updates can execute) it is not installed as a files extension update. While that was the way it worked back in Joomla 1.6 and 1.7, it quickly became apparent that many shared hosting environment were so darned slow that unpacking the files would time out. The solution to that was Joomla! Update which runs the update in three stages:

1. Download the update ZIP file (atomic operation in a single page load)
2. Extract the update ZIP file (multiple page loads)
3. Run the post–update code which:
   a. Applies SQL updates through the extensions installer code
   b. Removes obsolete files

Performing a full rollback would require maintaining a copy of the files which were replaced and a log of the new files created. Filesystem rollback would consist of copying the backups of the copied files and removing the new files.

(Note that for extensions it's just a matter of copying over entire folders. There are fewer files and it doesn't time out. For Joomla itself it's not possible, it would be equivalent to taking a backup of the site on the same server which is likely to time out or have us run out of disk space).

Likewise, performing a full rollback would require rolling back database changes. This is where things get VERY hairy. At the very least we'd need to have anti–update SQL files for each update SQL file we have. This means going back and doing that for every little update file which is a mammoth task, not guaranteed to succeed. We'd also need to know at which point to stop the rollback i.e. not roll back to a version before the one we started upgrading from. That's possible but convoluted.

Even if we have that, there are a few questions left to answer. What happens when we drop an old table? We obviously cannot restore it. Instead of dropping tables we'd have to rename them to a temporary name and at the end of the post–update code drop them (if the update was successful) or rename them back (if the update failed). This is complicated and means we can no longer use the same update code as extensions OR we have to introduce a hard b/c break for extension updates. Damned if you do, damned if you don't kind of situation.

Even worse, this kind of full rollback may actually take so long that the process times out and we have an inconsistent state of the site. I would actually say that on most servers this is exactly what will happen.

In other words, there are no guarantees that we can ever perform a full rollback to a consistent state or at the very least detect when things are broken beyond our ability to fix them. Incidentally, the latter state is exactly what will happen with a failed update.

The best advice you can give to users is to take a backup BEFORE updating so they can restore their site if the update fails. We already do that.

Moreover, we have to tell them why the update failed so they can fix that before they retry the update. This is something that didn't use to happen but was largely fixed in 4.0.4 with the new Joomla Update backend code I contributed. It will tell you why it broke and how to fix it.

The only thing missing is communicating correctly the reason why the database part of the update failed. The logging added here should help tons. Beyond that, I don't know what else we could do.

But, no, a full rollback is not practical unless we decide that the only way to do a Joomla Update is via the command line which makes Joomla useless for the majority of its target audience. And even then we can't really be 100% sure that the rollback will succeed.

Continuing on the same train of thought and since it's something we had started discussing back in 2015 at the J4WG, no, having a schema updater a la Doctrine won't help either. I've used that with Python (managing the updates on a Weblate installation) and I found that they are WORSE in the messy real world than Joomla's SQL update files. They can't do rollback any more than Joomla's SQL update files currently do (that is to say: not at all!) and they are even more sensitive to starting conditions. If the schema you start with does not match exactly the previous schema version's definition you're crap outta luck. You need to restore from a backup, make manual DB changes and hope it works this time. This means deciphering the metalanguage used to describe the schema and figuring out how it would be translated to the actual database schema. This works great if you have a fleet of servers with tightly controlled updates, it does not work in the messy real world with end users performing their own updates. That's why I was so against a DBAL for handling schema updates.

Moreover, we have to tell them why the update failed so they can fix that before they retry the update. This is something that didn't use to happen but was largely fixed in 4.0.4 with the new Joomla Update backend code I contributed. It will tell you why it broke and how to fix it.

this is my motivation for doing this as it really doesnt tell you what broke and it lies to you and says it is successful

... and it lies to you and says it is successful

@brianteeman Well, as people very likely did lie before when they checked the "I have made a backup ..." check box, I think that's ok 😄 (sorry for silly joke)

I think we should do the update of the schema version in database after each SQL script in a separate PR and let this one here handle the log messages and other messages, i.e. not try to make this one here solve everything because that could be too much in one PR.

For testing it is just sufficient to add some SQL which results in an error to one of the last update SQL (not the early ones because if those break then almost all is broken), e.g. a drop table statement for a table which does not exist. That should be enough to cause a failure at the desired point in an update SQL script.

E.g. if we put the statement which causes an error into script "4.0.3-2021-09-04.sql", then script "4.0.3-2021-09-05.sql" will not run. The last successfully run script would then be "4.0.0-2021-08-17.sql" when updating anything older that 4.0.3, with nothing happening when updating a 4.0.0 or 4.0.1 or 4.0.2 so for testing one should update from 3.10 or a 4.0.0-beta-x, and "4.0.0-2021-08-17" should be the schema version in DB after that.

I think we should do the update of the schema version in database after each SQL script in a separate PR and let this one here handle the log messages and other messages, i.e. not try to make this one here solve everything because that could be too much in one PR.

OK. I can do that tomorrow morning. I need to finish up some stuff for my own software today — and I am taking it slow and easy as I am technically on vacation :)

@richard67 my test instructions already simulate the errors

@nikosdion The tricky thing could be - if I remember right - that at the end of the update when it thinks it was successful, there is already an update of the schema version in the DB, and if I remember right it just sets the schema version according to the latest present file, regardless if that has been run with success or not. This could then make you trouble and at the end overwrite the result which your new functionality will have written. I thought I let you know in case if you run into this when testing your changes.

@richard67 my test instructions already simulate the errors

@brianteeman That's right. I just thought I could simplify.

@richard67 The trick is that we'd be removing this code since we update the schema version with the latest actually installed SQL update file. From a 3PD's perspective, this means that isolating each atomic change into its file guarantees that failures are recoverable. It is indeed one of the things I already have in mind. I even have a test fake component to test what happens with borked updates. I had built that to know for certain how Joomla behaves so I know how to provide support. I just need to shape it up so it can be used for testing the PR.

@richard67 The trick is that we'd be removing this code since we update the schema version with the latest actually installed SQL update file. From a 3PD's perspective, this means that isolating each atomic change into its file guarantees that failures are recoverable. It is indeed one of the things I already have in mind. I even have a test fake component to test what happens with borked updates. I had built that to know for certain how Joomla behaves so I know how to provide support. I just need to shape it up so it can be used for testing the PR.

@nikosdion I understood that. In fact I wanted to make that since some 2 years. I just thought I should mention that we need to remove a piece of code somewhere.

@brianteeman I assume right that the last section header in the testing instructions should be "After Test 3" and not "Before Test 3"?

yes - typo - now corrected

@brianteeman I've found 2 glitches (one already in 4.x-dev and another one in this PR) and made a PR for you with a fix. Please check brianteeman#155 . Thanks in advance.

@wilsonge The 2nd glitch is the one which you and me had expected, that the warning alert shown in backend will get lost with this PR here as it is now.

2022-01-08T15:48:01+00:00	INFO 192.168.98.1	update	Starting installation of new version.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	Finalising installation.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	Start of SQL updates.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	The current database version (schema) is 4.0.6-2021-12-23.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	Ran query from file 4.0.6-2022-01-07. Query text: UPDATE `#__gaga` SET `blabla` = 'blub';.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	JInstaller: :Install: Error SQL Table 'joomladb1.j4ux0_gaga' doesn't exist
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	End of SQL updates - INCOMPLETE.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	Cleaning up after installation.
2022-01-08T15:48:05+00:00	INFO 192.168.98.1	update	Update to version 4.0.6-dev+pr.36492 is complete.

@brianteeman Cool, the new logs. Really a good improvement, together with the fix of the wrong exception type.

I have tested this item ✅ successfully on dcf24bd

I have tested this item ✅ successfully on dcf24bd

@wilsonge I think this is a release blocker and has to go into 4.0.6 since it fixes also the wrong exception type, see screenshots in brianteeman#155 .

Thanks!

thanks