uploads dir should filtered for cover images

[m_uysl]

I used bp_attachments_cover_image_upload_dir for changing default cover-image path and it works, I can see the file into correct folder but AJAX callback gives error.

<?php
// hard-coded filter
add_filter( 'bp_attachments_cover_image_upload_dir', function ( $upload_dir ) {
        return array(
                'path'    => '/Applications/MAMP/htdocs/wordpress/wp-content/uploads/cover-image/2016/09/21/mustafa',
                'url'     => 'http://wordpress.dev/wp-content/uploads/cover-image/2016/09/21/mustafa',
                'subdir'  => '/2016/09/21/mustafa',
                'basedir' => '/Applications/MAMP/htdocs/wordpress/wp-content/uploads/cover-image',
                'baseurl' => 'http://wordpress.dev/wp-content/uploads/cover-image',
                'error'   => false,
        );
} );

I realized that callback fails from bp_attachments_cover_image_generate_file because bp_attachments_cover_image_ajax_upload gets uploads dir from bp_attachments_uploads_dir_get()

So in this case, overriding the default AJAX callback with bp_attachments_pre_cover_image_ajax_upload filter is current solution.

[boonebgorges]

Thanks for the ticket, @m_uysl.

I worked through the problem a bit and it's not straightforward to solve. The root issue is, as noted in the description, that the upload_dir isn't being filtered in a consistent way. When you filter via bp_attachments_cover_image_upload_dir, the filter is effective during wp_handle_upload() but not in other places in the API (such as bp_attachments_uploads_dir_get()).

There are a handful of places where it's possible that we could run upload_dir through upload_dir_filter(). For example, in bp_attachments_cover_image_ajax_upload(), we can do the following:

        // Get BuddyPress Attachments Uploads Dir datas.
        $bp_attachments_uploads_dir = bp_attachments_uploads_dir_get();
+       $bp_attachments_uploads_dir = $cover_image_attachment->upload_dir_filter( $bp_attachments_uploads_dir );

This fixes half of the comparisons during the AJAX handler. The other half is harder to solve. Later in bp_attachments_cover_image_generate_file(), we do the following check:

if ( false === $dimensions || $args['file'] !== $args['cover_image_dir'] . '/' . wp_basename( $args['file'] ) ) {
    return false;
}

This check fails without doing some manipulation to the BP_Attachment_Cover_Image object. That object caches some properties (like base_dir) in a way that isn't sensitive to the bp_attachments_cover_image_upload_dir filter.

This is not a very good explanation (I didn't really get to the bottom of things) but it gestures in the direction of a solution, which is to ensure the bp_attachments_cover_image_upload_dir filter is applied to all ways that the cover image object generates its upload paths.

[rekmla]

We're running into this issue as well. Currently on BP v2.6.1.1.
I was not able to figure out how to use the bp_attachments_pre_cover_image_ajax_upload filter without having to hack bp_attachments_cover_image_generate_file()

Currently testing with two hacks:

        // Get BuddyPress Attachments Uploads Dir datas.
        $bp_attachments_uploads_dir = bp_attachments_uploads_dir_get();
        $bp_attachments_uploads_dir = $cover_image_attachment->upload_dir_filter( $bp_attachments_uploads_dir );

        // The BP Attachments Uploads Dir is not set, stop.

in bp_attachments_cover_image_ajax_upload() and

        $bp_attachments_uploads_dir = bp_attachments_uploads_dir_get();
        $bp_attachments_uploads_dir = $cover_image_class->upload_dir_filter( $bp_attachments_uploads_dir );
        // Make sure the file is inside the Cover Image Upload path.
        if ( false === strpos( $args['file'], $bp_attachments_uploads_dir['path'] ) ) {

in bp_attachments_cover_image_generate_file()

Along with these hacks, we're using the following filters: bp_attachments_cover_image_upload_dir, bp_attachments_uploads_dir_get, bp_attachment_upload_dir

[boonebgorges]

Circling back around to this ticket. 7265.diff​ is a first pass at centralizing the filtering of the cover image upload directory. This does appear to fix the immediate issue of path filterability; filters on bp_attachments_cover_image_upload_dir work properly now, without breaking uploads.

@imath - I don't like to ping you about stuff like this, but could I ask you to take 5-10 minutes - no more!! - and think about whether it's going to cause other problems if we generate cover image upload paths like this, outside of the BP_Attachment_Cover_Image class? I think that the upload_dir_filter() method will still work internally, so $this->upload_path etc ought to be correct - but this is a pretty delicate system, so I'm nervous :-D

[imath]

Hi @boonebgorges

Thanks a lot for the ping, i'm always happy to help :) I'll look at it asap.

[imath]

@boonebgorges

I think there's no problem if we generate cover image upload paths outside of the BP_Attachment_Cover_Image class. But the cover image unlike the avatar is using a specific upload base_dir named buddypress :)

So i think BP_Tests_BP_Attachment_TestCases->test_bp_attachment_cover_image_user_upload() should fail with 7265.diff. So i'd suggest :

• 7265.02.patch which is passing the uploads data built within the Cover Image class into the argument of the new function bp_attachments_cover_image_upload_dir.
• or 7265.02-alternative.patch which is basically using bp_attachments_uploads_dir_get() instead of bp_upload_dir() into the new function bp_attachments_cover_image_upload_dir.

PS: I haven't tested both options, i've just successfully run the unit test suite on both of them.

PS (2): I remember my idea was to organize the uploads_dir so that every BuddyPress attachments are in
wp-content/uploads/buddypress/object_type/object_id/attachment_type/file.ext

[boonebgorges]

Thanks @imath ! I'm glad I asked you - I wasn't thinking about the buddypress directory.

I like that 7265.02.patch​ tries to keep most of the logic running through BP_Attachment - it feels wise to have it centralized. But I also find it much harder to reason about than 7265.02-alternative.patch​, which is (IMO) much more straightforward.

I think we might move toward a future where upload path generation happens in a more vertical fashion, so that (for example) bp_attachments_uploads_dir_get() is the ultimate source of truth used for the top-level attachment directory, and then everything else builds on it (BP_Attachment->upload_path etc would use this instead of bp_upload_dir(), cover images paths would be built this way, etc). This is not something that needs to be solved today, though, so I think that 7265.02-alternative.patch​ is probably the right tack.

@rekmla I've spoken with your team about this issue recently - would you mind giving 7265.02-alternative.patch​ a look to see if it solves the problem for you?

[rekmla]

@boonebgorges, we've tried this patch in our environment with BP 2.9.3 and this works as expected. Thanks!

[boonebgorges]

Thanks for the help with this one, all! Let's go with it.

[boonebgorges]

In 11875:

Centralize the generation of the cover image upload directory.

By using bp_attachments_cover_image_upload_dir() wherever the cover image
directory is needed, we ensure that it's possible to filter consistently, for
installations (such as multi-network installations) that may want a custom
location for these files.

Props rekmla, imath.
Fixes #7265.