Move Avatar local management into new Attachments component

[imath]

Relative to #5429

This is a first pass to suggest a way to bring the Attachments into core. Following the discussions team had at WCSF, this patch is :

1. creating a new component: Attachements
2. moving functions relative to avatar local management into it (upload/crop/delete) for the xProfile and Groups components
3. adding 2 new visible features :
   • managing group's avatar within the WordPress Group Administration screen (upload/crop/delete)
   • managing user's avatar within the WordPress Extended Profile Administration screen (upload/crop/delete)
4. including the API i've been working on in the ​BP Attachments plugin.

So far i've tested points 1, 2, and 3. Depending on what will happen with this ticket, i plan to create a new branch for the ​BP Attachments plugin to transform it into a demo plugin to explain how to use point 4. So point 4. will surely need some adjustements, i've left some @todos.

I Think, this first step is a nice way to progressively insert some attachments management features. On front-end everything should work like before (which is limiting the risks IMHO), and we're improving some Admin management tools for community managers (deleting, editing or adding an avatar for a group or a user).

The attachments component, just like the notifications one will automatically be activated on upgrade or install. It now holds the bp-disable-avatar-uploads setting. As it's used in one of the legacy templates (bp-legacy/buddypress/members/single/profile/change-avatar.php), if the component is deactivated, i thought it was safer to set this setting to true. I've tried to cover all back compat cases, to help me i've used this ​spreadsheet (but i may have missed some).

Improvements i can see :

• have a different options for each avatar objects (user/group ... blog?)
• function naming (especially in the "BP Attachments Editor", it's the script.js file using WP Backbone Views and Models)
• try to make this more modular, i'm including all files. Some functions are specific to attachments, some others to avatar and some are used by both objects. The "BP Attachments Editor" includes attachments & avatars management...
• in the plugin i was relying on terms to categorise attachments where component id => term slug. I've began using a mapping option just like components page to have component id => term id. I know there have been great improvements in WP Taxonomy API lately, but it may be safer to rely on ids.

NB: The patch is including the patch i've made about the "w" cropping bug #5999 (it was a bit annoying to have the notices and i thought this function would move in the component anyway).

[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 :)

[imath]

boonebgorges - Thanks a lot for your great feedback. It's true it took me a while to put all this together and i really appreciate the words you used in your comment :)

I've learnt a lot doing this work and it gave birth to 2 improvements in the group's navigation area so i'm not feeling like i would have done all this for "nothing". I think the most important is, as you said, to make the right choices about this feature and the users expectations. If this work confirms we shouldn't think about attachments as a component, it's really great too :)

I knew "File" was a difficult object, since the BuddyDrive plugin : i think i kind of really don't like it :) But it's an object that is really really used by people and some are even using it to collaborate ;)

Attachments shouldn't be a component.

I'd say it depends :)

The only component IMHO that stands on its own is the member one. All the others are, at least, dependant of it.

I agree, that each component Groups / Friends / Message, activities .. could use a core library to deal with files, just like it's the case in a way with avatars.

But on the other hand, we would need a great search engine if we are choosing not to rely on a "central" component to organise all this. If i attach a file in a private message, and i want to see the file later on, i'll need to go in the message. Maybe i won't remember what message it was, etc..

I believe an "Attachments" nav in the member's page would be interesting, i know that from there, i can find everything i shared and with what component.

I agree we don't see this in the patch, because in this first step i focused on avatars (which is a file btw, maybe that's why i think of them as attachments to the member's component) so we don't need a nav yet ;)

About "dependency issues: enabling Groups would mean that you'd need Attachments"
I see what you mean, because if we simply disable the option show avatars in WordPress, then in single group's header, Group Admins/mods become invisible :)

Actually i think it's a problem belonging to the groups component. There shouldn't be any problem to use it without avatars.

"And under what circumstances would someone ever need a UI to *turn off* attachment functionality?"
My think about this, is:
Some users might want to completely or temporarly disable uploads, some others to only disable avatar uploads for members, and other to only disable avatar uploads for groups... So i think we should have at least separate options for groups avatars and members avatars. If not an option to completely disable uploads :)

Finally organizing features into components is in a way less risky, if something is wrong with one i'm able to keep on using BuddyPress without it.

We're doing too much.

It's true i've deprecated a lot of avatar functions: so it's really risky :)

wp.media

I think wp.media shouldn't be used on front end. But in backend it can be interesting to use it in order to be as close to WordPress backend UI. But i agree with most of what you said. Having a specific BuddyPress app would be great :)

boonebgorges - "et merci infiniment d'avoir reconnu le travail" :)

[boonebgorges]

I believe an "Attachments" nav in the member's page would be interesting, i know that from there, i can find everything i shared and with what component.

Ah, yes. I agree with this. But, to me, this suggests having an "Attachment" (or "Album" or "Media" or "Files") component, which would create all of the UI, nav, etc, and which would *use* the core attachment API to fetch and store data, but would be separate from it. That is, "Attachments" in this sense (the UI component) would be another specific implementation of "attachments" (the BP API).

Some users might want to completely or temporarly disable uploads, some others to only disable avatar uploads for members, and other to only disable avatar uploads for groups...

Yes, and we can provide filters and/or options for each of these. But this is independent of whether "Attachments" logically belongs on Dashboard > Settings > BuddyPress > Components.

But in backend it can be interesting to use it in order to be as close to WordPress backend UI.

Yeah. When you put it this way, I think that your technique is probably a good one, at least as a stopgap, and only when used in the backend. Echoing what I said earlier, I think it would be better framed as *the BP avatar upload UI* (maybe bp.avatar_uploader) rather than a general and reusable bp.media.

[imath]

Replying to boonebgorges:

"Attachments" in this sense (the UI component) would be another specific implementation of "attachments" (the BP API).

Ouch! I should have read more carefully DJPaul's notes, i've just read again the "Attachments" part and your latest comment helped me to better understand the scope. I realize this patch is out of the scope (a bit "hors sujet" actually). Thanks a lot for your patience :)

[imath]

Forgot to close this in my latest comment.