Changes between Version 4 and Version 5 of Ticket #6278, comment 4
- Timestamp:
- 03/04/2015 05:10:09 PM (10 years ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
Ticket #6278, comment 4
v4 v5 12 12 * 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. 13 13 * 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. 14 * Should we even be calling `wp_mkdir_p()`? It looks like `wp_upload_dir()` handles this for us? 14 15 * See: `groups_avatar_upload_dir()`, `bp_core_signup_avatar_upload_dir()`, and `xprofile_avatar_upload_dir()` respectively. 15 16 * 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. 16 17 * 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. 17 18 * 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. 18 * Should we be calling `remove_all_filters( 'upload_dir' )` in our `upload()` method? This filter makes me nervous since so many things touch it, that I'm afraid something else will be hooked in and break it. Do we need to do the `switch_to_blog()`dance?19 * 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. 19 20 20 21 More specific feedback about `BP_Attachment_Avatar`: