Skip to content
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.0] Joomla Update: Cleanup of file writes in UpdateModel #41097

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jul 1, 2023

Summary of Changes

In UpdateModel of the Joomla Update component, we are doing some write operations on the filesystem as preparation before unpacking the update package. This code contains some unnecessary steps and is missing error handling.

Going through the changes one by one:

  • With the switch from the CMS to the framework File package, the File::delete() now checks if the file exists first and otherwise throws an exception. Thus we either have to wrap it in is_file() or catch the exception.
  • We then try to write the update package file to our filesystem, but we never check if that was successfull.
  • Trying to delete the update.php, File::delete() does the same as unlink() now and already does the opcache invalidation.
  • Since Joomla 4 doesn't have the FTP layer anymore, we can simplify the last codeblock greatly.

Testing Instructions

This code change resulted from manual code review and is not based on a specific error condition. Simply said, updating Joomla should still work the same as before.

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

@richard67
Copy link
Member

richard67 commented Jul 1, 2023

  • With the switch from the CMS to the framework File package, the File::delete() now checks if the file exists first and otherwise throws an exception. Thus we either have to wrap it in is_file() or catch the exception.

@Hackwar I think the same applies to deleting files and folders on update in script.php. Of course something for another PR. Is that right?

Update: I see we have already an is_file check there for the files so that should be ok. Not sure about the folders, will check later.

@HLeithner HLeithner merged commit d7cb349 into joomla:5.0-dev Jul 1, 2023
3 checks passed
@HLeithner
Copy link
Member

Should be easier to test this when it's in nightlies

@Hackwar Hackwar deleted the 5.0-joomlaupdate-2 branch March 22, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants