[buddypress-trac] [BuddyPress Trac] #6004: Move Avatar local management into new Attachments component

[buddypress-trac]

#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
 we're providing.

 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
 conservatively.

 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/>
BuddyPress Trac