[buddypress-trac] [BuddyPress Trac] #6004: Move Avatar local management into new Attachments component
noreply at wordpress.org
Tue Nov 11 01:25:25 UTC 2014
#6004: Move Avatar local management into new Attachments component
Reporter: imath | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: 2.2
Component: Avatars | Version:
Severity: normal | Resolution:
Keywords: has-patch dev-feedback |
Comment (by boonebgorges):
imath - Thanks for your continued work on this. I've done some testing,
and functionally, I think the experience is pretty good.
I have some high-level concerns about the technical architecture, though.
They fall into roughly three categories, which I'll go into a bit below.
These are all meant as discussion points - I'm not completely convinced
about any of this, and would welcome some feedback from other devs.
1. __Attachments shouldn't be a component.__
My biggest worry is that we're thinking about attachments all wrong.
They're not like Groups, Friends, Messages, etc in a couple ways.
Attachments doesn't stand on its own; it only works in conjunction with
other components. Attachments doesn't need any of the stuff that
`BP_Component` provides - Attachments don't have their own navigation,
admin bar items, not much need for global data. And under what
circumstances would someone ever need a UI to *turn off* attachment
functionality? Since avatars throughout BP (among other things, in the
long run) will require this new functionality, we'd suddenly be forced to
deal with all sorts of component dependency issues: enabling Groups would
mean that you'd need Attachments, and so forth. It seems like the wrong
tool for the job.
A better model, IMO, is Attachments as "library" rather than "component".
In bp-core, we would provide some basic API-level functions - the
BP_Attachment class, wrappers for the CRUD functions, and (maybe) some of
the shared logic for handling form submits and attachment saving. Then,
individual components that want to implement attachments for one reason or
another will do most of the work in their own components - think, for
instance, of attachments for private messages. In a way, the case of
avatars is a complex one to use as a prototype, since various components
will have slightly different implementations of avatars, and avatars
themselves are a specific implementation of attachments. So we really have
*two* levels of abstraction in this case, one for the general avatar logic
(probably in bp-core-avatars) and one for each component's specific
implementation (ie, Groups screen functions). A lot of this is blended
together in your patch, and I think it will be conceptually clearer to try
to keep it separate - especially as we attempt to build other things in BP
that leverage the central Attachments functionality.
2. __We're doing too much.__
We should be pretty careful about trying to do too much at once.
Introducing a new component, moving large amounts of existing code,
deprecating large amounts of stuff - this is pretty serious work. Once we
release a version of BP with these changes in place, we will always have
to provide backward compatible support for them. And this is before we're
really sure what plugin authors are going to do with the API functions
I think we should try to be more modest. For example, you've deprecated
much of the avatar form-handling logic, and moved it into the attachments
component. This is a way of encouraging developers to reuse the code for
something other than avatars. But it's not clear to me that this is
something we want to encourage at all - it'd require lots of jumping
through hoops for the plugin dev, and then we'd have to make sure we
didn't break all of that customization on the next upgrade to BuddyPress.
A more modest approach (in keeping with my first point) is to think of
avatar uploads as *a specific implementation of* attachments, and to leave
most of the avatar-specific logic where it currently is. We may end up
tearing out the guts of, say, `bp_core_handle_avatar_upload()` - that's
totally fine. But by leaving it in bp-core-avatars.php, it's a way of
saying to plugin authors: for now, you will have to build your own version
of this logic.
Then, over time, as we start to get a handle on what the actual use cases
of attachments will be, we can start abstracting out small bits of the
functionality baked into avatars (and whatever implementations we decide
to do in core). But this is something we can roll out fairly slowly and
3. __We're relying too much on wp.media.__
While there are some small workflow changes I would suggest, I like the
work that you've done extending and modifying `wp.media`. But I'm also a
little worried about it. When I look at how that codebase has evolved in
the last few versions of WP, I'm not convinced that it's stable enough for
us to be modding in this way, at least without having to reinvent the
wheel with each WP release. Moreover, wp.media arguably does *way more*
than we need, at least for avatar uploads - so we're essentially taking
this huge, fancy application, and turning most of it off. That seems
strange to me.
At WCSF, Clean-Cole started looking into working on this problem from the
opposite direction: building a custom JS app for BP uploads that would be
purpose-built, rather than tweaking wp.media. This is not to say that we
wouldn't use some of the core tools - we shouldn't try to reinvent WP's
implementation of pluploader, for instance - but starting from scratch
with data models and views that suit our specific purposes might
ultimately result in a codebase that'll be easier to maintain and more
reusable. Maybe Cole will chime in here with some thoughts (nudge nudge).
To go back to the beginning: imath, thanks for working on this. I don't
want my comments to sound too unpleasant. I think what you've written here
is ideal for a plugin, but I think we need to be more cautious in BP about
the architecture that we commit to. You have done a huge amount of the
initial work necessary to sketch out the kinds of modifications we'll need
to do in order to make WP's internal attachments APIs work for our
purposes. Donc, merci et bravo :)
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6004#comment:1>
BuddyPress Trac <http://buddypress.org/>
More information about the buddypress-trac