[4.3] Work around a successful upgrade with silent errors

[Fedik]

Summary of Changes

Kind of a fix for failed upgrade that report success.
I have added error collector, the most messages are loged now.
Also updated the "complete" layout, now it will show a meesage about error instead of "success".

Note: it is just a work around. A proper fix require more coding/re-coding of upgrade process.

Main issue that all messages like

joomla-cms/administrator/components/com_admin/script.php

Line 217 in 3d07930

echo Text::sprintf('JLIB_DATABASE_ERROR_FUNCTION_FAILED', $e->getCode(), $e->getMessage()) . '<br>';

are lost, and after upgrade is finished, the complete upgrade page shows just a static "Success"

joomla-cms/administrator/components/com_joomlaupdate/tmpl/joomlaupdate/complete.php

Line 23 in e7f5cc1

<?php echo Text::sprintf('COM_JOOMLAUPDATE_VIEW_COMPLETE_MESSAGE', '&#x200E;' . JVERSION); ?>

Testing Instructions

Download upgrade packge from the pr.
In joomla 3 in any Fields plugin add throw new Exception('Plugin test error'); in plugin constructor.
Run joomla upgrade, with manual upload of upgrade packge.

public function __construct(&$subject, $config)
{
  parent::__construct($subject, $config);
  throw new Exception('Error error, Fields plugin');
}

Addittionaly, try to add throw new Exception('Update test error'); in coupe of upgrade methods in administrator/components/com_admin/script.php

Actual result BEFORE applying this Pull Request

The finalise step will crash with messed up page.
Upgrade log reaport about "Files deleting blabla", but no error info.

Expected result AFTER applying this Pull Request

The finalise step are finished.The completion page show a message about unsuccessful update.
Upgrade log contains logs for error 'Plugin test error'.

[Fedik]

Addittionaly, if we can get this joomla-framework/event#35 in, we also can debug/ignore a broken plugins while upgrade.

[richard67]

I've made a few tests, and more will follow.

I've tested with updating from 4.3.1 stable so that there are some update SQL scripts to be executed.

For testing I have created custom update URLs and modified update packages based on the nightly build from today morning for the actual results without this PR applied, and the same based on the update package buit by Drone for this PR today.

Each of the packages has a modification to cause an error specific for the test. The modifications are described below for each test.

The links to the update packages and corresponding custom update site URLs are given below and will remain online for some time so other testers can use them, too.

Test 1: Cause database error in fixTemplateMode method of script.php so preflight fails with exception

Modification in the update packages in file "administrator/components/com_admin/script.php":

    protected function fixTemplateMode(): void
    {
        $db = Factory::getContainer()->get('DatabaseDriver');

        foreach (['atum', 'cassiopeia'] as $template) {
            $clientId = $template === 'atum' ? 1 : 0;
            $query    = $db->getQuery(true)
                ->update($db->quoteName('#__template_styles'))
                // Test PR #41367: Simulate database error
                // ->set($db->quoteName('inheritable') . ' = 1')
                ->set($db->quoteName('not_existing_column') . ' = 1')
                // Test PR #41367 end
                ->where($db->quoteName('template') . ' = ' . $db->quote($template))
                ->where($db->quoteName('client_id') . ' = ' . $clientId);

            try {
                $db->setQuery($query)->execute();
            } catch (Exception $e) {
                $this->collectError(__METHOD__, $e);
            }
        }
    }

This shall simulate any kind of database error happening at that place.

The above code is from the package with the PR applied, in the package without the PR it is done accordingly but of course without the changes from this PR.

Actual result without the PR applied

• Custom update URL: obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The "joomla_update.php" log file doesn't show anything special, i.e. no differences to the log from a normal update without errors beside of the time stamps, the update zip name and the user ID. All steps of the update have been processed.

Expected result with the PR applied

• Custom update obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The backend shows a page with is blank except of error messages or alerts, i.e. it is missing all backend menus and modules because using the system template.

Reloading the page doesn't change that.

