[buddypress-trac] [BuddyPress Trac] #6278: Attachment Library

[buddypress-trac]

#6278: Attachment Library
------------------------------------+------------------
 Reporter:  imath                   |       Owner:
     Type:  enhancement             |      Status:  new
 Priority:  normal                  |   Milestone:  2.3
Component:  API                     |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  has-patch dev-feedback  |
------------------------------------+------------------

Comment (by johnjamesjacoby):

 I agree with Boone, that a flexible base class that the other components
 can subclass is ideal for a few reasons.

 * A general `BP_Attachment` class that accepts an array of arguments in
 its constructor, the way you have it now. This class should not mention
 (or care about) any existing avatar code (yet) and likely contain generic
 CRUD actions for interfacing with attachments.
 * Any component wanting Attachments extend `BP_Attachment` passing in an
 array of: file-system requirements (folders, dimensions, file-sizes,
 maximum allowed?), identifiers for namespacing, etc... This subclass would
 include any special interface methods, and could also potentially override
 any parent class interface methods for unique attachment handling, though
 ideally `BP_Attachment` is robust enough where this is not a requirement
 for extending.
 * `buddypress()->avatar` has always been our `stdClass` for stashing
 sizes, locations, path, & url. We'll want to maintain backwards
 compatibility here, as much I would enjoy gutting it. Several components
 touch these class values directly, and any component that has cloned the
 Groups component in the past is undoubtedly still doing the same.

 Related observations:

 * Perhaps we have a `BP_Attachment_Avatar` subclass of `BP_Attachment` and
 then also have `BP_Attachment_XProfile_Avatar` and
 `BP_Attachment_Group_Avatar` extend `BP_Attachment_Avatar`. This way all
 of the "avatar" specific functionality currently in `BP_Attachment` can
 live in an avatar-specific place that is easily extended in third-party
 components like Events, shopping carts, and ...gasp... maybe even our own
 Blogs component finally gets to play.
 * The main `BP_Attachment` parent class should error trap `wp_mkdir_p()`
 in a user friendly way. Right now we just `¯\_(ツ)_/¯` and keep going,
 witch isn't very helpful if a file-system issue occurs when an unassuming
 community member attempts to set their avatar.
 * See: `groups_avatar_upload_dir()`, `bp_core_signup_avatar_upload_dir()`,
 and `xprofile_avatar_upload_dir()` respectively.
 * The three functions mentioned above would either be deprecated in favor
 of (or act as wrappers for) methods in the subclasses mentioned earlier.
 This way they all take advantage of any `wp_mkdir_p()` handling similar to
 how you have `upload_dir_filter()` in the patch.
 * One thing Boone eluded to, but did not explicitly point out, is by
 encapsulating variables and methods in the classes, we avoid passing
 `$upload_dir_filter` around into the `upload()` method.
 `bp_core_avatar_handle_upload()` would need to stay how you've patched it
 for backpat.
 * Maybe we should add a note to the `bp_attachment_upload_dir` filter
 pointing out that it's only for short-circuiting rather than extending. I
 can envision someone less familiar with OOP thinking they just hook in
 there and more easily introduce new upload paths.
 * Should we be calling `remove_all_filters( 'upload_dir' )` in our
 `upload()` method? This filter makes me nervous since so many things touch
 it, that I'm afraid something else will be hooked in and break it.

 More specific feedback about `BP_Attachment_Avatar`:
 * Move variable defaults from `BP_Attachment` into it
 * Move avatar specific component check in constructor into it (without the
 `component` check obviously)
 * Move avatar defaults in `set_upload_dir()` into it
 * Copy the `validate()` method into it, and maybe we have more generic
 file validation in `BP_Attachment`?

 @imath - this is outstanding work. You've done the bulk of the heavy
 lifting, and we are well on our way to getting this into 2.3 very early,
 which is hugely excellent.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6278#comment:4>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac