Opened 21 months ago
Last modified 11 months ago
#18476 new enhancement
Duplicate code in filesystem classes
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | Filesystem | Version: | 3.2.1 |
| Severity: | minor | Keywords: | dev-feedback |
| Cc: | kurtpayne, gary@… |
Description
Each filesystem class extends a base class and most, but not all, include an implementation of chown. I propose a slight refactoring to include a default implementation of chown in the base class and let each subclass override chown when necessary.
Attachments (2)
Change History (7)
I noticed this recently. The base class is also missing a number of methods that some or all of the others then define. It would be much better if they listed the method as a point of reference for what the WP_Filesystem API is capable of.
Replying to nacin:
I noticed this recently. The base class is also missing a number of methods that some or all of the others then define. It would be much better if they listed the method as a point of reference for what the WP_Filesystem API is capable of.
What if the implementations in the base class were the same as the implementations in the direct access class? This would accomplish the goal of giving a reference implementation for each method. The direct access class would be essentially empty, but open for future extension, and the API would remain intact.
As he who wrote the classes in question:
- The Direct class was written first, and thus is the most fleshed out (without thought as to what methods would be supported over FTP)
- The FTP Sockets class was written second
- The FTP Extension class attempts to mimick the Sockets class
- The Base class had any methods common to both FTP libraries thrown into it.
The classes are not exactly standardised, I've wanted to (on many occasions) rip out all the "useless" methods and functionality which core doesn't use, or isn't implemented in all of them (For example, chown() can't be done via FTP, therefor, the abstraction shouldn't have a method for it on any of them).
I like the idea of using the Direct class as the defacto "goto place" for docs, but feel they should all be documented individually, But I don't see the need to have stubs in the baseclass - It's not a PHP5 Interface, and if all subclasses are documented correctly, there's little need for it IMHO

Sorry, all class-wp-filesystem*.php classes currently do include a chown implementation. This enhancement converges two of them ftpext and ftpsockets into the base class.