Attachment Library

[imath]

Once upon a time :)

I've first worked on a component manage attachments for other attachments (see #5429)
It was built as a plugin on our ​github repository

Then during WCSF 2014, members of the team who attended this event had a ​discussion about BuddyPress and ideas for the future.

On the "Attachments" topic, the discussed road was :

1. API improvements around handling media (specifically avatars) are long overdue.
2. No appetite for a "photo album" component in core at the moment, but our new APIs should be built to let other plugin developers build one "easily".
3. We want to overhaul/rebuild the user avatar upload process/cropper/management etc as a first step for a UI change directly related to media. Clean-Cole has begin to do some refinement based on prior work by @imath

A bit later, i've worked on a patch on ticket #6004, and it appeared :

• it was suggesting a lot of changes ( too much :) )
• the component organization wasn't great, as uploading an avatar would require the Attachments component to be active
• we shouldn't rely on wp.media (at least for the front end part of the API)

The consensus seems to be to work on "attachments" as a core library. Avatar would use this library, and so custom BuddyPress components/plugins.

The attached patch is my suggestion for the first step : building an "attachment" tool (actually it's mainly dealing with managing uploads) the avatar/plugins could use to upload files. For this first patch, i've worked with one of my plugin in mind, trying to answer to the question: how could BuddyPress help even more BuddyDrive to manage uploads?

BuddyDrive needs are :
a) to create a specific directory under WP_CONTENT uploads (the 'buddydrive' folder)
b) to put the uploaded file in it filtering 'upload_dir'
c) to have specific rules when a file is uploaded (each user has a quota of "disk space", and mime types can be restricted by Administrator)

If we look at avatars, we're doing similar things :
a) we need to create 2 folders 'avatars' & 'group-avatars'
b) member and group avatars are using specific filters on 'upload_dir'
c) only image mime types are allowed

The BP_Attachment class of the patch is making sure the avatar management is working as before, but also allows any plugin sharing BuddyDrive's needs to use it to manage their uploads. Like Avatars custom components could set up a global in their BP_Component->setup_globals() function and then simply use (when needed) :

buddypress()->{component_id}->upload( $file );

I've also thought about the UI changes of the above point 3. I don't know if Clean-Cole made progress about it, but working on another plugin (BP Avatar Suggestions) gave me an idea. I agree using wp.media on front end is not so great: overriding wp.media needs to be done even in wp-pluploads as the Attachments model is used in there. So a BuddyPress specific UI would be great specially if :

• it deals with uploads/cropping (of course)
• but also if it deals with other ways to set a user or a group avatar (eg: grabbing a file from an url, or allowing to directly take a photo of the user see #5548).

Anyway i'm too curious about these last points, so i'll explore them :)

[johnjamesjacoby]

Thanks for this; excited to check it out.

I will look and review in about 15 hours.

[imath]

Thanks a lot @johnjamesjacoby :)

[boonebgorges]

imath - Thank you for your continued work on this. I think we're getting closer to a solid foundation for an API that will be usable by all components. I love the implementation details of the upload handling - the way you're setting upload directories, filtering 'upload_dir', wrapping WP's upload functions, etc are all great.

The syntax doesn't seem quite right to me. I have two broad thoughts about it.

1. It seems to me that BP_Attachment should be a base class (maybe even abstract), with BP_Attachment_Member_Avatar and BP_Attachment_Group_Avatar as extending classes. Extending classes would be responsible for (a) setting the attachment subdirectory path, (b) managing the connection between attachment and object (in the case of avatars, this just means putting it in the correct directory with the proper filename, but in the future it might mean things like post-to-post relationships or taxonomies or postmeta or whatever), (c) attachment-specific validation (eg, avatars must be image files), and other stuff like that. In other words - extending classes would do a lot of the work that you're currently doing with the arguments that you are passing to BP_Attachment::__construct(). This would allow BP_Attachment to be free of any internal reference to avatars.

