Support oembed

[DJPaul]

Throughout BP where appropriate. Look at r-a-y's plugin as a start point.

[r-a-y]

Patch is against 1.3-bleeding.

I've only added support for activity updates and forum posts in this patch to give a sense of what is needed. Added shortcode support for forum posts so you can use the [embed] shortcode there as well.

Try:

[embed width=200]YOUTUBE URL[/embed]

Feedback definitely welcome!

[r-a-y]

Adds missing diff for bp-core.php

[boonebgorges]

r-a-y - Looking mostly really great.

As I was testing 2707.002.patch, I got some wp_debug errors in BP_Embed::shortcode() around patched line 796; it says $cache is undefined. I don't see anywhere in the class (or in the WP_oEmbed class you're extending) where the cache is actually being loaded with wp_cache_get using the $cachekey you are generating. Am I missing something?

[r-a-y]

Ah, yes, forgot to declare $cache as empty.

Add the following above line 796:

$cache = '';

BP_Embed::shortcode() is basically a base function. It doesn't do anything unless you pass a BP component's cache into it. See bp_embed_init() in bp-core-embed.php where the cache is actually grabbed.

[boonebgorges]

Cool, that makes sense, r-a-y.

You mentioned that this was only for activity updates and forum posts - do you envision supporting oembed in other kinds of content?

Are you happy with this patch being committed so that we can do iterative improvements?

[boonebgorges]

Declares an empty $cache to avoid WP_DEBUG errors

[DJPaul]

Can we register each component's oEmbed supports in that component's _setup_globals() funcs?

[r-a-y]

The plan is to add embed support to group descriptions with group meta and xprofile fields with xprofile meta.

The only concern with xprofile fields is not all admins will want embed support for all fields. A comma-delimited define could work; if one isn't defined, then all fields will be enabled by default. Sound good? Or do we want to move these type of settings into the admin area?

---

Re: committing current patch - did you guys get the email I sent you about a week ago? It detailed what I had in mind for a potential WP media.php patch to prevent unnecessary code duplication. That can probably be looked at for WP 3.2, but in the meantime I'm okay with what we have if everyone else is on board.

Also 2707.003.patch is missing the diff for bp-core-embed.php.

---

(component)_setup_globals() might be too early; needs investigating.

[DJPaul]

I think BP 1.4 is a more likely time frame to look at per-xprofile field specific settings. I suggest you put your patch onto the wp-trac so that those involved can take a look at your approach and give feedback; I've no problems putting in as-is until WP oEmbed has been updated the way you say it needs to be.

[r-a-y]

2707.004.patch hooks into the component's loop to grab the object ID and cache instead of on init.

Patch depends on #2981 (see bp_forums_embed() in bp-core-embed.php).

This approach is cleaner than hooking into init and doing conditional checks for components.

Could move bp_activity_embed() into bp-activity.php and bp_forums_embed() into bp-forums.php if desired.

[boonebgorges]

See [3616] for the #2981 fix.

I would prefer to see bp_activity_embed() in bp-activity.php (mutatis mutandis for forums).

Also, I'm with Paul that we put your patch into BP until WP's oEmbed is updated accordingly.

[DJPaul]

2707.005.2.patch is a refresh. JJJ, please could you have a look at this and decide if the oembed stuff should be in its own file? My feeling is that those few functions could live in their respective component's -core.php file.

Also, will need to update the hooks from the init action (bp_init?)

[r-a-y]

2707.005.2.patch is missing the diff for bp-forums-filters.php to enable shortcode support in forum posts.

You can also change "init" to "bp_init" for bp_embed_init().

[DJPaul]

Let's pick this up again. r-a-y, can you remind please what you felt was needed to be changed in WordPress core to support part of this (without duplication)? Has it been resolved in WordPress 3.2, or is there a WordPress trac ticket?

[r-a-y]

Apologies for getting to this ticket a little late.

I've definitely dropped the ball on this one as I didn't post my WP patch upstream. Since it's late in the 3.2 cycle, there's no chance it will be included.

I've just added the patch: ​#WP17857

---

The BP patch currently adds some link checking that's not currently in WP. I'd like to prevent this and only override lines 806-834 in the 2707.005.02 patch. With the proposed WP patch, this would mean only overriding the new parse_oembed() function. Let me know if you'd like to see a patch detailing how this might all look like in the future!

Also, the current BP patch needs a refresh to account for priority changes made to the activity filters. I'll take a look at this in the next few days.

[r-a-y]

2707.006.patch takes into consideration ticket ​#WP17857.
If the WP patch is accepted, BP_Embed::shortcode() can be removed from a future BP release.

I forgot to alter line 1173 of bp-core-classes.php in the patch to remove the author_can() check. Though, this does bring up an interesting point.

I think we should remove the author_can( $id, 'unfiltered_html' ) check for BP because unless you're a super admin,

$attr['discover']

will most likely always return false. Thus, negating the usefulness of oEmbed discovery.

However, WP_Embed::shortcode() has always had the author_can() check. So if we remove BP_Embed::shortcode() in the future, we'll likely to face the same problem as the majority of users will not have the 'unfiltered_html' cap.

Thoughts?

[r-a-y]

There's also a typo on line 48 of bp-forums-filters.php.

The comment should say:

Allow shortcodes in forum posts

[DJPaul]

If the patch needs to re-implement part of WordPress core's oEmbed, which uses author_can(), then our implementation should keep parity in the duplicated sections of the code. i.e. if WP core uses author_can() here, we should too.

[r-a-y]

Now that I've looked into it some more, author_can() is restricted to Wordpress posts:
​http://core.trac.wordpress.org/browser/tags/3.1.3/wp-includes/capabilities.php#L1076

So in the current BP patch, $attr[discover] will always return false.

Might need to wrap $attr[discover] around a new filter entirely for the WP patch.

[boonebgorges]

Bump. I do not want to punt this ticket beyond 1.3, but time is a-runnin' short.

[r-a-y]

I just need a little dev feedback.

My sentiment is we can remove author_can() from the BP patch and iterate. The BP patch uses its own oEmbed discovery filter (bp_embed_oembed_discover) instead of the WP one (embed_oembed_discover) anyway.

In addition, I think we should switch the "embed_oembed_html" filter to a new one (like "bp_embed_oembed_html"). "embed_oembed_html" handles the output of the embedded content; I'd like to use a new one to avoid any possible conflicts with existing WP Embed plugins that deal with posts.

I don't have a problem with punting this ticket if necessary, as I can always add this code into my oEmbed plugin regardless.

---

Patch needs to be refreshed again due to the newer changes in trunk. Once everyone has chimed in, I'll create a new patch.

[boonebgorges]

DJPaul said:

if WP core uses author_can() here, we should too.

If I'm reading the discussion and the code correctly, this won't work. $attr[discover] will always return false, except for admins. At the very least, we can ask for the proposed WP patch to include a filter on this value. We'll want to include clear documentation on how oEmbed can be turned off, if admins decide that it is a security risk.

"embed_oembed_html" handles the output of the embedded content; I'd like to use a new one to avoid any possible conflicts with existing WP Embed plugins that deal with posts.

On the other hand, it might be frustrating if someone is applying a filter to embeds in blog posts, only to find out that the same filters aren't being applied here. In either case, it's easy to fix - just add_ or remove_ filters - but we should probably make the decision based on the expected behavior. As I think about it, it'll be a bit easier to make it a different filter name, as it's pretty easy to add_filter(), while remove_filter() will require some logic to test whether the filtered content is on a BP page. So I guess I agree with r-a-y on balance.

DJPaul, can you jump in with some thoughts before r-a-y goes to the trouble of refreshing the patch (which does not, as he notes, apply to the current trunk)?

[DJPaul]

WP_Embed::shortcode, which is where part of this patch is copied from, has the $attr[discover] author_can( 'unfiltered_html' ) check. WordPress' oEmbed providers are whitelisted, and discovery is disabled by default unless a plugin turns it on. See discussion on ​#WP10337

Since this patch has a lot of copy and paste in, I am saying we keep with WordPress' implementation here.

[r-a-y]

author_can() will not be useful to BP until BP 1.4 when the move to CPT is scheduled.

Yes, oEmbed discovery is disabled by default, however some people will want oEmbed discovery turned on.

I've updated the WP patch in ​#WP17857 so a filter is wrapped around author_can(); in the forthcoming BP patch, the $attr[discover] line will follow what is in the WP patch verbatim.

Any other comments before I re-patch?

[DJPaul]

Boone and I discussed the $attrdiscover? in IRC last night, in some detail. We concluded that the following was a good option:

$attr['discover'] = ( apply_filters( 'bp_embed_oembed_discover', false ) && current_user_can( 'unfiltered_html' )

[boonebgorges]

To expand on what DJPaul has said: it makes sense to override the ::shortcode() method for the time being, using the current_user_can() check that he suggests. If and when your request for a filter in WP core is granted, we can deprecate BP_Embed::shortcode() and just filter the cap in question.

If that all makes sense, please go ahead and refresh the patch.

[r-a-y]

current_user_can() applies to the logged-in user, which 99% of the time is okay. However if you're programmatically adding content (like using bp_activity_add()), current_user_can() will return false.

Since this is an edge case, I'm okay with this.

Now, let's say an admin wanted to allow oEmbed discovery, it would require adding the "unfiltered_html" capability to a role.

If the admin adds the "unfiltered_html" capability to the base role of Subscriber, this would work from a security standpoint because Subscribers cannot publish WP posts and it will still allow BP to use oEmbed discovery, so that's good!

oEmbed discovery is relatively safe anyway! It would take some work by a culprit just to embed a piece of malicious script. (eg. setup a domain and an oEmbed endpoint and a user would have to paste a URL from said domain.)

Patch on the way!

[r-a-y]

Brainfart!

[djpaul]

(In [4709]) Add oembed support to activity stream items and forum posts. Fixes #2707, massive props r-a-y

[djpaul]

(In [4829]) Add oembed support into private messages. See #2707