@dgrammatiko There is no need to remove the statement from the update SQL script. It does not do any harm as long as it does not have any syntax errors (which it doesn't).

Your proposed solution here is the fallback case that the SQL statement for some reason (SQL error in some other statement running in some other script before that one) has not been run. In such a case the backend is not usable, that's why we need the fallback.

P.S.: So I suggest you revert the changes on the 2 update SQL scripts and adust the PR description accordingly.

I suggest you revert the changes on the 2 update SQL scripts and adust the PR description accordingly.

Done

P.P.S.: Thanks for following my suggestion. I see my comment was not 100% right because the new fix runs before the update SQL scripts, so it is not really a fallback, but we should not change the old scripts if not necessary so the suggestion is still ok.

But now I ask myself what happens when we update from 3.10 to 4.1.

In this case with one of the early 4.0 update SQL scripts the atum and cassiopeia templates are added. Your new patch will run before that so it has nothing to update. So we again rely on the 4.1 update SQL to do that.

I think you should move the call to your new function to after the SQL updates so it is really a fallback like I first thought before I have noticed the call order.

I think you should move the call to your new function to after the SQL updates so it is really a fallback like I first thought before I have noticed the call order.

If the SQL updates have some fatal error will the function run then? That was my point running it before the SQL part...

If the SQL updates have some fatal error will the function run then? That was my point running it before the SQL part...

Up to now I think it will be the case, and if in future some proper error handling will be added we have to make sure it is run also when the SQL updates have failed (i.e. are incomplete).

I have tested this item ✅ successfully on 3bf7f17

@bembelimen consider this one as a release blocker

I have tested this item ✅ successfully on ca3738f

I have tested this item ✅ successfully on ca3738f

Unfortunately the new procedure added by this PR will not be executed if the update SQL scripts fail, and it is no solution to move the call to it to before the update SQL scripts run because then the statement in the new procedure will have no effect.

I think it needs the changes from PR #36506 and then see if we still need the fix from here and where we can hook it in so it is also called when the update SQL scripts fail.

@richard67 since the other PR was already merged is this one needed anymore?

@dgrammatiko I don't know yet. If you mean with the other one #36576 , then it does not help with the inheritable value. It still could need something like here to fix that in some case. But Nicholas' PR #36506 could help us here, that was the one I mean we should wait for, and that is not merged yet.

@richard67 I meant Nickolas' PR but I somehow mixed the numbers.

I think that this PR is still needed. Today with Joomla 4 RC1 the issue is still there.

@bembelimen @richard67 this remains kinda a release blocker. So if the #36506 doesn't solve the reported problems from @brianteeman and @joeforjoomla then this (in the current or another form) needs to be merged. It's important not to break the UI...

@dgrammatiko In the current form it does not help because your new method runs after the database updates, so if those fail it will not run, and if you move it to before the database update, it will not do anything because the data to be updated is not there yet.

It needs to run your new method after the database updates, but also if they fail.

Maybe @nikosdion has an idea where to hook that in?

Hmm, thinking about it further: When we run the new method before the db updates, it will not do anything when we update from a 3.10 because at this point there will not be any Atum and Cassiopeia templates yet. But when we update from 4.0.x it will.

The ugly sledge hammer could be to run it 2 times, one time before and one time after the db updates.

Do we want that?

It would be really not nice, but it would work.

The place where we could call the method in case if the schema updates fail is here: https://github.com/joomla/joomla-cms/blob/4.1-dev/administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L801

We could just add a $manifestClass->fixTemplateMode there, wrapped into an if ($manifestClass && method_exists($manifestClass, 'fixTemplateMode')), like later done below for other methods of script.php. For this it needs to make your new method public.

@dgrammatiko I will make a PR against your branch but before you merge that into your branch and so into this PR we should carefully test it, maybe adding some debug output to see if it's really called when we have an SQL error in an update SQL (like SELECT foo FROM notexistingtable;).

@richard67 I think you are overcomplicating this.

If the update fails at any stage then it should stop and tell the user where it failed or at least tell them to look in the log.

The problem right now is that instead of stopping it just goes on to the next stage of the update and never tells the user there was a problem.

The problem right now is that instead of stopping it just goes on to the next stage of the update and never tells the user there was a problem.

@brianteeman We know that, and it's what Nicholas will fix with his PR.

This PR here is about that when an update to 4.1 fails, whatever then will happen (sopping or going to the next stage), the wrong inheritable flag of the template will cause all backend assets not being found and so css and js for Atum not working and the backend being unusable.

And this will remain a problem if the other thing has been fixed.

i dont care what happens when an update fails - the site is screwed and you need to restore a backup - what dies it matter if the message is displayed in nice css?

the site is screwed and you need to restore a backup

Actually a well designed system should never allow you to reach to that point in the first place. Eg the order of execution in the updater is wrong, imho. The updater should check if all the db changes could be applied (without actually committing the changes) if so go on copy the files and then run the db changes. It's an architectural miss, the part with child templates just exposed it very clearly

The updater should check if all the db changes could be applied (without actually committing the changes)

@dgrammatiko This works only for DML (data manipulation language, i.e. INSERT, UPDATE, DELETE) but not for DDL (data definition language, DDL, e.g. ALTER TABLE). For DDL you don't have transactions and commit and rollback, they always apply immediately on (almost) any kind of relational database.

So for this approach it would need to create copies of the manipulated tables and apply the changes on these.

And we cannot really de-couple our DDL and DML because in the time over history they belong together, e.g. a DML statement might require a DDL statement to have run before.

@richard67 Off the top of my head.

The code by @dgrammatiko can go into the preflight() method. This runs before the SQL files run.

As for the code in the SQL files, you can merge #36506 and mark them with /** CAN FAIL **/. This way the SQL failing later wouldn't cause the entire update to fail.

@dgrammatiko You said:

Actually a well designed system should never allow you to reach to that point in the first place. Eg the order of execution in the updater is wrong, imho. The updater should check if all the db changes could be applied (without actually committing the changes) if so go on copy the files and then run the db changes. It's an architectural miss, the part with child templates just exposed it very clearly

Man... I have been saying that since 2012. I wrote a schema updater in FOF which did exactly that: each SQL query is executed conditionally i.e. only if it applies instead of based on a schema version. This worked beautifully for DDL and DML, I was using it for 8 years and it had solved all my problems across dozens of millions of installations and updates. Maybe by 2042 the Joomla project will see the light and implement something like that. I am not holding my breath nor am I wasting it anymore.

In any case, PR 36506 tries to address that issue by allowing some SQL statements to fail. It's the symmetrical solution to conditional execution (i.e. unconditional execution can lead to a failure we decide is acceptable), not as good though. Still it's better than Joomla going TITSUP (Total Inability To Sustain Update Process).

The code by @dgrammatiko can go into the preflight() method. This runs before the SQL files run.

Well, that would fix it at least for those who update from 4.0.x.

For those coming from 3.10 we could change the old update SQL "4.0.0-2018-03-05.sql" which is the very first which runs when updating from 3.10 and inserts the new templates and template styles for Atum and Cassiopeia so that it inserts the template style records with the right value 1 for the inheritable column (which would work because that column has been already added to 3.10 in past to fix broken updating).

Both together would be sufficient, I think.

But I don't think @brianteeman will like us to change the old update SQL.

For discussion I have created dgrammatiko#10 . Please let me know your opinion.

Maybe by 2042 the Joomla project will see the light and implement something like that

@nikosdion hopefully someone would be brave enough to do it even earlier

@richard67 looks good to me, maybe we should also add the /** CAN FAIL **/ in the sql files 2021-11-28... (although these files shouldn't be the failing point). Anyways it's a ? from me

@richard67 looks good to me, maybe we should also add the /** CAN FAIL **/ in the sql files 2021-11-28... (although these files shouldn't be the failing point).

No, the /** CAN FAIL **/ we should use for those where we have duplicate primary key (when inserting the workflows stuff) or when we try to rename a table which already has been renamed with a previous attempt before.

For update statements which do nothing if there is no data which matches the where condition, we do not need to allow to fail.

@brianteeman Could you check dgrammatiko#10 and let me know if this would be an acceptable exception from the update files being (almost) immutable? I think it really would be useful here because we already have unpacked the 4.1 files at this point and so the templates styles have to be inheritable from beginning on when updating to 4.1.

@dgrammatiko One can only hope. I certainly have the experience. If the project wants my help I can offer my experience and possibly my code.

@richard67 I agree with Dimitris, add the /** CAN FAIL **/ in case the table schema is somehow wrong. This will prevent the UPDATE from failing cold and leaving the DB in an unrecoverable state.

@richard67 I agree with Dimitris, add the /** CAN FAIL **/ in case the table schema is somehow wrong. This will prevent the UPDATE from failing cold and leaving the DB in an unrecoverable state.

@nikosdion Let's agree to decide about where we use that /** CAN FAIL **/ when your PR to handle that has been tested and merged. I will try all to find time for testing latest next weekend.

Here in this PR I would not add the /** CAN FAIL **/ before your PR got merged because it will not do anything right now.

@nikosdion Does dgrammatiko#10 look ok to you and is the change in the old update SQL acceptable in your opinion?

@richard67 It does look good to me and I agree with changing the update SQL. Not ever fixing mistakes, even if it means retroactive changes, is a bad policy. As long as it's possible to upgrade a site from 3.10 -> 4.0 -> 4.1 just as well as it's possible to upgrade a site from 3.10 -> 4.1 directly a retroactive change is acceptable.

So, yes, I agree with this change.

As long as it's possible to upgrade a site from 3.10 -> 4.0 -> 4.1 just as well as it's possible to upgrade a site from 3.10 -> 4.1 directly a retroactive change is acceptable.

This change would not allow that. You could only go from 3.10 -> 4.1

As long as it's possible to upgrade a site from 3.10 -> 4.0 -> 4.1 just as well as it's possible to upgrade a site from 3.10 -> 4.1 directly a retroactive change is acceptable.

This change would not allow that. You could only go from 3.10 -> 4.1

In the 4.0-dev branch and so in any 4.0 package still the old files will be in.

The modification takes place only in the 4.1-dev branch and later.

So both will still be possible, update a 3.10 to 4.0 and this the later to the 4.1 which includes this PR, and updating the 3.10 directly to 4.1 which includes this PR, and this PR would fix it in both cases. Without the update SQL changes, the update from 3.10 to 4.1 would work with the wrong inheritable value when the update breaks somewhere at the end.

@dgrammatiko I think we can move from draft to ready for review.

Regarding testing, I suggest we create two modified update packages, one based on an unpatched 4.1 update (latest nightly) and one based on the package built by drone.

In both packages we produce an SQL error in script "4.1.0-2021-11-20.sql" (for both databases) at the end, e.g. with an update statement for a not existing table. This will simulate that something happens with the database just before the script "4.1.0-2021-11-28.sql" with the update for the child templates runs, e.g. an unexpected database server shutdown.

With these modified update packages test updating from 3.10 to the particular package and with another test from 4.0.x to the same package.

In both cases update from 3.10 and update from 4.0.x, following should be true:

• In case of the package without your PR, the backend will be messed (css and js) and so not be usable, e.g. buttons are missing.
• In case of the package with your PR, the backend will still be usable.

I will create the 2 packages tomorrow

I am just on it. Waiting for drone to build the one with the PR.

@dgrammatiko I have created the update packages:

• Update package with SQL error and without the changes from this PR is https://test5.richard-fath.de/Joomla_4.1.0-dev-Development-Update_Package_test-with-sql-error.zip
• Update package with SQL error and with the changes from this PR is https://test5.richard-fath.de/Joomla_4.1.0-dev+pr.36585-Development-Update_Package_test-with-sql-error.zip

Nice, I've updated the description, now we need couple brave testers

8 tests if one has both kinds of databases MySQL and PostgreSQL.

For each db type 4 tests:

• Update from 3.10.x to the 4.1 package with SQL error and without your PR => actual messed result
• Update from 4.0.x to the 4.1 package with SQL error and without your PR => actual messed result
• Update from 3.10.x to the 4.1 package with SQL error and with your PR => expected usable result
• Update from 4.0.x to the 4.1 package with SQL error and with your PR => expected usable result

@dgrammatiko Please update your testing instructions to make clear it needs to update from 3.10.x or 4.0.x. Updating from 4.1 (e.g. a previous beta) will not be affected by the change in this PR.

The update tests from 4.0 with both kinds of database I just have finished with success. Now I have a short break, and then I make the tests from 3.10.

@dgrammatiko I've pimped the testing instructions a bit, I hope that's ok.

I can test but not until tomorrow pm

I have tested this item ✅ successfully on 73b97af

For each db type MySQL and PostgreSQL I have tested the following 4 tests (so 8 tests in total):

• Update from 3.10.x to the 4.1 package with SQL error and without your PR => actual messed result
• Update from 4.0.x to the 4.1 package with SQL error and without your PR => actual messed result
• Update from 3.10.x to the 4.1 package with SQL error and with your PR => expected usable result
• Update from 4.0.x to the 4.1 package with SQL error and with your PR => expected usable result

When updating from 3.10 there was an additional problem with and without this PR applied, which I will describe more detailed in my next comment.

But that's not related to this PR and has to be fixed with another PR.

@dgrammatiko When updating from 3.10 there is an additional issue not related to your PR. But maybe you should mention it or link to this comment in the testing instructions so people will not count it as failed test.

After the files have been updated (but I think before the database update starts), you get a blank page, and the page title shows an error message (full message in a tool tip when hovered):

Then just refresh the page, and the update continues and then ends as described in the testing instructions.

The database checker will show errors because the old update SQL scripts with versions from 2.5. to 3.x have not been deleted.

When testing the update from 4.0, the blank page does not happen. There will also old files not be deleted, but because there are no old update SQl files to be deleted, there will not be such database checker errros like after an update from 3.10.

But all that happens with and without your PR and so has to be fixed elsewhere.

In all my original tests I never saw that blank page error

@brianteeman I could just reproduce that several times when updating a current 3.10-dev to 4.1, using a "normal" update package without any SQL error, e.g. the last nightly build https://developer.joomla.org/nightlies/Joomla_4.1.0-dev-Development-Update_Package.zip or the custom update URL https://update.joomla.org/core/nightlies/next_minor_list.xml for that nightly.

@richard67 @brianteeman isn't that the bug where an instance of $this->db was replaced by $db in the Table.php?

@richard67 @brianteeman isn't that the bug where an instance of $this->db was replaced by $db in the Table.php?

Yes, was my PR #36733 ... but it was not in table but in redirect, and here I do not have redirects enabled.

Yes, was my PR #36733 ... but it was not in table but in redirect, and here I do not have redirects enabled.

Actually the problem is different, in 3.10 the driver is JDatabaseDriverMysqli

		$db = isset($config['dbo']) ? $config['dbo'] : \JFactory::getDbo();

In 4.1 it's JDatabaseDriverMysqli

$db = $config['dbo'] ?? Factory::getDbo();

In short, the alias is not yet established...

BTW this raises some questions...

Actually the problem is different, in 3.10 the driver is JDatabaseDriverMysqli

		$db = isset($config['dbo']) ? $config['dbo'] : \JFactory::getDbo();

In 4.1 it's JDatabaseDriverMysqli

$db = $config['dbo'] ?? Factory::getDbo();

In short, the alias is not yet established...

Please post that also in the new issue #36833 . Thanks in advance.

I have partially tested it with success as I had a branch from 4.0.6 with npm ci done. Then I switched to 4.1, did npm ci, which resulted in template files not loaded. Then run the SQL update script from this pr, did an npm ci and template did load fine then.

Didn't test the installer script.

I have tested this item ✅ successfully on f77dfc4

Bug confirmed for 3.10.5 and 4.0.6.
Patch worked and upgrading to 4.1 from J3 and J4 with the patched package was successful. In both cases DB repair worked and fixed the errors.

RTC

Thx

I can test but not until tomorrow pm

I guess I dont need to test this then