Skip to:
Content

Opened 2 years ago

Closed 2 years ago

Last modified 19 months ago

#6574 closed defect (bug) (fixed)

Attachment API conflicts with wp_enqueue_media()

Reported by: landwire Owned by: imath
Milestone: 2.4 Priority: normal
Severity: normal Version: 2.3.2
Component: Media Keywords: has-patch
Cc:

Description

I am getting a JS error ("Uncaught TypeError: Cannot read property 'add' of undefined") with the new Buddypress Attachments API (Avatars) when I am loading media scripts with wp_enqueue_media() on the same page as the attachments upload. I confirmed this on a new install with no plugins except BuddyPress.

Attachments (4)

6574.patch (415 bytes) - added by imath 2 years ago.
6574.02.patch (821 bytes) - added by imath 2 years ago.
6574.03.option1.patch (685 bytes) - added by imath 2 years ago.
6574.03.option2.patch (756 bytes) - added by imath 2 years ago.

Download all attachments as: .zip

Change History (28)

#1 @imath
2 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.3.3
  • Owner set to imath
  • Status changed from new to assigned

Thanks for your feedback @landwire. I confirm and was able to reproduce. My bad :(

I think we have two ways to fix this : rename bp.Uploader to something different eg: bp.bpUploader. Or only take what we need from the window.wp global. I'm suggesting this second way because there are at least two plugins extending the Avatar UI (2 of my plugins) and there could be others.

I've tested 6574.patch and it seems to fix the issue. I think we should fix it for 2.3.3.

@imath
2 years ago

#2 @DJPaul
2 years ago

I am only looking at the patch quickly but if you needed to stick the contents of window.wp into the window.bp global for WP scripts to work, is it possible to put it somewhere like window.bp.wp instead?

Last edited 2 years ago by DJPaul (previous) (diff)

#3 @imath
2 years ago

Thanks a lot DJPaul for your feedback, i see what you mean but i think it won't change anything because we need to use window.wp - window.wp.Uploader because we are using our own model for plupload. So i really think the best way is to _.omit( window.wp, 'Uploader' ) in our window.bp so that our plupload model can set our custom bp.Uploader.

And i've tested this by adding our Uploader inside the WP Admin edit post screen, the wp.media editor and our Uploader are working fine together :)

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

#4 @landwire
2 years ago

Thanks for the quick patch!

#5 @DJPaul
2 years ago

I can't comment if this is the best fix for the branch, but once we have it resolved, I want to open another ticket for trunk to discuss moving/namespacing the WP property.

#6 @imath
2 years ago

I understand and it's absolutely fine with me.

#7 @johnjamesjacoby
2 years ago

Patch seems fine at a cursory. Agree with Paul we should have a 2.4 plan.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


2 years ago

#9 @imath
2 years ago

@johnjamesjacoby, @djpaul i'm going to commit the patch for the 2.3 branch and leave this ticket open so that i'll be able to come back on it with a new patch for 2.4. Thanks again for your feedbacks.

#10 @imath
2 years ago

In 10032:

Make sure the bp.Uploader is fully available even if wp_enqueue_media() is being used.

See #6574 (branch 2.3)

#11 @imath
2 years ago

In 10033:

Make sure the bp.Uploader is fully available even if wp_enqueue_media() is being used.

See #6574 (trunk)

#12 @imath
2 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.3.3 to 2.4

#13 @imath
2 years ago

From the window.wp global we only need the Backbone, ajax and template attributes.

So i'm suggesting 6574.02.patch which makes sure, we only get what we need from window.wp without adding any new attribute to it. In other words window.wp will remain unchanged.

@imath
2 years ago

#14 @imath
2 years ago

  • Keywords has-patch added; needs-patch removed

#15 @DJPaul
2 years ago

This does stomp all over the window.bp variable but since this is the only place we really use it, it's acceptable for a 2.3.x fix.

#16 @imath
2 years ago

@DJPaul, there's something i don't understand, you absolutely want to put window.wp in window.bp.wp ?

As there's no interference on window.wp with the patch it don't get the interest of this, because anyway window.wp should always be available ?

#17 @imath
2 years ago

ok i think i got it, ideally you'd rather the window.bp variable to not spread this widely. But i think if it wasn't the case, then avatar.js and webcam.js would probably fail. Do you have an idea to do this in another way ?

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


2 years ago

#19 @imath
2 years ago

In 10069:

Improve the way the BuddyPress uploader is getting window.wp needed attributes.

We only need wp.Backbone, wp.ajax and wp.template.

See #6574 (2.3 branch)

#20 @imath
2 years ago

In 10070:

Improve the way the BuddyPress uploader is getting window.wp needed attributes.

We only need wp.Backbone, wp.ajax and wp.template.

See #6574 (trunk)

#21 @imath
2 years ago

I've been thinking about this. I think just like buddypress() or $bp we need the window.bp to be available so that it can be extended. Eg: avatar.js is extending it and so does webcam.js and so does mentions.js in a way and so does at least 2 of my plugins :)

But i agree .02.patch is not ideal because it's resetting completely the window.bp global to the part we need from the window.wp global. So just like buddypress() or $bp i think we need to be carefull to always extend it and not reset it.

So i'm suggesting 2 new patches which are basically doing the same, the first option is doing it in 1 line, the second one in 3 lines. The second one may be more comprehensive.

#22 @DJPaul
2 years ago

I don't know enough to prefer one style over the other, but this is a good improvement.

#23 @imath
2 years ago

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

In 10207:

BuddyPress Uploader: Make sure to extend the window.bp global instead of resetting it.

Props DJPaul

Fixes #6574

#24 @DJPaul
19 months ago

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