-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4.3] Work around a successful upgrade with silent errors #41367
Conversation
Addittionaly, if we can get this joomla-framework/event#35 in, we also can debug/ignore a broken plugins while upgrade. |
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 exceptionModification in the update packages in file "administrator/components/com_admin/script.php":
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
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
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":
This shall simulate the preflight method failing due to a missing or incomplete manifest cache. Actual result without the PR applied
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
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
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
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: SummaryIn 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:
@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. |
@richard67 thanks, that a huge test ;)
This is in purpose, to increase a chance to see the result page of failed upgrade.
I just replaced all It still would need a proper refactoring of whole upgrade code, there many strange pieces. Did you tried to test CLI update? UPD. I will try to add "Dashboard button", or something like that. |
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. |
I have added "back button", and changed message to be more helpfull. @brianteeman please check the message text, thanks! |
the english is fine if thats the message you want to say. i have no opinion on that |
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. |
@richard67 That a huge work, thank you Richard! I have to look on untranslated |
@Fedik Maybe better do that with a new, separate PR. |
I think I juts replace it to fixed text. this should not require extra test, or what do you think? I thinking to add "file" and "line" to the log message, as I see from test result it maybe helpfull. |
Okay, will do in another PR |
@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. |
I have tested this item ✅ successfully on bec0115 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. |
I think these also pretty useless joomla-cms/administrator/components/com_admin/script.php Lines 8041 to 8046 in ba11ab4
Curentlly I skipped it. But as I see, they also should be untranslated:
Does it make sense to add them to log by default? and ignore |
I think it could make sense, but it needs to be tested.
Why ignore it? The default value of that parameter is |
To fully eliminate any Yeah, can also be both, echo and log, for now. |
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. 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. Test 3 OK under PHP 7.4.33 Test 4 OK Test 5 OK Test 6 update never ends, stays at 90.8%
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% Last 3 lines in joomla_update.php:
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:
Test 9 OK under PHP 7.4.33 Thank you, Richard @richard67, for providing all these sample files and test directions, It was invaluable. |
@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. |
@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. |
Updated from 3.10.12 in most cases, sometimes from 4.3.4 when tests did not require 3.10.12. Using MySQLi. |
I have tested this item ✅ successfully on ba11ab4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41367. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41367. |
* [4.3] fix localised cli installation (#41706) * [4.3] Work around a successful upgrade with silent errors (#41367) * Log all * Capture errors * Log messages * Error collector * Error collector * More error collecting * More error collecting * Show errors * Cli command check errors * phpcs * phpcs * Back button and better text * phpcs * logs * Make sure logging is working before continue * Apply suggestions from code review --------- Co-authored-by: Richard Fath <richard67@users.noreply.github.com> Co-authored-by: Harald Leithner <leithner@itronic.at> * cs --------- Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com> Co-authored-by: Fedir Zinchuk <getthesite@gmail.com> Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
* [4.4] Upmerge (#41733) * [4.3] fix localised cli installation (#41706) * [4.3] Work around a successful upgrade with silent errors (#41367) * Log all * Capture errors * Log messages * Error collector * Error collector * More error collecting * More error collecting * Show errors * Cli command check errors * phpcs * phpcs * Back button and better text * phpcs * logs * Make sure logging is working before continue * Apply suggestions from code review --------- Co-authored-by: Richard Fath <richard67@users.noreply.github.com> Co-authored-by: Harald Leithner <leithner@itronic.at> * cs --------- Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com> Co-authored-by: Fedir Zinchuk <getthesite@gmail.com> Co-authored-by: Richard Fath <richard67@users.noreply.github.com> * [4.3] Fix untranslated constants in script.php (#41734) * restrict choicesjs input on container (#41478) * [4.4] Add Joomla 5 compat plugin to 4.4.0 to fix class "JPlugin" not found error on update to 5 (#41738) * Add J5 compat plugin to 4.4.0 * Adapt provider.php to latest changes in 5.0-dev * Add to ExtensionsHelper * Use protected = 0 * Better XML description. * Use correct creationDate in XML file Co-authored-by: Brian Teeman <brian@teeman.net> --------- Co-authored-by: Brian Teeman <brian@teeman.net> * [4.4] Smart Search: Limiting highlighting of tokens (#41463) * Smart Search: Limiting highlighting of tokens * Update components/com_finder/tmpl/search/default_results.php Co-authored-by: Quy <quy@nomonkeybiz.com> * Adding inline help text --------- Co-authored-by: Quy <quy@nomonkeybiz.com> Co-authored-by: Olivier Buisard <olivier.buisard@simplifyyourweb.com> Co-authored-by: Allon Moritz <allon.moritz@digital-peak.com> Co-authored-by: Martin Carl Kopp <6154099+MacJoom@users.noreply.github.com> * No longer a code owner build.xml is part of the testing team stuff and the rest I’m removing myself from * Fields is maintained by everybody now * Invalid import from upmerge * Set the configuration options through a command (#41787) * Set the configuration options through a command * cs * PHP 8.2 Creation of dynamic property (#41554) * Fix deprecated message in categories (#41587) * [4.3] fix finder common words (#41468) * [4.3] Fix missing filter for subject (#41726) * Fixed database upmerge conflicts * Update installation/sql/mysql/base.sql Co-authored-by: Richard Fath <richard67@users.noreply.github.com> --------- Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com> Co-authored-by: Fedir Zinchuk <getthesite@gmail.com> Co-authored-by: Richard Fath <richard67@users.noreply.github.com> Co-authored-by: Magnus Singer <work@magnus-singer.com> Co-authored-by: Brian Teeman <brian@teeman.net> Co-authored-by: Hannes Papenberg <info@joomlager.de> Co-authored-by: Quy <quy@nomonkeybiz.com> Co-authored-by: Olivier Buisard <olivier.buisard@simplifyyourweb.com> Co-authored-by: Allon Moritz <allon.moritz@digital-peak.com> Co-authored-by: Martin Carl Kopp <6154099+MacJoom@users.noreply.github.com> Co-authored-by: George Wilson <georgejameswilson@googlemail.com> Co-authored-by: Denitz <197527+Denitz@users.noreply.github.com> Co-authored-by: Christiane Maier-Stadtherr <dev@chmst.de>
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
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
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.
Addittionaly, try to add
throw new Exception('Update test error');
in coupe of upgrade methods inadministrator/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'.