Wrong parameter value for upload_dir filter in BP Attachment API

[rittesh.patel]

The $accepted_args parameter value is set to "0" for upload_dir filter in base BP_Attachment class. It should be "1" as WordPress passes the parameter to callback function.

​https://buddypress.trac.wordpress.org/browser/tags/2.3.2/src/bp-core/classes/class-bp-attachment.php#L269

[rittesh.patel]

Change $accepted_args value to 1 instead of 0 for upload_dir filter.

[imath]

Thanks for your feedback. Actually i've reproduced what we used to do in xprofile and groups upload dir filters. But i agree it can be interesting to pass the orginal array so that we can easily get subfolders like YY/MM.

I don't think it would have any impact on our objects. So we should do it imho.

[imath]

Ah! spoke too quick see groups_avatar_upload_dir() and xprofile_avatar_upload_dir() So the patch will need to be refreshed to avoid passing the original array for these filters.

[imath]

I think 6591.02.patch should be ok for every cases, can you confirm ?

[rittesh.patel]

Hi @imath,

Just checked and tested new patch, it looks good.

[imath]

Hi @rittesh.patel

Thanks a lot :) I'll be off for a week, so this will let the other members of the team to eventually check/comment about it. Then when i'll be back i'll commit it if no objections ;)

[imath]

I was about to commit 6591.02.patch, then i had a second thought. I think it's safer to reverse the logic. If the plugin needs the original upload dir, then it should add an extra parameter when constructing its class e.g.:

Extend_Attachment_Class {
	public function __construct() {
		parent::__construct( array(
			// the other parameters..
			'upload_dir_filter_args' => 1,
		) );
	}
}

See 6591.03.patch. What others are thinking about this ?

[rittesh.patel]

I think ​6591.02.patch should be committed because WordPress by default passes the upload dir parameters in callback function. If any plugin needs different behavior than it should override the default value.

[imath]

I understand your point @rittesh.patel. But it's BuddyPress Attachment class not WordPress one :)

I need to run some tests to see what's the best option. The most important is: anyway this will be fixed in 2.4, you'll be able to get the original Uploads dir data within your filter.

[imath]

I still think second option is safest. If we are using some functions to filter the upload dir and to do something else depending on the passed parameters, i can imagine other plugins doing the same.

Plugins wishing to receive the original upload dir filter simply have to add the 'upload_dir_filter_args' and set it to 1 inside the array they use to construct their Attachment class.

6591.04.patch is taking in account the new Cover Image Attachment class and improve the unit test.

I've been testing this patch several times (including some tests with plugins) and i think we're ready to have this in :)

[imath]

In 10194:

Allow class extending the BP_Attachment class to get the original WordPress upload dir within their upload_dir_filter() method.

To do so, you simply need to add the argument $upload_dir_filter_args to the array you use inside your constructor and set it to 1.

Props rittesh.patel

Fixes #6591