[#42533] - [4.4] File::delete() throws error and terminates installation/update/uninstall
- Fixed in Code Base
- 19 Dec 2023
- Medium
- Build: 4.4-dev
- # 42533
- Diff
- Elfangor93:fix-issue-42495
User tests: Successful: 2 alikon, rowi68 Unsuccessful: 0
With PR #40257 the File::delete() method in the framework was changed. It now throws a PHP error if it is called on a non-existing file path.
This behavior prevents installation, update and uninstall scripts of extensions from beeing executed correctly since the whole execution gets terminated as soon as Joomla\CMS\Installer tries to delete a file which does not exist anymore.
Pull Request for Issue #42495.
Summary of Changes
All occurrences of File::delete() in Joomla\CMS\Installer are first checked to see if the provided path exists and is a file by using the PHP built-in function if (is_file()) {File::delete()}.
Testing Instructions
- Create a fresh Joomla v4.4.x installation featuring the changes of this PR
- Install com_blank v1.0.0 (J3mainTest branch). This is a version of the component with an old MVC structure.
- Update com_blank v1.0.1 (updateScriptTest branch). This is a version of the component with the new MVC structure.
Actual result BEFORE applying this Pull Request
Update terminates and a message appears stating Joomla\Filesystem\File::delete: Failed deleting inaccessible file controller.php and Error installing component
After this unsuccessful update, uninstalling partially removed the component also does not work anymore.
It states Component Uninstall: Can't uninstall. Please remove manually. and Error uninstalling component.
Expected result AFTER applying this Pull Request
Update runs through without warning or error for non-existing files.
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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
PR-4.4-dev
|
||
but with the patch applied I still have the same error
@alikon The message you get with com_patchtester is a warning telling that a folder which should be deleted during the install/update is not existent. This gives you a warning, but is does not terminate the script. With non-existing files it is different:
Before applying this PR the install/update script was terminated and the installation was not successful leaving you behind with a partially installed extension which has to be removed by hand.
Deleting of non-existent folders is not scope of this PR since it "just" outputs you warning messages. In worst case you will get a huge number of messages telling all the different folders which could not be deleted. But the installation/update was still successful and the system is still healthy and usable.
I updated the initial comment of this PR to show you the error you get before applying this PR.
There you see a bunch of warnings because of non-existent folders and one warning followed by an error because of a non-existent file. The non-existent file terminated the installation completely!
For all the testers...
Prebuilt download packages of this PR are available here https://artifacts.joomla.org/drone/joomla/joomla-cms/4.4-dev/42533/downloads.
Use this packages to setup your fresh Joomla installation (step 1).
I think we should rather fix the Folder class as this is a BC break when a libraries function all of the sudden throws an exception instead of returning false instead.
I have tested this item ✅ successfully on ccb0e59
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42533.
I think we should rather fix the
Folderclass as this is a BC break when a libraries function all of the sudden throws an exception instead of returning false instead.
The problem was the File class, not the Folder class, and the problöem came from changing the CMS code to use the framework library instead of the CMS one. To change the framework library now so it behaves like the CMS library did would mean a b/c break for the framework library.
@Elfangor93 i've got your point now
thanks for the clarification
I have tested this item ✅ successfully on ccb0e59
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42533.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42533.
I think we should rather fix the
Folderclass as this is a BC break when a libraries function all of the sudden throws an exception instead of returning false instead.The problem was the
Fileclass, not theFolderclass, and the problöem came from changing the CMS code to use the framework library instead of the CMS one. To change the framework library now so it behaves like the CMS library did would mean a b/c break for the framework library.
So for me is sounds more reasonable to add a delete function back to the CMS class, call the base class with a try and catch. Like that is the BC break fixed. Or?
I think we should rather fix the
Folderclass as this is a BC break when a libraries function all of the sudden throws an exception instead of returning false instead.The problem was the
Fileclass, not theFolderclass, and the problöem came from changing the CMS code to use the framework library instead of the CMS one. To change the framework library now so it behaves like the CMS library did would mean a b/c break for the framework library.So for me is sounds more reasonable to add a delete function back to the CMS class, call the base class with a try and catch. Like that is the BC break fixed. Or?
@laoneo Sorry I don’t get it. You want to change the code so it uses the CMS class again and that one shall call the framework class with a try catch? Sorry but that is unnecessarily complicated, and requiring that from this PR here is a perfect example of how to stop a good PR with no reason.
P.S.: As far as I remember we want to come away from the CMS‘ filesystem library at all so we can throw it away one day, so using that again would be a step back.
Ah..., forget what I said. I thought that the File::delete from the CMS got remove and we work directly on the fw class. But all what was done is to change the namespace. So fine for me.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-12-19 08:25:41 |
| Closed_By | ⇒ | laoneo | |
| Labels |
Added:
RTC
bug
|
||
Sorry, for the noise. All good here. Thanks for the fix!
i've replicated the issue using com_patchtester 4.3.1
but with the patch applied I still have the same error