2. What is the advantage of buddypress()->avatar being a BP_Attachment object? Is it for backward compatibility? It seems to me that attachments are objects in the proper sense, and do not need to be forced into the singleton pattern. As a developer using the API, I would expect a syntax closer to this:

$avatar = new BP_Attachment( $args ); // or BP_Attachment_Member_Avatar or whatever
$success = $avatar->upload( $uploaded_file );

or maybe, for future implementations of attachments:

$avatar = new BP_Attachment_Foo();
$success = $avatar->upload( $uploaded_file );
if ( $success ) {
    $avatar->associate_with_group( $group_id );
    $avatar->associate_with_user( $user_id );
    // etc etc etc
}

Obviously, this is very rough. But it seems more intuitive to me, as a potential user of the API. What do others think?

[johnjamesjacoby]

I agree with Boone, that a flexible base class that the other components can subclass is ideal for a few reasons which I'll outline below in a few different sections.

Generic feedback & thoughts:

• BP_Attachment should be a parent 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 and should only contain CRUD methods for interfacing with attachments, and widely applicable methods for error handling and interfacing with the file system.
• Any component wanting Attachments would then extend BP_Attachment and pass an array; here's a few ideas that may-or-may-not make sense: file-system requirements (folders, dimensions, file-sizes, maximum allowed?), identifiers for namespacing, etc...
• Subclasses 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.
• Should we even be calling wp_mkdir_p()? It looks like wp_upload_dir() handles this for us?
• 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? Do we need to do the switch_to_blog()dance again? 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.

[imath]

Thanks a lot for your feedback boonebgorges :)

My initial thought was that this class could be used when setting up globals in the component in order to set once and for all the avatar uploads dir. That's what is currently done in bp_core_set_avatar_globals()when setting the upload path. I understood this was done to avoid switching blog too many times. Then, you're right, the second reason was that i wanted to be sure the $bp->avatar was like before, reason of the unit test.

But i think you're completely right. What i've done, means each time a component would use the class, it would actually switch blog. So that's not that great. I guess, we could have a core function to set this in a global once and for all, this way we'd eventually be sure to do only one switch blog and the BP_Attachment base class could use this function.

@johnjamesjacoby, thanks a lot for your feedback too :)

I agree with both of you about having a very generic BP_Attachment class that can be extended by the avatar/component.

• (folders, dimensions, file-sizes, maximum allowed?) and mime types for example
• about wp_mkdir_p() & the 'bp_attachment_upload_dir' filter : you're absolutely right

Do we need to do the switch_to_blog()dance?

i'll check but i think so, else it might create folders in sub blogs file system uploads/sites/{$blog_id}. But as mentioned above in my comment, if we make sure to do it once, then i think it's ok.

i'll work on version 2 of the patch, asap :)

[johnjamesjacoby]

​6278.02.patch roughly (and incompletely) implements a number of my above feedbacks. Patch is intended to be iterated upon and cleaned up.

I also did a few other things that maybe are mistakes:

• removed the $component variable since it's no longer a requirement
• use the $action variable where $component used to be used for namespacing
• Added test_form to wp_handle_upload() overrides, so form submission works
• Added create_dir method to encapsulate directory creation and whether it's necessary
• Removes wp_mkdir_p() usages and cleans up related upload_dir filters
• A bunch of other random surrounding cleanup I don't have time this minute to finish typing up here. :/

@imath - sorry if I've duplicated your efforts. I didn't see the notification about your reply until just now, hence me uploading this (kind of incomplete) patch sooner than later.

[johnjamesjacoby]

To be absolutely clear, there is more to do beyond my attached patch. It's a quick iteration to discuss at dev-chat and implements only a few of our suggestions above, and still needs a cleaner OOP implementation.

[imath]

@johnjamesjacoby

sorry if I've duplicated your efforts. I didn't see the notification about your reply until just now, hence me uploading this (kind of incomplete) patch sooner than later.

