Skip to content

Comments

[4.4] Converting File class from CMS to framework package#40257

Merged
laoneo merged 3 commits intojoomla:4.4-devfrom
Hackwar:4.4-filesystem-file-delete
Apr 3, 2023
Merged

[4.4] Converting File class from CMS to framework package#40257
laoneo merged 3 commits intojoomla:4.4-devfrom
Hackwar:4.4-filesystem-file-delete

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Mar 30, 2023

Summary of Changes

This PR changes the Joomla\CMS\Filesystem\File class over to Joomla\Filesystem\File where possible without any further changes. The remaining 12 files using the CMS class are using methods which aren't in the framework class or are in there differently.

Testing Instructions

Codereview

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

@laoneo
Copy link
Member

laoneo commented Mar 31, 2023

This needs some proper testing as the system tests are crashing on various places. Really I would split this pr file by file and add proper testing instructions, just to be sure we do not break something.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 31, 2023

The used code in the File classes are identical.

@laoneo
Copy link
Member

laoneo commented Mar 31, 2023

Why are then the system tests failing on various places?

@sandewt
Copy link
Contributor

sandewt commented Mar 31, 2023

Cypress, local test: test was NOT scuccessful

Before patch:

pr_40257_before

After patch: and so forth
pr_40257_after

@sandewt
Copy link
Contributor

sandewt commented Mar 31, 2023

Some additional information:

pr_40257_after-2

@Hackwar
Copy link
Member Author

Hackwar commented Mar 31, 2023

I reverted one file which had also calls to File::getExt(). That makes this all pass again.

@laoneo laoneo merged commit 7a8b628 into joomla:4.4-dev Apr 3, 2023
@laoneo
Copy link
Member

laoneo commented Apr 3, 2023

Thanks!

@laoneo laoneo added this to the Joomla! 4.4.0 milestone Apr 3, 2023
@Hackwar Hackwar deleted the 4.4-filesystem-file-delete branch April 8, 2023 21:16
@Hackwar Hackwar mentioned this pull request May 11, 2023
4 tasks
@Elfangor93
Copy link
Contributor

Since this PR is merged some of my extensions can not be updated anymore using the Joomla installer.

Reason

Joomla\CMS\Filesystem\File::delete() just ignores files which should be deleted but do not exist. Joomla\Filesystem\File::delete() on the other hand throws an error. Therefore update scripts which delete some files already in the preflight suddenly do not work anymore. They stop in the middle of the updating process and outputting "Joomla\Filesystem\File::delete(): Failed deleting inaccessible file ".

@Hackwar @laoneo Do you think, should we update Joomla\Filesystem\File::delete() to ignore non existing files instead of throwing an error? From my point of view it is not necessary to throw an error if you try to delete a file which is not existent.

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.

5 participants