WordPress.org

Make WordPress Core

Opened 10 years ago

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

Download all attachments as: .zip

Change History (20)

#1 @kurtpayne
10 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.

#2 follow-up: @nacin
10 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.

#3 in reply to: ↑ 2 @kurtpayne
10 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.

#4 @GaryJ
10 years ago

  • Cc gary@… added

@kurtpayne
10 years ago

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

#5 @dd32
9 years 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

#6 @dd32
8 years 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.

#7 @dd32
8 years 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.

#8 @dd32
8 years ago

  • Keywords filesystem added

@DrewAPicture
8 years ago

docs cleanup, first-pass

#9 @DrewAPicture
8 years 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 years ago by DrewAPicture (previous) (diff)

#10 @DrewAPicture
8 years ago

  • Cc xoodrew@… added

#11 @DrewAPicture
8 years ago

  • Keywords needs-refresh removed

@DrewAPicture
8 years ago

Refresh at r25558

@DrewAPicture
8 years ago

Remaining docs mentioned in comment:9

#12 @DrewAPicture
8 years 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

@DrewAPicture
8 years ago

@since Unknown => 2.5.0

#13 @dd32
8 years 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

#14 @nacin
8 years 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.