-
-
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
[5.1] Joomla Update: Improving error handling when writing files #41096
Conversation
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. |
@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 |
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 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. |
@viocassel |
I have tested this item ✅ successfully on c9f55bb This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41096. |
Thanks you! |
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 simpleif
should be enough here.is_string($unzipData)
gzinflate()
andbzdecompress()
both return a string in case of success. In case of an error, the return value is eitherfalse
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()
returnsfalse
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