Only using the "back" button of the browser 2 times or more or changing the URL in the browser's URL field e.g. to administrator/index.php without any options brings us back to the administrator template.

The "joomla_update.php" log file shows 2 errors as the error happens 2 times, one time for each core template. All steps of the update have been processed. See here the comparison, left side = with PR applied, right side = without PR applied:

Test 2: Let preflight method of script.php fail e.g. due to broken manifest cache and return false.

Modification in the update packages in file "administrator/components/com_admin/script.php":

    public function preflight($action, $installer)
    {
        if ($action === 'update') {
            // Get the version we are updating from
            /* Test PR 41367: Simulate missing or broken manifest
            if (!empty($installer->extension->manifest_cache)) {
                $manifestValues = json_decode($installer->extension->manifest_cache, true);

                if (array_key_exists('version', $manifestValues)) {
                    $this->fromVersion = $manifestValues['version'];

                    // Ensure templates are moved to the correct mode
                    $this->fixTemplateMode();

                    return true;
                }
            }
            Test PR 41367 end */

            return false;
        }

        return true;
    }

This shall simulate the preflight method failing due to a missing or incomplete manifest cache.

Actual result without the PR applied

• Custom update URL: obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The backend shows an alert with an unspecific warning.

The "joomla_update.php" log file shows no warning or error. But compared to the log file from a successful update you can see that the SQL update scripts have not been run. See following comparison, left side = log from this test without PR applied, right side = log from a successful update:

Expected result with the PR applied

• Custom update URL: obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The backend shows a page with is blank except of error messages or alerts, i.e. it is missing all backend menus and modules because using the system template.

Reloading the page doesn't change that except of closing the yellow alert, which is the same as shown without this PR applied.

Only using the "back" button of the browser 2 times or more or changing the URL in the browser's URL field e.g. to administrator/index.php without any options brings us back to the administrator template.

The "joomla_update.php" log file shows 1 error and 1 warning. Same as without the PR applied, the SQL update scripts have been not processed. See here the comparison, left side = with PR applied, right side = without PR applied:

Test 3: Cause an SQL error in an update SQL script (new script 4.3.4-2023-08-20.sql).

New update SQL scripts "4.3.4-2023-08-20.sql" with an erroneous SQL statement in the update packages.

This shall simulate any kind of SQL or database error during the SQL updates.

Actual result without the PR applied

• Custom update URL: obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The backend shows an alert about the SQL error.

The "joomla_update.php" log file shows the SQL error and reports the incomplete SQL updates.

Expected result with the PR applied

• Custom update URL: obsolete link removed
• Update package: obsolete link removed

Backend after the update:

The backend shows a page with is blank except of error messages or alerts, i.e. it is missing all backend menus and modules because using the system template.

Reloading the page doesn't change that except of closing the yellow alert, which is the same as shown without this PR applied.

Only using the "back" button of the browser 2 times or more or changing the URL in the browser's URL field e.g. to administrator/index.php without any options brings us back to the administrator template.

The "joomla_update.php" log file shows 1 warning and 1 error, but they are some kind of redundant to the logs reported already without the PR applied and which are still there when the PR is applied. See here the comparison, left side = with PR applied, right side = without PR applied:

Summary

In Test 1, without this PR, neither the backend nor the update log file show anything about the error, while with this PR both do.

For Test 2 the same applies as for Test 1, except that without the PR you can see in the update log file that the SQL updates have not been executed if you are an expert on database update.

My thoughts:

1. I'm not sure if it's a good idea to always use the system template, except if we can extend it by a link to the administrator/index.php without any options, so we navigate to the home dashboard.
2. The logs in test 3 added by this PR seem to be redundant, but that might not be the case in every scenario, so I am ok with them. @Fedik But maybe you want to change something there since it seems that SQL errors in the update SQL scripts are already handled well without your PR.

@Fedik More tests will follow sooner or later, but maybe you want to change something after these first results, so I will wait a bit for your feedback.

[Fedik]

@richard67 thanks, that a huge test ;)

I'm not sure if it's a good idea to always use the system template

