[#41096] - [5.1] Joomla Update: Improving error handling when writing files
- Fixed in Code Base
- 7 Dec 2023
- Medium
- Build: 5.0-dev
- # 41096
- Diff
- Hackwar:5.0-joomlaupdate-1
User tests: Successful: Unsuccessful:
Summary of Changes
In several support channels we do encounter sites where the files are not consistently on one version of Joomla. It seems as if updating somehow aborted midway or for specific files. This PR is an effort to review the code and find possible reasons for this behavior.
While loop
The while loop in line 1530 is not necessary. PHP is strictly linear and thus there is nothing which could change the $zipData during the runtime of the loop. In a theoretical worst case, this could actually create an infinite loop. Instead a simple if should be enough here.
is_string($unzipData)
gzinflate() and bzdecompress() both return a string in case of success. In case of an error, the return value is either false or an integer. Up till now we are not checking for this. This could for example happen if the compressed data is somehow corrupted. So in case that we didn't get a string as $unzipData, we should abort here.
fwrite()
This is actually the place where shit most likely hits the fan. We are writing unverified data to the file pointer without checking the result and suppressing any errors. I'm actually guessing, that fwrite() would throw at least a notice if $unzipData was not a string (thus decompressing failed) and not write anything, resulting in the previous file not being updated, but which we are suppressing with the @.
We are also not checking if writing of the file somehow failed for other reasons (diskspace full, file pointer somehow corrupted, space rays...) So in this place we are now checking if fwrite() returns false and then throw an error.
Testing Instructions
I was unable to recreate the situations described above and wrote this PR based on code review. Thus I can't provide testing instructions. If someone has corrupted update files, I would be very gratefull if you could attach this to this PR.
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
joomla-cms-bot
- | Category | ⇒ | Administration com_joomlaupdate |
| Status | New | ⇒ | Pending |
@brianteeman I will look into that potential source of problems tomorrow. This PR however concentrates on writing the files to the disc, not on the later stages with deleting files and running SQL updates.
@brianteeman please see #41098
| Labels |
Added:
PR-5.0-dev
|
||
The issue mentioned in @brianteeman 's comment above that updates might fail without the user getting notice of that has been tackled and in my opinion solved with PRs #41367 and #41690 .
I have tested this item ✅ successfully on 0a668f2
I've reviewed the code changes and can confirm that they are correct. In addition, I've successfully tested that a normal update still works.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096.
as said in maintainers that makes more sense to be backported to 4.4 so more tests for 5.0.0beta2 could be made
This pull request has been automatically rebased to 5.1-dev.
I have tested this item ✅ successfully on 0a668f2
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096.
| Labels |
Added:
Feature
|
||
@viocassel How have you applied this PR for testing and on which Joomla version? I'm asking because the PR meanwhile has been rebased from 5.0-dev to 5.1-dev and it had conflicts which I just have solved. If you have tested with 5.0: Could you do a brief retest on 5.1-dev? Thanks in advance. I just see testing instructions say "code review", and the code hasn't really changed since your test, so I will restore the test result. No need to test again.
| Labels |
Added:
PR-5.1-dev
Removed: PR-5.0-dev |
||
I have tested this item ✅ successfully on c9f55bb
Code review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096.
| Title |
|
||||||
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096.
| Labels |
Added:
RTC
|
||
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-12-07 10:44:48 |
| Closed_By | ⇒ | Razzo1987 |
Thanks you!
In my experience (and I had it multiple times today) this problem occurs when the installation doesn't 100% complete the install but the user thinks it has.
In a recent release we added a log of each sql file that is processed. But we still dont log the files AND we dont display anything to a user who does not get a 100% install.
These are what the last few lines of the log looks like currently.
Today I had an upgrade that at first seemed to fail but then a browser refresh made me think it was a success. However when I checked the log I could see that it finished just before the end of the sql section. So no files/folders were deleted.
How many people even know about this log file? How many basic users can even access this file?
My immediate idea would be to have a flag set when an update begins and that flag is only reset when the update is complete. If for any reason that flag has not been reset then a big red warning can be displayed.