No problem, on the contrary: many thanks, it's really helpful :)

[imath]

@johnjamesjacoby: about remove_all_filters( 'upload_dir' )

So far we are not taking in account the fact that a plugin/administrator ... may filter 'upload_dir' in a more "global" scope than we do.

I can see reasons why we would want to make sure to get an "unfiltered" wp_upload_dir() : for instance we can fear a plugin/theme to play 'not nicely' in this area.

On the other hand, i'm unsure how an administrator would set a different path for uploads for "security reasons" (eg: move the uploads out of the site's web directory) : filtering 'upload_dir' or defining the ​UPLOADS constant.

So i think we have 2 choices :

• do nothing. So far everything seems to be ok with avatars.
• or use a bp_upload_dir() out of an unfiltered wp_upload_dir(), but then we would need to explain how to set a different path

What do you think ?

[imath]

@johnjamesjacoby i've tested 6278.02.patch. This is shaping good :)

A few suggestions :

Added test_form to wp_handle_upload() overrides, so form submission works

I think we shouldn't bypass the test form. Your form submission wasn't working because the upload action was not set. Defining test_form to false simply allow any upload.

So i moved the 'bp_avatar_upload' into the arguments of $bp->avatar = new BP_Attachment_Avatar( $args );

I've also introduced a new global : $bp->upload_dir so that the first to use BP_Attachment will define once and for all the upload dir so we eventually switch blog only once.

Finally i think we should provide a way to restrict uploads by mime types. So i've introduced a new property $allowed_mime_types.

About the way WordPress is managing this is a bit annoying it restricts the download, which is good but the message is a bit weird ('Sorry, this file type is not permitted for security reasons.') and i think we cannot override it. That's why i keep on using the specific rule for avatars.

see BP_Attachment->validate_mime_types() for some other comments.

I've included these points in 6278.03.patch

[imath]

Oops, i missed this one :
If the component is specifying a $base_dir property, we are using it to set the $upload_path and $url, so let's create it, once url & uplaod_path are set

Avatars are setting their base_dir later (eg: 'group-avatars' in groups_avatar_upload_dir()) and do not use this property.

Updated the patch to 04.patch

[boonebgorges]

Changes are looking good here - thanks for the rapid progress!

I understand the need to set the values of buddypress()->avatar for backward compatibility, and I appreciate the value of using the singleton as a poor-man's cache (for stuff like upload paths).

However, I feel uncomfortable that these requirements are driving the architecture here. buddypress()->avatar->upload() seems very counterintuitive to me. More than that, we end up with global variable pollution, so that things like buddypress()->avatar->original are still set after the upload is complete. IMO avatar instances should be independent of each other, and global config data should be set separately. Let's not sacrifice what could be a fairly clean OOP design for the sake of backward compatibility. Am I off base?

[johnjamesjacoby]

You are not off base. Your list of todos just are not todone yet. We will get there before this gets committed.

[boonebgorges]

Replying to johnjamesjacoby:

You are not off base. Your list of todos just are not todone yet. We will get there before this gets committed.

I todon't want to seem impatient :-D

[imath]

@boonebgorges :)

I agree with you. But i think, we'll first need to build some more unit tests before "unglobalizing" $bp->avatar because so far for instance :
1- we have $bp->avatar_admin->original set
2- a lot of the things that are set in $bp->avatar are also used in bp_core_fetch_avatar()
eg: bp_core_avatar_upload_path(), bp_core_avatar_url()

I was also wondering, since we are somehow advising to extend the BP_Attachment class if we should abstract it, and use an abstract method to be sure to get the upload action ?

[boonebgorges]

We should still set the legacy globals, but there's no reason we have to do it using a BP_Attachment_Avatar object.

[imath]