This is in purpose, to increase a chance to see the result page of failed upgrade.
And to avoid modules rendering, in case some SQL fail to update one of them. Example fail of upgrade #__menu schema will lead to crash of the menu module.

The logs in test 3 added by this PR seem to be redundant,

I just replaced all echo to do log. However existing log to jerror category produce a message.
I tried to minimise changes in behavior. I would keep it as it is for 4.x.

It still would need a proper refactoring of whole upgrade code, there many strange pieces.
The upgrade process managed to survive all the way from Joomla 1.6 😄

Did you tried to test CLI update?

UPD. I will try to add "Dashboard button", or something like that.

[richard67]

Did you tried to test CLI update?

No, not yet. Meanwhile I have updated my custom update URLs so they can also be used for updating from 3.10.12. Will provide more test in the next days.

[Fedik]

I have added "back button", and changed message to be more helpfull.

@brianteeman please check the message text, thanks!

[brianteeman]

the english is fine if thats the message you want to say. i have no opinion on that

[richard67]

Up to now 7 of 9 tests were successful for me, the other 2 I haven't performed yet. I will update the results soon in my previous comment.

[Fedik]

@richard67 That a huge work, thank you Richard!

I have to look on untranslated FILES_JOOMLA_ERROR_MANIFEST.

[richard67]

@richard67 That a huge work, thank you Richard!

I have to look on untranslated FILES_JOOMLA_ERROR_MANIFEST.

@Fedik Maybe better do that with a new, separate PR.

[Fedik]

I think I juts replace it to fixed text. this should not require extra test, or what do you think?
It comes from files_joomla.sys.in, no idea what is it and how it suposed to be loaded.

I thinking to add "file" and "line" to the log message, as I see from test result it maybe helpfull.
What do you think?

[Fedik]

Okay, will do in another PR

[richard67]

I thinking to add "file" and "line" to the log message, as I see from test result it maybe helpfull.
What do you think?

@Fedik Good idea, but maybe also better with a new follow-up PR after this one. I wanna finish my tests and their descriptions, and then will try to find a 2nd tester.

[richard67]

I have tested this item ✅ successfully on bec0115

Detailed results of all my tests can be found in my previous comment here: #41367 (comment)

The comment contains also links to packages which I've created to produce the diverse errors, and links to corresponding custom update site URLs, so other testers can use them.

---

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

[Fedik]

I think these also pretty useless

joomla-cms/administrator/components/com_admin/script.php

