Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#6591 closed defect (bug) (fixed)

Wrong parameter value for upload_dir filter in BP Attachment API

Reported by: rittesh.patel Owned by: imath
Milestone: 2.4 Priority: normal
Severity: normal Version: 2.3.0
Component: Media Keywords: has-patch commit
Cc: ritzpatel91@…

Description

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

Attachments (4)

6591.patch (815 bytes) - added by rittesh.patel 4 years ago.
Change $accepted_args value to 1 instead of 0 for upload_dir filter.
6591.02.patch (3.2 KB) - added by imath 4 years ago.
6591.03.patch (3.7 KB) - added by imath 4 years ago.
6591.04.patch (11.4 KB) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (15)

@rittesh.patel
4 years ago

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

#1 @imath
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.4
  • Owner set to imath
  • Status changed from new to assigned
  • Version changed from 2.3.2 to 2.3.0

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.

#2 @imath
4 years ago

  • Keywords needs-refresh added; has-patch removed

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.

Last edited 4 years ago by imath (previous) (diff)

#3 @imath
4 years ago

  • Keywords has-patch reporter-feedback added; needs-refresh removed

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

@imath
4 years ago

#4 @rittesh.patel
4 years ago

  • Cc ritzpatel91@… added
  • Keywords reporter-feedback removed

Hi @imath,

Just checked and tested new patch, it looks good.

#5 @imath
4 years ago

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 ;)

#6 @imath
4 years ago

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 ?

@imath
4 years ago

#7 @rittesh.patel
4 years ago

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.

#8 @imath
4 years ago

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.

#9 @imath
4 years ago

  • Keywords commit added; dev-feedback removed

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
4 years ago

#10 @imath
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

#11 @DJPaul
3 years ago

  • Component changed from Component - Attachments to Media
Note: See TracTickets for help on using tickets.