#6278 closed enhancement (fixed)
Attachment Library
Reported by: | imath | Owned by: | imath |
---|---|---|---|
Milestone: | 2.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Core | Keywords: | has-patch dev-feedback |
Cc: |
Description
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 :
- API improvements around handling media (specifically avatars) are long overdue.
- 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".
- 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 :)
Attachments (12)
Change History (89)
#3
@
10 years ago
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.
- It seems to me that
BP_Attachment
should be a base class (maybe evenabstract
), withBP_Attachment_Member_Avatar
andBP_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 toBP_Attachment::__construct()
. This would allowBP_Attachment
to be free of any internal reference to avatars.
- What is the advantage of
buddypress()->avatar
being aBP_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?
#4
@
10 years ago
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 ourstdClass
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 ofBP_Attachment
and then also haveBP_Attachment_XProfile_Avatar
andBP_Attachment_Group_Avatar
extendBP_Attachment_Avatar
. This way all of the "avatar" specific functionality currently inBP_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 trapwp_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 likewp_upload_dir()
handles this for us? - See:
groups_avatar_upload_dir()
,bp_core_signup_avatar_upload_dir()
, andxprofile_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 haveupload_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 theupload()
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 ourupload()
method? Do we need to do theswitch_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 inBP_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.
#5
@
10 years ago
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 :)
#6
@
10 years ago
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
towp_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 relatedupload_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.
This ticket was mentioned in Slack in #buddypress by jjj. View the logs.
10 years ago
#8
@
10 years ago
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.
#9
@
10 years ago
@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 :)
#10
@
10 years ago
@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 unfilteredwp_upload_dir()
, but then we would need to explain how to set a different path
What do you think ?
#11
@
10 years ago
@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
#12
@
10 years ago
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
#13
@
10 years ago
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?
#14
follow-up:
↓ 15
@
10 years ago
You are not off base. Your list of todos just are not todone yet. We will get there before this gets committed.
#15
in reply to:
↑ 14
@
10 years ago
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
#16
@
10 years ago
@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 ?
#17
@
10 years ago
We should still set the legacy globals, but there's no reason we have to do it using a BP_Attachment_Avatar
object.
#18
@
10 years ago
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
#19
@
10 years ago
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()
#20
follow-up:
↓ 21
@
10 years ago
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.
#21
in reply to:
↑ 20
@
10 years ago
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.
#22
@
10 years ago
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.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
10 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
10 years ago
#25
@
10 years ago
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?
#26
@
10 years ago
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.
#27
@
10 years ago
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.
#28
@
10 years ago
@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.
#29
@
10 years ago
The hash mimics Gravatar, and originally all avatars were saved in the same directory (vs. each member having their own.)
#30
@
10 years ago
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
#31
@
10 years ago
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
#32
@
10 years ago
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...
#34
@
10 years ago
@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
#35
@
10 years ago
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.
#36
@
10 years ago
Thanks a lot for your feedback boonebgorges, i completely agree : waiting for johnjamesjacoby's review :)
#37
@
10 years ago
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.
#38
@
10 years ago
This is looking great. Agree with Paul some more general cleanup should happen once it drops.
The only thing I see 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.
The same will probably go for any of our image editing utilities. Thinking future enhancements like tagging members in photos, facial recognition, etc… These types of utilities could also eventually grow into their own tools with their own classes, if they aren't already. I guess what I'm saying is, let's be sure we are deliberate with what functionality we empower what classes with.
#40
@
10 years ago
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',
#41
@
10 years ago
@modemlooper the BP_Attachmens class already handles that. Change your parent_folder array key to base_dir and it should be good to go
#43
@
10 years ago
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.
#44
@
10 years ago
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
#45
@
10 years ago
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.
#46
@
10 years ago
I'm wondering if some of the access modifiers for the properties in BP_Attachment
could be made protected
or private
since they're only intended to be used as args in the public constructor?
#54
@
10 years ago
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.
#55
@
10 years ago
Congratulations @imath and team! It's been a long road since ticket 5429 and it's finally coming to fruition :) Simply awesome.
#57
@
10 years ago
In 6278.08.patch, i'm trying to improve the code following DJPaul's recommandations (we've been talking about it at #wcldn) :
- Improve i18n for the avatar file types error message and allow people to restrict the image types for avatar.
- abstract the BP_Attachment class, so it must be extended.
- avoid using
get_class_vars( __CLASS__ )
to get the default values in the BP_Attachment constructor - Sanitize the base_dir if set
- Make sure the action and file_input parameters are also sanitized
- use a
is_dir()
check instead of afile_exists()
one when checking if the upload_path exists - 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 ?
#58
follow-up:
↓ 66
@
10 years ago
- Owner set to imath
- Status changed from new to assigned
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
ofelse 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 likeBP_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 baseBP_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.
#59
@
10 years ago
Thanks for your feedback.
I'm planing to commit:
- abstract the BP_Attachment class, so it must be extended.
- avoid using
get_class_vars( __CLASS__ )
to get the default values in the BP_Attachment constructor - Sanitize the base_dir if set
- Make sure the action and file_input parameters are also 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
- 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
#64
@
10 years ago
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.
#66
in reply to:
↑ 58
@
10 years ago
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 likeBP_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 baseBP_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
#68
@
10 years ago
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
#72
@
10 years ago
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 :(
#73
@
10 years ago
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?
#74
@
10 years ago
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 ?
Thanks for this; excited to check it out.
I will look and review in about 15 hours.