Sorry i misunderstood :(

then i think we just need to :

• create a core function to get the wp_upload_dir() once
• edit the set_upload_dir() method to use this function
• edit bp_core_get_upload_dir() to use this function

I'll work on a new patch asap

[imath]

with 6278.05.patch, uploading an avatar looks like

// Upload the file
$avatar_attachment = new BP_Attachment_Avatar();
$bp->avatar_admin->original = $avatar_attachment->upload( $file, $upload_dir_filter );

I've reverted bp_core_set_avatar_globals() the way it was before, so removed the unit test.
I've reverted most of the bp_core_get_upload_dir() code except that it's now using a new function that can be used by any component: bp_upload_dir();

And @johnjamesjacoby, we need the switch_blog() dance because when on a multisite, bp_core_fetch_avatar() is using bp_core_get_upload_dir()

[rosyteddy]

In a WP + BP site, for a site member, will all the uploaded media (mainly images, whether via WP blog or via BP) be available in one central place or will they continue to remain scattered all over, with plugins (like media plugins) adding yet more places that the member has to keep track ? Thanks a lot.

[modemlooper]

Replying to rosyteddy:

In a WP + BP site, for a site member, will all the uploaded media (mainly images, whether via WP blog or via BP) be available in one central place or will they continue to remain scattered all over, with plugins (like media plugins) adding yet more places that the member has to keep track ? Thanks a lot.

Streamlining media handling in BP is part of the reason to have a dedicated api.

[imath]

In 6278.06.patch, i'm suggesting to add a crop method to the BP_Attachment_Avatar class. I think it can be interesting to have this method if we want to improve the upload/crop avatar UI or gives the ability to do a default cropping right after the avatar has been uploaded if a plugin or if we need it.

[imath]

Adds a crop method

[henry.wright]

Hi @imath

I just took a look through 6278.06.patch, the whole approach is getting me excited. I love how the intention is for the base class to be extended, that opens up lots of development possibilities.

One thought, create_dir() is called each time an object is created. Could directory creation be moved to the BP plugin activation routine? That will avoid having to check if the upload path exists each time. But then on second thoughts, there's always the issue of the webmaster manually deleting the directory which could cause problems if there's no wp_mkdir_p() to make things right again. Not sure what is best?

[henry.wright]

Also, just wondering why a hash is used as the filename rather than a simple unique identifier such as user ID or user_login?

$full_filename  = wp_hash( $absolute_path . time() ) . '-bpfull.'  . $ext; 
$thumb_filename = wp_hash( $absolute_path . time() ) . '-bpthumb.' . $ext;

592757b8618756393518f6121da2001b-bpthumb.jpg

vs

henrywright-bpthumb.jpg or 8276-bpthumb.jpg

Although I'm sure there's a good reason, to me hashing for this purpose seems unnecessary effort.

[modemlooper]

Prolly so file is unique. I confused by this entire thing. Dont see how this is an attachment api. It looks like just and update to avatars. Where's bp-core-attachments.php?

Seems this should have been the route and then avatars a subclass.

[henry.wright]

@modemlooper it's the foundations of an API. Think the plan is for it to be fleshed out. Also, the intention is for the BP_Attachments class to be an abstract class with BP_Attachments_Avatars being just one type of functionality extending it.

[johnjamesjacoby]

The hash mimics Gravatar, and originally all avatars were saved in the same directory (vs. each member having their own.)

[henry.wright]

Thinking ahead, should the /avatars/ directory name be changed to /attachments/? If each member has their own subfolder within that directory then it could serve as a dumping ground for all sorts of files that don't fit nicely into the WP Media Library (avatars being one example).

So the current dir structure of:

/uploads/avatars/1/filename.jpg
/uploads/avatars/1/filename2.jpg

becomes

/uploads/attachments/1/filename.jpg
/uploads/attachments/1/filename2.jpg

On multisite the current structure is:

/uploads/sites/2/avatars/1/filename.jpg
/uploads/sites/2/avatars/1/filename2.jpg

which becomes

/uploads/sites/2/attachments/1/filename.jpg
/uploads/sites/2/attachments/1/filename2.jpg

[modemlooper]

what's needed is a generic upload available to different file types. having atachment/file-type makes sense

allowing sub folder organization is important

atachments/avatar
atachments/pdf
atachments/gallery

we at WDS want to use this API to create a gallery plugin

[imath]

Thanks for your feedbacks.

1/ Avatar folders shouldn't change.
I think it's too risky to create an upgrade routine to move avatars in a new folder.

2/ Each component using the BP_Attachment class can define/manage their own folder organization, add htaccess files...

[modemlooper]

sounds great!

[henry.wright]

@imath regarding my last post about changing the avatar directory name, please ignore that. I now see what you're doing with base_dir - looks great

[boonebgorges]

imath - The improvements in 06.patch and especially the changes in 05.patch look really good to me. At this point, I feel comfortable with the architecture of the system, and I'd be glad for you to commit it and do further work in trunk. It'd be nice to get johnjamesjacoby's OK on this too.

[imath]

Thanks a lot for your feedback boonebgorges, i completely agree : waiting for johnjamesjacoby's review :)

[DJPaul]

I have not been following this ticket but took a quick-ish look at the code. I think the architecture is OK, too.
I spotted a bunch of small issues (some i18n, some code style issues, some questionable default values) but I am sure we will address those in due course.

[johnjamesjacoby]

This is looking great. Agree with Paul some more general cleanup should happen once it drops.

The only thing I can think of to think more intently about is whether the crop method belongs in the Avatar class, or if it should be available to all Attachments as a general utility.

[modemlooper]

+++ to image utilities

[modemlooper]

played with API and is good. Think we should add more to parent construct to set parent folder for items. I don't see developers needing weird structures. Can leave in a filter but for most instances seems like this structure would work for most everything.

parent_folder/user_id/files

same as avatars but to specify the parent folder and automatically use the id of user who is uploading.

parent::__construct( array(
	'action' => 'gallery_upload',
        'parent_folder' => 'gallery',

[henry.wright]

@modemlooper the BP_Attachmens class already handles that. Change your parent_folder array key to base_dir and it should be good to go

[modemlooper]

Perfect

[imath]

Thanks DJPaul, johnjamesjacoby, modemlooper & henry.wright for your feedbacks.

In 6278.07.patch, I've tried to fix the i18n issue (i guess my bad english isn't helping me!), code style... But i guess the best will be to show me exactly where the remaining problems are in this area.

I've included a crop method to the BP_Attachment class. I think if we do so, we must be sure people will only play in the uploads dir.

I've made the action and file_input parameters required parameters. Meaning if they are not set, nothing will happen. Anyway, WordPress needs them.

FInally, i've included a validation rule if the original_max_filesize is set, else, the file size wasn't checked and i've found it weird to provide this parameter without using it.. Of course, the method can still be overriden from the child class.

I've also added a bunch of unit tests.

To use them, you'll need to create a files subfolder to tests/phpunit/assets and put in it

• ​https://buddypress.org/media/buddypress_logo.pdf
• ​https://buddypress.org/media/disc.png
• any random jpg file you will call "buddypress-book.jpg" (i've used this ​file and renamed it)

Now, i think the sooner we'll get this in (and iterate when needed) will be the better. It will be easier for people to test and easier for me to work on #6290. I really think we need to try to provide a UI plugins will be able to use in association with this API.

[johnjamesjacoby]

Patch looks good.

I'm fine with this going in now, and being iterated on in trunk during the 2.3 cycle.

Can't wait to use the wp_delete_file() function instead of @unlink() someday. ​#WP17864

[imath]

Thanks for your feedback johnjamesjacoby

wp_delete_file() looks promising :) Although as far as i can see it's like a shortcut to @unlink() having a filter just before.

I'm going to split the last patch into smaller organized parts and will commit them in a few hours today, leaving this ticket open for further iterations.

[henry.wright]

Just wondering if some of the access modifiers for the properties in BP_Attachment could be made protected or private if they're not intended for use outside the class or extending classes?

[modemlooper]

Why don't we create bp_delete_file() ?

[imath]

In 9622:

Introduces the BuddyPress Attachments API

BP_Attachment is a new class custom and core components can extend to manage file uploads.

• It is taking care of setting the uploads data (path, url..)
• It is creating the specific base uploads directory for the component if requested
• It is taking care of uploading the files into the uploads directory
• Custom validation rules can be set in order to for instance: restrict mime types or file size
• If, like avatars, cropping an image file is needed, the class includes a crop method to help you.

bp_upload_dir() is also a new function to make sure the uploads data will always be the one of the site BuddyPress is activated on (the root site).

Props johnjamesjacoby, boonebgorges

See #6278

[imath]

In 9623:

Extends the BP Attachment class for the avatars need

BP_Attachment_Avatar is a new class that extends the BP_Attachment one to manage the avatar uploads.
It contains specific methods required by avatars, such as shrinking or cropping an image.

Props johnjamesjacoby, boonebgorges

See #6278

[imath]

In 9624:

Adapts Avatars core functions so that they use the BP_Attachment_Avatar class.

Props johnjamesjacoby, boonebgorges

See #6278

[imath]

In 9625:

Do some clean up in avatar uploads directory filters

Let WordPress creates the uploads directory for us.

Props johnjamesjacoby

See #6278

[r-a-y]

Nice work everyone!

[imath]

thanks a lot @r-a-y :)

[DJPaul]

We can do a pair code review at WC London, and I can show you some of the general areas where we can make improvements on the code you've already committed, imath.

[mercime]

Congratulations @imath and team! It's been a long road since ticket 5429 and it's finally coming to fruition :) Simply awesome.

[imath]

Thanks a lot mercime :)

DJPaul : great idea and thanks in d'avance :)

[imath]

In ​6278.08.patch, i'm trying to improve the code following DJPaul's recommandations (we've been talking about it at #wcldn) :

1. Improve i18n for the avatar file types error message and allow people to restrict the image types for avatar.
2. abstract the BP_Attachment class, so it must be extended.
3. avoid using get_class_vars( __CLASS__ ) to get the default values in the BP_Attachment constructor
4. Sanitize the base_dir if set
5. Make sure the action and file_input parameters are also sanitized
6. use a is_dir() check instead of a file_exists() one when checking if the upload_path exists
7. Use bp_parse_args in the BP_Attachment crop method

I'm also suggesting unit tests (actually there were already in 07.patch, but i am hesitating). I'm adding some files in assets: 1 pdf, 2 images ; and i'm simulating some uploads : is that Ok with Travis ?

[DJPaul]

Some random feedback:

bp_core_get_allowed_avatar_types is a good improvement, but it looks like it could be improved: I think it is overcomplicated, and the array manipulation/flipping is confusing. You can't set the mime type with the filter (only the file extension). I think we need to be using some combination of WP's get_allowed_mime_types (maybe), wp_check_filetype_and_ext, wp_match_mime_types, and for the filter, sanitize_mime_type. I think we also want unit tests for the new mime functions, as they will be important to ensure they always work (from a security perspective with file uploads, etc).

bp_core_check_avatar_type could use a filter on the return value.

Thanks for working on the i18n for the error message, I'm not sure it's quite right (not every language will use a comma to separate a list of items, I guess). I'll see if I can find examples of similar places in WordPress core we could use to base our message on.

It looks like bp_core_check_avatar_type (and related functions) should be committed separately and have its own ticket.

And minor things:

• Please use elseif of else if. I think it's in the WP coding standards.
• Also there is some != use that ought to be replaced with the type sensitive version.

In the tests:

• BP_Attachment_Extend doesn't make sense to me as a class name for the unit tests. I suggest something like BP_Tests_Attachment??.
• Do we really need to upload files in our unit tests and write them to disk? It will be slow. Can't we just fake some data to pretend that has happened?
• The rrmdir function would best go in the base BP_UnitTestCase to avoid duplicating them.
• For test_bp_upload_dir_ms, please see #6162.
• The more mime type handlers we have to write, the more unit tests I want for them. If we can use WP's functions here, I don't mind not having unit tests for them.

[imath]

Thanks for your feedback.

I'm planing to commit:

1. abstract the BP_Attachment class, so it must be extended.
2. avoid using get_class_vars( __CLASS__ ) to get the default values in the BP_Attachment constructor
3. Sanitize the base_dir if set
4. Make sure the action and file_input parameters are also sanitized
5. use a is_dir() check instead of a file_exists() one when checking if the upload_path exists
6. Use bp_parse_args in the BP_Attachment crop method
7. use 'elseif' instead of 'else if' use '!==' instead of '!='.

Any objections ?

Then i'll open a new ticket for the i18n improvements that introduces the need to improve bp_core_check_avatar_type

[johnjamesjacoby]

Any objections ?

None from me.

[imath]

In 9660:

Improve the BP_Attachment class

• abstract the BP_Attachment class, so it must be extended,
• avoid using get_class_vars( __CLASS__ ) to get the default values in the constructor,
• sanitize the base_dir if set,
• make sure the action and file_input parameters are sanitized,
• use a is_dir() check instead of a file_exists() one when checking if the upload_path exists,
• use bp_parse_args() in the BP_Attachment->crop() method and include a filter,
• improve code formatting.

Props DJPaul

See #6278

[imath]

I still need to improve (by simplifying them) the unit tests.

[imath]

@DJPaul, Just created #6336 about bp_core_check_avatar_type()

[imath]

In the BP_Attachment_Avatar->crop() function i was finding interesting to use bp_core_avatar_dimension() instead of bp_core_avatar_full_width() && bp_core_avatar_full_height() that was a mistake. If somebody filters 'bp_core_avatar_full_height' && 'bp_core_avatar_full_width', the new dimensions will not be taken in account as bp_core_avatar_dimension is not filtered.

We need to avoid possible confusion with users as in ​2.2 we were using these functions.

If no objections, i will commit 6278.09.patch.

[imath]

In 9704:

Prevents a possible regression in the crop() function of BP_Attachment_Avatar

Instead of using bp_core_avatar_dimension(), use bp_core_avatar_full_width() and bp_core_avatar_full_height() when cropping an avatar, like it was the case in 2.2.

See #6278

[imath]

Replying to DJPaul:

6278.unittests.patch is simply adding unit tests for the BP Attachment class following your recommandations

In the tests:

• BP_Attachment_Extend doesn't make sense to me as a class name for the unit tests. I suggest something like BP_Tests_Attachment??.

Renamed it to BPTest_Attachment_Extension

• Do we really need to upload files in our unit tests and write them to disk? It will be slow. Can't we just fake some data to pretend that has happened?

To fake uploads, i suggest to interrupt _wp_handle_upload() just before it tries moving the temp file. To do so i'm including a specific upload error handler function in the upload overrides that creates an error 'fake_upload_success'. So we only need at the very list to include a new asset to fake the temporary file and i suggest to use this file : ​https://buddypress.org/media/disc.png

• The rrmdir function would best go in the base BP_UnitTestCase to avoid duplicating them.

Moved it in BP_UnitTestCase and removed it in the /testcases/core/avatars.php

• For test_bp_upload_dir_ms, please see #6162.

This test is now using $this->markTestSkipped(); for default test config

[DJPaul]

Cool, thanks.

[imath]

We can use the mystery-man.jpg file instead of adding a files folder and an image into the tests assests. That's what i'm doing in 6278.unitests.02.patch

[imath]

In 9755:

Avatar UI: Add Conditional functions and new BP Attachment methods

• bp_avatar_is_front_edit() checks if the user is editing his or a group avatar on front-end
• bp_avatar_use_webcam() checks if the Camera feature should be enabled. By default, it is not the case for touch devices as it can introduce some confustions with the possibility of using the camera of the device to take a selfie, save it as an image an upload it using the Browse button of the Avatar UI. Chrome and Firefox are the only browsers supporting getUserMedia.

NB: these two functions can be filtered to disallow the Camera feature or to completely disallow the new Avatar UI if you prefer to carry on using the legacy UI.

BP_Attachment::script_data() and BP_Attachment_Avatar::script_data() are two new methods to build the javascript localization data. The bp_attachment_avatar_params filter is fired if the current object is not a group or a user and can be used to build the params of another object such as a blog. You can also use the filter bp_attachment_avatar_script_data if you need to override data for any object.

Props boonebgorges, DJPaul.

See #6290
See #6278

[imath]

In 9760:

Improve bp_core_get_allowed_avatar_types(). Make sure to get the file type if the upload was not done using html5

The way we used to check the image file type is not working if the browser is Internet Explorer < 10. In this particular case, the Plupload runtime is falling back to flash and the file type is application/octet-stream. We are now using wp_check_filetype_and_ext() to fix this issue.

You can use the filter bp_core_get_allowed_avatar_types if you wish to *restrict* the avatar image types. Our supported types are: jpg, jpeg, png and gif.

Props DJPaul.

Fixes #6336
See #6290
See #6278

[imath]

todo get the list of allowed avatar types to build the error string

[imath]

ouch!! this is problematic was about to commit the unit tests but before i switched back to 3.7, then 3.8, then 3.9 just to be sure and i've found that a lot of hooks i'm using in the BP Attachment API are not available.

Meaning if version < 4.0 then the Attachment API will fail! Will try to find a solution.. but this is not great at all :(

[r-a-y]

Meaning if version < 4.0 then the Attachment API will fail! Will try to find a solution.. but this is not great at all :(

Would it be possible for the Attachment API to require WP 4.0? What issues are there if we did require at least 4.0?

[imath]

What a night! Ok here's the situation:

Since WordPress 4.0, an important filter for the API is dynamically built. In WordPress < 4.0 this filter was static wp_handle_upload_prefilter so we simply need to add/remove it before/after upload and it should be ok because in 3.9/3.8/3.7/3.6 WordPress was using extract() and the parameter names we're passing into the overrides arguments (e.g.: 'upload_error_strings' ) are matching with the var used inside _wp_handle_uploads().

So in 6278.10.patch i'm adding the fix for WordPress < 4.0, the unit tests, and a more dynamic way to build the available avatar allowed extensions which is needed since r9760.

@DJPaul, before i commit, could you check how the file extensions are listed into the error message. I've tried to find what you were talking about when saying :

I'm not sure it's quite right (not every language will use a comma to separate a list of items, I guess)

The only thing i've found was how WordPress was comma separating the post tags, so i've simply reproduced. Is That ok for you ?

[imath]

Spent my day testing 6278.10.patch on 3.6 / 3.7 / 3.9 / trunk. I'm able to confirm the patch is making sure the Attachment API works from 3.6 to trunk. But i'll update #6290 because under 3.9 we have another trouble.

[imath]

In 9831:

Make sure the BP Attachments API behaves the right way when WordPress < 4.0

Before WordPress 4.0, there was no dynamic filter based on the name of the action posted in the upload form. So we also need to filter wp_handle_upload_prefilter to make sure the BP_Attachments->validate_upload() function will actually validate the uploads for versions of WordPress < 4.0.

Improve the i18n of the avatar type error, as the avatar types can now be filtered in the BP_Attachment_Avatar class.

Add unit tests for the BP Attachement class.

Fixes #6278