Skip to content

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Mar 13, 2023

Summary of Changes

This PR extends the CMS File class from the framework Filesystem File class. This removes all duplicate code from the CMS class, using the framework methods where possible. This means that now this class throws exceptions and that all FTP remnants are removed.
Looking at the remaining code, we should consider moving some of this into the framework as well. We probably should deprecate canFlushFileCache() as well.
I've changed this a bit agressively, no remaining method stubs with deprecation notices, no hints at the removed FTP code, but I hope that this is still okay.

Testing Instructions

Everything should work like before... I think this should rather be done by 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

@HLeithner
Copy link
Member

same as Folder, can't be done this way.

Also we don't use underscore for class names, in this case we use PascalCase

@laoneo
Copy link
Member

laoneo commented Mar 14, 2023

Is there some code which needs to stay in the CSM File class? From a quick glance most of it can be moved to the framework package. Then I would even go a step further and deprecate the whole class.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

I'm working on a deprecation PR for the whole filesystem package in the CMS library for 4.4. Will take me some time. Most likely I will have to re-do these PRs here then again.

@laoneo
Copy link
Member

laoneo commented Mar 14, 2023

I tried it already #37518. It is a hell lot of work to change the core.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

Yes, I know. But we have to do it at some point and 4.4 is the point.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

The comments in your PR are also something that we have to fix in the framework package then.

@Hackwar Hackwar marked this pull request as draft March 23, 2023 10:07
@Hackwar
Copy link
Member Author

Hackwar commented Apr 3, 2023

I did some further cleanup and copied code over to the framework Filesystem package. This means that for this to be able to be merged, joomla-framework/filesystem#54 and joomla-framework/filesystem#55 need to be merged first. This leaves the upload() method as the last one which we need to replace to really completely remove this class in 6.0.

@Hackwar Hackwar marked this pull request as ready for review April 3, 2023 22:51
@HLeithner HLeithner added Feature b/c break This item changes the behavior in an incompatible why. HEADS UP labels Apr 7, 2023
@Hackwar Hackwar closed this Apr 18, 2023
@Hackwar Hackwar reopened this Apr 18, 2023
@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:50
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.1-dev.

@Hackwar Hackwar changed the title [5.0] Extending Joomla\CMS\Filesystem\File from Joomla\Filesystem\File [5.1] Extending Joomla\CMS\Filesystem\File from Joomla\Filesystem\File Jan 2, 2024
@Hackwar
Copy link
Member Author

Hackwar commented Feb 25, 2024

I doubt this will ever be merged, so I'm closing this.

@Hackwar Hackwar closed this Feb 25, 2024
@Hackwar Hackwar deleted the 5.0-filesystem-file branch February 25, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

b/c break This item changes the behavior in an incompatible why. HEADS UP Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants