User tests: Successful: Unsuccessful:
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
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.
Labels |
Added:
?
|
I agree with @joomdonation. Also much of that "difference" used by CMS itself.
Deprecate without a proper replacemen is a bad idea.
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.
Maaan, that is your PR you should have been checked what consequences it can have for CMS
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.
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.
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.
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").
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.
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
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.
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.
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-04-14 07:34:32 |
Closed_By | ⇒ | laoneo |
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 theupload
method in framework package.Just name of a few by looking at it quickly. What will we have to do with these differences/changes?