Lines 8041 to 8046 in ba11ab4

	if ($suppressOutput === false && count($status['folders_errors'])) {
	echo implode('<br>', $status['folders_errors']);
	}
	if ($suppressOutput === false && count($status['files_errors'])) {
	echo implode('<br>', $status['files_errors']);

Curentlly I skipped it. But as I see, they also should be untranslated:

joomla-cms/administrator/components/com_admin/script.php

Line 8004 in ba11ab4

$status['files_errors'][] = Text::sprintf('FILES_JOOMLA_ERROR_FILE_FOLDER', $file);

joomla-cms/administrator/components/com_admin/script.php

Line 8020 in ba11ab4

$status['folders_errors'][] = Text::sprintf('FILES_JOOMLA_ERROR_FILE_FOLDER', $folder);

Does it make sense to add them to log by default? and ignore $suppressOutput variable.
It probably also for another PR.

[richard67]

Does it make sense to add them to log by default?

I think it could make sense, but it needs to be tested.

and ignore $suppressOutput variable.

Why ignore it? The default value of that parameter is false, and in administrator/components/com_joomlaupdate/finalisation.php and administrator/components/com_admin/script.php in the update method we call deleteUnexistingFiles without that parameter. Only in libraries/src/Console/RemoveOldFilesCommand.php is a call with the parameter set to true. So I think we can keep that condition as it is, we just have to use the result which we currently don't do.

[Fedik]

Why ignore it?

To fully eliminate any echo from script.php.
Also it does not make any sense to me, maybe it was used in past, but currently I cannot find any.

Yeah, can also be both, echo and log, for now.

[obuisard]

Tests done on a Windows 11 system, Firefox browser, local Bearsampp server running Apache 2.4.55 PHP 8.0.28 MySQL 8.0.32.
Following results are once the PR is applied.

Test 1 OK

Test 2 OK

Test 3 the update never ends, it stays stuck on 'The update is finalising'. I tried to update the modified package (created by Richard https://test5.richard-fath.de/Joomla_4.3.5-dev+pr.41367-Development-Update_Package_2023-09-09_test-3.zip), I just can't get it to finish. It just hangs.
Server error log:
PHP Warning: Uncaught Joomla\\Database\\Exception\\PrepareStatementFailureException: Unknown column 'not_existing_column' in 'field list' in E:\\bearsampp\\www\\MyWork_3_latest\\libraries\\vendor\\joomla\\database\\src\\Mysqli\\MysqliStatement.php:141

Test 3 OK under PHP 7.4.33

Test 4 OK

Test 5 OK

Test 6 update never ends, stays at 90.8%
Last 3 lines in joomla_update.php:

2023-09-12T02:02:18+00:00	INFO 127.0.0.1	update	End of SQL updates.
2023-09-12T02:02:18+00:00	INFO 127.0.0.1	update	Deleting removed files and folders.
2023-09-12T02:02:22+00:00	WARNING 127.0.0.1	jerror	Failed to load extension details.

Test 6 OK under PHP 7.4.33 That is a great one because it gives very useful feedback.

Test 7 update never ends, stays at 90.8%
PHP Warning: Undefined property: stdClass::$type in E:\bearsampp\www\MyWork_3_latest\administrator\components\com_admin\script.php on line 643

Last 3 lines in joomla_update.php:

2023-09-12T02:29:03+00:00	INFO 127.0.0.1	update	End of SQL updates.
2023-09-12T02:29:04+00:00	INFO 127.0.0.1	update	Deleting removed files and folders.
2023-09-12T02:29:10+00:00	WARNING 127.0.0.1	jerror	Failed to load extension details.

Test 7 OK under PHP 7.4.33

Test 8 OK

Test 9 update never ends, stays at 90.8%, then:

Last 2 lines in joomla_update.php:

2023-09-12T02:49:59+00:00	INFO 127.0.0.1	update	End of SQL updates.
2023-09-12T02:49:59+00:00	INFO 127.0.0.1	update	Deleting removed files and folders.

Test 9 OK under PHP 7.4.33

Thank you, Richard @richard67, for providing all these sample files and test directions, It was invaluable.

[richard67]

@obuisard Regarding the stuck and never ending tests: Which Version have you updated from? Have you used the MySQLi or the MySQL (PDO) driver? And how long have you waited? The error with undefined property for stdClass in test 7 could be caused by the error which I have added.

[richard67]

@obuisard Ah i know why it fails for you: error reporting and maybe debug on? Please try with standard error reporting and without debug, anything else breaks on php 8 when updating from 3.10.12, also without this PR applied.

[obuisard]

@obuisard Ah i know why it fails for you: error reporting and maybe debug on? Please try with standard error reporting and without debug, anything else breaks on php 8 when updating from 3.10.12, also without this PR applied.

Oh! I did not think that debug would mess it up. I will retry the failed tests.

[obuisard]

@obuisard Regarding the stuck and never ending tests: Which Version have you updated from? Have you used the MySQLi or the MySQL (PDO) driver? And how long have you waited? The error with undefined property for stdClass in test 7 could be caused by the error which I have added.

Updated from 3.10.12 in most cases, sometimes from 4.3.4 when tests did not require 3.10.12. Using MySQLi.
Waited a long time, sometimes (launching - taking a shower - still running :-) )

[obuisard]

I have tested this item ✅ successfully on ba11ab4

I have gotten satisfactory results for all tests 1 thru 9.
Initial failed tests happened under PHP 8.0.28, probably a problem of configuration. Time outs may happen on user systems as well, which cannot be prevented.

---

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

[richard67]

RTC

---

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