WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

#18476 closed enhancement (fixed)

Duplicate code in filesystem classes

Reported by: kurtpayne Owned by: kurtpayne
Milestone: 3.7 Priority: normal
Severity: minor Version: 3.2.1
Component: Inline Docs Keywords: filesystem has-patch commit
Focuses: Cc:

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 (6)

filesystem.diff (2.7 KB) - added by kurtpayne 3 years ago.
filesystem2.diff (9.5 KB) - added by kurtpayne 3 years ago.
More verbose implementation of class-wp-filesystem-base.php
18476.diff (18.1 KB) - added by DrewAPicture 8 months ago.
docs cleanup, first-pass
18476.2.diff (18.9 KB) - added by DrewAPicture 7 months ago.
Refresh at r25558
18476.3.diff (22.1 KB) - added by DrewAPicture 7 months ago.
Remaining docs mentioned in comment:9
18476.4.diff (22.1 KB) - added by DrewAPicture 7 months ago.
@since Unknown => 2.5.0

Download all attachments as: .zip

Change History (20)

kurtpayne3 years ago

comment:1 kurtpayne3 years ago

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.

comment:2 follow-up: nacin3 years ago

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.

comment:3 in reply to: ↑ 2 kurtpayne3 years ago

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.

comment:4 GaryJ3 years ago

  • Cc gary@… added

kurtpayne3 years ago

More verbose implementation of class-wp-filesystem-base.php

comment:5 dd3222 months ago

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

comment:6 dd328 months ago

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

I retract this statement :)

I like attachment:filesystem2.diff​ and think it'd be the best place to document the expected behaviour of the subclasses.

comment:7 dd328 months ago

  • Component changed from Filesystem to Inline Docs
  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.7

This would go well with the Filesystem and Inline docs cleanups in 3.7.

The patch, although it'd still apply, needs some cleanup for todays docs standards.

comment:8 dd328 months ago

  • Keywords filesystem added

DrewAPicture8 months ago

docs cleanup, first-pass

comment:9 DrewAPicture8 months ago

18476.diff is a first-pass docs cleanup and refresh

Still need clarified parameter and return descriptions for:

::copy(),
::move(),
::delete(),
::exists(),
::is_file(),
::is_dir(),
::is_readable(),
::is_writeable(),
::atime(),
::mtime(),
::size(),
::touch(),
::mkdir(),
::rmdir(),
::dirlist()
Last edited 8 months ago by DrewAPicture (previous) (diff)

comment:10 DrewAPicture8 months ago

  • Cc xoodrew@… added

comment:11 DrewAPicture8 months ago

  • Keywords needs-refresh removed

DrewAPicture7 months ago

Refresh at r25558

DrewAPicture7 months ago

Remaining docs mentioned in comment:9

comment:12 DrewAPicture7 months ago

  • Keywords has-patch commit added

18476.3.diff finishes the docs for methods mentioned in comment:9.

The patch includes some docs copied from a patch on #23122, props bananastalktome

DrewAPicture7 months ago

@since Unknown => 2.5.0

comment:13 dd327 months ago

In 25560:

First pass at documenting the WP_Filesystem methods. This also introduces stubs of the methods into the base class which are documented, which subclasses can override, some methods were cleaned up at the same time.
See #18476 See #23122. Props kurtpayne, bananastalktome, and, DrewAPicture

comment:14 nacin7 months ago

  • Resolution set to fixed
  • Status changed from new to closed

This seems done. Please reopen if I'm wrong.

Note: See TracTickets for help on using tickets.