? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
8 Apr 2022

Summary of Changes

This pr deprecates the CMS filesystem package in favor of the framework one. Replacing the references of the CMS filesystem library should be done in followup pr's as a search and replace doesn't really work on all classes and functions.

Followup of #33390.

Only code review.

avatar laoneo laoneo - open - 8 Apr 2022
avatar laoneo laoneo - change - 8 Apr 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Apr 2022
Category Libraries
avatar laoneo laoneo - change - 8 Apr 2022
The description was changed
avatar laoneo laoneo - edited - 8 Apr 2022
avatar joomdonation
joomdonation - comment - 8 Apr 2022

The two packages are not the same, so if we deprecate the CMS package like this, it will cause trouble for developer using the methods from CMS package which are not available in the Framework package. For example, when I look at the File class for the methods which I am using in my extensions, there are already difference:

  • getExt, exists methods are not available in the framework package.
  • upload method in the CMS package has different signature (thus different logic) with the upload method in framework package.

Just name of a few by looking at it quickly. What will we have to do with these differences/changes?

avatar laoneo
laoneo - comment - 8 Apr 2022

That's why I wrote, we can't just search and replace. You have to check it cases by case and the same must be done by the extension developers. That's why we deprecate it now so all have a enough time to migrate. For the functions which do not exist in the framework package we must add a special deprecate note. If there is a different signature, then I guess the developer can figure out by himself how to do the transition.

avatar laoneo laoneo - change - 8 Apr 2022
Labels Added: ?
avatar Fedik
Fedik - comment - 8 Apr 2022

I agree with @joomdonation. Also much of that "difference" used by CMS itself.
Deprecate without a proper replacemen is a bad idea.

avatar laoneo
laoneo - comment - 8 Apr 2022

What has no replacement? You have to become specific, such general statements doesn't bring any value. There is for every deprecation a replacement (except for the helper) in this pr and when individual functions are removed, then we can add a specific deprecate message.

avatar Fedik
Fedik - comment - 9 Apr 2022

Maaan, that is your PR you should have been checked what consequences it can have for CMS ?

Here is my list (I did not checked sub-classes):

File:
getExt
append
exists
upload - have a criticall difference, and cannot be replaced directly
invalidateFileCache - non criticall difference
canFlushFileCache

Folder:
exists

Helper:
fileUploadMaxSize
parseSize
parseSizeUnit

Patcher:
reset

Path:
removeRoot

Stream:
filesize

ClientHelper class not exists.


What I meant, that it should be first replaced in CMS, to make sure all works correctly, so we can mark it as deprecated safely.
Not like: we mark it as deprecated and later figure out what to do.

Addittionaly, because it is static classes, each method should trigger E_USER_DEPRECATED:

@trigger_error(
	sprintf('%s is deprecated. Use Joomla\Filesystem\Foo::bar() instead.', __METHOD__),
	E_USER_DEPRECATED
);

For non static classes it should be enought to trigger it once in class constructor.

avatar laoneo
laoneo - comment - 9 Apr 2022

The only reason we left the CMS filesystem package in core for 4 was because of the FTP layer. We did it many times that we deprecated stuff and then replaced it afterwards, see JFactory functions or *::getInstance(). As I wrote in the description, when this is merged, then we need to replace it with the framework package on a case by case basis to see nothing breaks. What I want, is to have one way to use the filesystem and not duplicated stuff.

avatar brianteeman
brianteeman - comment - 9 Apr 2022

The only reason we left the CMS filesystem package in core for 4 was because of the FTP layer.

Really? The report by @Fedik suggests otherwise

As I wrote in the description, when this is merged, then we need to replace it with the framework package on a case by case basis to see nothing breaks.

As per @nibra previous comments about deprecations this PR should also replace all the usage in the core of the CMS filesystem layer with the framework library. The core should not be using anything that is deprecated.

avatar Fedik
Fedik - comment - 9 Apr 2022

The only reason we left the CMS filesystem package in core for 4 was because of the FTP layer.

Yeah, that probably was a general reason. However if look in deep there a "little more details".

I more concerning about File::upload(), this also kind of "security changes".
A rest can be handled easily somehow (but better "before" than "after").

avatar laoneo
laoneo - comment - 9 Apr 2022

The only reason we left the CMS filesystem package in core for 4 was because of the FTP layer.

Really? The report by @Fedik suggests otherwise

As I wrote in the description, when this is merged, then we need to replace it with the framework package on a case by case basis to see nothing breaks.

As per @nibra previous comments about deprecations this PR should also replace all the usage in the core of the CMS filesystem layer with the framework library. The core should not be using anything that is deprecated.

I general I do agree when single functions are deprecated, but here we deprecate a full namespace. If I would replace every occurrence in all the 120 files then the pr is not testable. So I suggest to deprecate it first and then replace it step by step. I really don't understand why we have again to discus this back and forth instead of just going forward.

avatar brianteeman
brianteeman - comment - 9 Apr 2022

I really don't understand why we have again to discus this back and forth instead of just going forward

Where was the previous discussion and decision

avatar Bakual
Bakual - comment - 9 Apr 2022

We should not deprecate things we still use ourself. That's imho a bad practice we used in the past.
Replace the usage and then deprecate is the better approach. Otherwise we have deprecation warnings in the code all over the place which is bad.

avatar Fedik
Fedik - comment - 10 Apr 2022

If I would replace every occurrence in all the 120 files then the pr is not testable. So I suggest to deprecate it first and then replace it step by step.

I thought about it again :)
You can (if you want) do step by step in parallel PR(s), replace methods that does not use FTP, like:

File::getExt
File::stripExt
File::makeSafe
File::invalidateFileCache
File::canFlushFileCache
File::exists

Folder::exists
Folder::files
Folder::folders
Folder::listFolderTree
Folder::makeSafe

etc.

Because we cannot drop FTP on the middle. Then we can merge this PR to deprecate a rest.
But with exception, we cannot deprecate File::upload because there no direct replacement, and so File class also cannot deprecate, only methods in it.

And still: every public static method should trigger E_USER_DEPRECATED, even when whole class is deprecated (because it is static classes).

@trigger_error(
	sprintf('%s is deprecated. Use Joomla\Filesystem\Foo::bar() instead.', __METHOD__),
	E_USER_DEPRECATED
);

With info what to use instead.
Or we end up with stuff like this #35380 again.

That I think will be better way to go.

avatar laoneo
laoneo - comment - 11 Apr 2022

The problem is that pr's get tested very slowly, especially from me. So when I want to do all these pr's it will take ages and when I do afterwards the deprecation, then we arrive at Joomla 7 with the filesystem deprecated. So better to do it now, soe extension developers can clearly see that they shouldn't use the CMS filesystem package anymore and we have enough time to do the replacement.

avatar laoneo laoneo - change - 14 Apr 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-04-14 07:34:32
Closed_By laoneo
avatar laoneo laoneo - close - 14 Apr 2022
avatar Fedik
Fedik - comment - 16 Apr 2022

@laoneo if you will decide to make such PRs, ping me, I try to test,
Ideally a couple small ones: one for Files, one for Folder so on.

Add a Comment

Login with GitHub to post a comment