[buddypress-trac] [BuddyPress Trac] #6278: Attachment Library

[buddypress-trac]

#6278: Attachment Library
------------------------------------+-----------------------
 Reporter:  imath                   |       Owner:  imath
     Type:  enhancement             |      Status:  assigned
 Priority:  normal                  |   Milestone:  2.3
Component:  API                     |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  has-patch dev-feedback  |
------------------------------------+-----------------------
Changes (by DJPaul):

 * owner:   => imath
 * status:  new => assigned


Comment:

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

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6278#comment:58>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac