[buddypress-trac] [BuddyPress Trac] #6278: Attachment Library
noreply at wordpress.org
Wed Mar 4 16:45:41 UTC 2015
#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
* `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.
* 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
* 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/>
More information about the buddypress-trac