[4.1] Fix the upgrade path #36585

dgrammatiko · 2022-01-06T11:41:00Z

Pull Request for Issue # .

Summary of Changes

Introduced a function in the script.php that will run independently of the batch work of the db updates. The reason is that this particular db fix is essential so that the UI will not become broken if for any reason the update files fail somehow

Testing Instructions

Using https://test5.richard-fath.de/Joomla_4.1.0-dev-Development-Update_Package_test-with-sql-error.zip try to

Update from 3.10.x to the 4.1 => actual messed result
Update from 4.0.x to the 4.1 => actual messed result
(these SHOULD FAIL leaving you with unstyled front/backend templates)

Using https://test5.richard-fath.de/Joomla_4.1.0-dev+pr.36585-Development-Update_Package_test-with-sql-error.zip try to

Update from 3.10.x to the 4.1 => expected usable result
Update from 4.0.x to the 4.1 => expected usable result
(these should show an SQL error at the end, but the backend is usable and the db fixer can be used)

Actual result BEFORE applying this Pull Request

When the database update fails before the update for child templates would have run, the UI is unusable.

Expected result AFTER applying this Pull Request

The UI is usable. An SQL error is shown, but the backend works, and the database fixer can be used.

Documentation Changes Required

@brianteeman @joeforjoomla could you test this one?

@richard67 I hope I didn't messed up here

richard67 · 2022-01-06T11:44:55Z

@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.

richard67 · 2022-01-06T11:46:05Z

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

dgrammatiko · 2022-01-06T11:49:22Z

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

Done

richard67 · 2022-01-06T11:52:43Z

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.

dgrammatiko · 2022-01-06T11:57:17Z

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...

administrator/components/com_admin/script.php

richard67 · 2022-01-06T12:00:45Z

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).

joeforjoomla · 2022-01-06T20:34:30Z

I have tested this item ✅ successfully on 3bf7f17

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

dgrammatiko · 2022-01-06T21:59:15Z

@bembelimen consider this one as a release blocker

administrator/components/com_admin/script.php

dgrammatiko · 2022-01-07T17:00:12Z

administrator/components/com_admin/script.php

+	{
+		$db = Factory::getDbo();
+
+		array_map(


functional programming? No real reason, it's a bit shorter

Don't believe everything you find on the internet :)

FWIW browsers (JS) are optimizing for these functions and I would expect PHP would be doing that as well but for PHP I don't have any data...

Edit: actually this comment aligns with what I wrote ;)

Functional programming is very popular in the JS space and the reason is because React, Vue, Svelte, etc basically made the UI a function of the state UI = Fn(state)

I hate that I can't use the spread operator yet...

Co-authored-by: Phil E. Taylor <phil@phil-taylor.com>

AJAY007L · 2022-01-08T11:23:52Z

I have tested this item ✅ successfully on ca3738f

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

richard67 · 2022-01-08T12:59:42Z

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.

dgrammatiko · 2022-01-14T17:39:42Z

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

dgrammatiko · 2022-01-24T21:09:55Z

I will create the 2 packages tomorrow

richard67 · 2022-01-24T21:10:20Z

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

richard67 · 2022-01-24T21:20:02Z

@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

dgrammatiko · 2022-01-24T21:24:19Z

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

richard67 · 2022-01-24T21:27:14Z

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

richard67 · 2022-01-24T21:28:30Z

@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.

richard67 · 2022-01-24T21:47:41Z

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.

richard67 · 2022-01-24T21:55:57Z

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

brianteeman · 2022-01-24T22:09:32Z

I can test but not until tomorrow pm

richard67 · 2022-01-24T22:17:51Z

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.

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

richard67 · 2022-01-24T22:24:11Z

@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.

brianteeman · 2022-01-24T22:25:58Z

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

richard67 · 2022-01-24T22:34:03Z

@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.

dgrammatiko · 2022-01-24T22:37:31Z

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

richard67 · 2022-01-24T22:40:57Z

@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.

dgrammatiko · 2022-01-24T22:48:53Z

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...

richard67 · 2022-01-24T22:49:59Z

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.

laoneo · 2022-01-25T09:28:53Z

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.

RickR2H · 2022-01-25T09:45:52Z

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.

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

richard67 · 2022-01-25T09:51:12Z

RTC

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

bembelimen · 2022-01-25T13:41:58Z

Thx

brianteeman · 2022-01-25T14:15:15Z

I can test but not until tomorrow pm

I guess I dont need to test this then

Use a function for the new mode

acb2f98

joomla-cms-bot added the PR-4.1-dev label Jan 6, 2022

dgrammatiko mentioned this pull request Jan 6, 2022

Assets not loaded after upgrading from 4.0 to 4.1 #36567

Closed

cs

77ae2cc

ctrl+z

18b2f17

richard67 reviewed Jan 6, 2022

View reviewed changes

administrator/components/com_admin/script.php Outdated Show resolved Hide resolved

CS

075f797

richard67 reviewed Jan 6, 2022

View reviewed changes

administrator/components/com_admin/script.php Outdated Show resolved Hide resolved

int

172d057

dgrammatiko added 4 commits January 6, 2022 13:02

order please

84c17cd

Merge branch '4.1-dev' into 4.1/update

5d671e6

int

c372b47

Merge branch '4.1-dev' into 4.1/update

3bf7f17

PhilETaylor reviewed Jan 7, 2022

View reviewed changes

administrator/components/com_admin/script.php Outdated Show resolved Hide resolved

PhilETaylor reviewed Jan 7, 2022

View reviewed changes

administrator/components/com_admin/script.php Outdated Show resolved Hide resolved

PhilETaylor reviewed Jan 7, 2022

View reviewed changes

dgrammatiko and others added 2 commits January 7, 2022 17:59

Update administrator/components/com_admin/script.php

541456d

Co-authored-by: Phil E. Taylor <phil@phil-taylor.com>

Update administrator/components/com_admin/script.php

ca3738f

Co-authored-by: Phil E. Taylor <phil@phil-taylor.com>

dgrammatiko marked this pull request as draft January 8, 2022 12:39

Merge branch '4.1-dev' into 4.1/update

f77dfc4

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 25, 2022

bembelimen merged commit 5f68ac3 into joomla:4.1-dev Jan 25, 2022

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 25, 2022

bembelimen added this to the Joomla 4.1.0 milestone Jan 25, 2022

dgrammatiko deleted the 4.1/update branch January 25, 2022 18:03

richard67 mentioned this pull request Feb 27, 2022

[4.2] Make update SQL scripts resilient to multiple executions so broken updates can be fixed #37156

Merged

richard67 mentioned this pull request Mar 30, 2023

[5.0] Remove child template conversion of core templates from script.php #40253

Merged

2 tasks

[4.1] Fix the upgrade path #36585

[4.1] Fix the upgrade path #36585

Conversation

dgrammatiko commented Jan 6, 2022 • edited by richard67

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

richard67 commented Jan 6, 2022

richard67 commented Jan 6, 2022

dgrammatiko commented Jan 6, 2022

richard67 commented Jan 6, 2022

dgrammatiko commented Jan 6, 2022

richard67 commented Jan 6, 2022

joeforjoomla commented Jan 6, 2022

dgrammatiko commented Jan 6, 2022

This comment was marked as abuse.

dgrammatiko Jan 7, 2022

Choose a reason for hiding this comment

This comment was marked as abuse.

dgrammatiko Jan 7, 2022 • edited

Choose a reason for hiding this comment

This comment was marked as abuse.

dgrammatiko Jan 7, 2022

Choose a reason for hiding this comment

This comment was marked as abuse.

dgrammatiko Jan 7, 2022

Choose a reason for hiding this comment

This comment was marked as abuse.

AJAY007L commented Jan 8, 2022

richard67 commented Jan 8, 2022 • edited

dgrammatiko commented Jan 14, 2022

dgrammatiko commented Jan 24, 2022

richard67 commented Jan 24, 2022

richard67 commented Jan 24, 2022

dgrammatiko commented Jan 24, 2022

richard67 commented Jan 24, 2022

richard67 commented Jan 24, 2022

richard67 commented Jan 24, 2022

richard67 commented Jan 24, 2022

brianteeman commented Jan 24, 2022

richard67 commented Jan 24, 2022

richard67 commented Jan 24, 2022

brianteeman commented Jan 24, 2022

richard67 commented Jan 24, 2022

dgrammatiko commented Jan 24, 2022

richard67 commented Jan 24, 2022

dgrammatiko commented Jan 24, 2022 • edited

richard67 commented Jan 24, 2022

laoneo commented Jan 25, 2022

RickR2H commented Jan 25, 2022

richard67 commented Jan 25, 2022

bembelimen commented Jan 25, 2022

brianteeman commented Jan 25, 2022

dgrammatiko commented Jan 6, 2022 •

edited by richard67

dgrammatiko Jan 7, 2022 •

edited

richard67 commented Jan 8, 2022 •

edited

dgrammatiko commented Jan 24, 2022 •

edited