oembed activity review

[DJPaul]

Some observations from a post-commit code review:

• ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-core/classes/class-bp-oembed-component.php?rev=10837#L43 throws an exception -- we do not use exceptions anywhere in BuddyPress, and they are infrequent in WordPress core (for good or bad). I think this should be refactored somehow, maybe just assume this property is always set, or die.
• In bp_activity_embed_media(), there's $_REQUEST['hide_media']. How is this variable going to be passed over a POST? It's a GET endpoint, so can't this be GET?
• css-activity.php; why isn't this just a CSS file?
• bp_activity_setup_oembed() we could just move these filters into acitivity-filters.php, which I think is the aim of that file? Doesn't really impact performance even if running on a too-old version of WordPress.
• bp_activity_embed_has_activity() the (int) $activity_id === (int) $activity->id bit -- like we did recently, can't we please just change whatever calls this function to make damn sure integers are always passed?
• bp_activity_get_embed_excerpt() the "<func> includes the 'Read More'" bit -- that comment block should open with /* not /** as it's not a PHPDoc block, and the latter will confuse the WP Code Reference documentation parser here.
• bp_activity_embed_excerpt_onclick_location_filter() -- the regular expression is too brittle. All it'd take is for the href property not to start at the beginning of the link block. There's a better way to do this which I'm trying to remember and will leave a comment later.
• Also in bp_activity_setup_oembed(), we should use version_compare() to check the WordPress function. I know we are internally inconsistent but let's get it right here.

Thanks for your work on this, it looks really great!

[r-a-y]

In 10844:

Activity: In bp_activity_embed_media(), use $_GET instead of $_REQUEST.

The oEmbed endpoint uses GET (see WP_REST_SERVER::READABLE), so no
need to look in POST as well.

Props DJPaul.

See #7104.

[r-a-y]

In 10845:

Activity: Fix up multi-line inline doc snafu.

See #7104.

[r-a-y]

Thanks for the feedback, @DJPaul. I'm going to reply to each point in a separate comment.

First:

css-activity.php; why isn't this just a CSS file?

Initially, it's because we did not have dedicated functions to locate and load assets in Core, so I kind of hacked my way around by using a template part. On hindsight, this is bad and breaks theme developer conventions.

So css.patch is what I originally envisioned for locating and loading assets. It introduces two functions:

• bp_locate_template_asset() - which is almost exactly like bp_locate_template()
• bp_get_template_asset() - which is almost exactly like bp_get_template_part()

I feel this should replace BP_Legacy::locate_asset_in_stack().

[r-a-y]

throws an exception -- we do not use exceptions anywhere in BuddyPress, and they are infrequent in WordPress core (for good or bad).

I think exceptions are good, but I understand the concerns of sticking with what WordPress is doing.

I'm fine with whatever you decide here.

---

bp_activity_setup_oembed() we could just move these filters into acitivity-filters.php

Will make the change.

Update - Fixed in r10852.

---

Also in bp_activity_setup_oembed(), we should use version_compare() to check the WordPress function.

Will make the change. Is there a legitimate use for bp_get_major_wp_version()?

Update - Fixed in r10851.

---

bp_activity_embed_excerpt_onclick_location_filter() -- the regular expression is too brittle.

Good catch. I guess a dev that is filtering the embed excerpt could inject link attributes before the onclick filter is run.

Anyway, this should work:

function bp_activity_embed_excerpt_onclick_location_filter( $text ) {
        return preg_replace_callback( '/<a\s+[^>]*href=\"([^\"]*)\"/iU', 'bp_activity_embed_excerpt_onclick_location_filter_callback', $text );
}

---

bp_activity_embed_has_activity() the (int) $activity_id === (int) $activity->id bit

I'll commit some stuff from #6977 to fix this.

Update - Fixed in r10854.

[r-a-y]

Updated to fix some whitespace issues.

[DJPaul]

Probably not much use for bp_get_major_wp_version() except perhaps whoever added it didn't know better, and missed it in review. We can probably stop using it sometime.

[r-a-y]

In 10851:

Embeds: Use version_compare() instead of a direct operator check.

Props DJPaul.

See #7104.

[r-a-y]

In 10852:

Embeds: Move embed filters to bp-activity-filters.php.

This increases legibility from a dev's perspective.

Props DJPaul.

See #7104.

[r-a-y]

In 10861:

Embeds: Do not use exceptions.

Until we are ready to use exceptions across the BuddyPress codebase, we
shouldn't use it here. Instead, just return.

See #7104.

[r-a-y]

In 10863:

Revert "Grunt: Modify RTLCSS to generate RTL files for inline CSS template parts."

Using inline CSS as a template part is a no-no. This reverts r10841.

See #7104, #6772.

[r-a-y]

In 10864:

Activity Embeds: Use CSS asset file for inline CSS.

Previously, we were using a template part to grab the inline CSS. This
is not a great idea.

Instead, we are using the actual CSS file and including it inline.

See #7104, #6772.

[r-a-y]

In 10865:

Activity Embeds: Make regex for top.location.href filter less brittle.

See #7104.

[r-a-y]

Okay, closing this one as all points should be addressed now.

Thanks for the feedback, @DJPaul!