Skip to:
Content

Opened 20 months ago

Closed 20 months ago

Last modified 19 months ago

#7104 closed defect (bug) (fixed)

oembed activity review

Reported by: DJPaul Owned by:
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

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!

Attachments (1)

7104.css.patch (16.9 KB) - added by r-a-y 20 months ago.
Updated to fix some whitespace issues.

Download all attachments as: .zip

Change History (15)

#1 @r-a-y
20 months ago

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.

#2 @r-a-y
20 months ago

In 10845:

Activity: Fix up multi-line inline doc snafu.

See #7104.

#3 @r-a-y
20 months ago

  • Keywords has-patch added

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 attachment:7104.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().

Last edited 20 months ago by r-a-y (previous) (diff)

#4 @r-a-y
20 months ago

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.

Last edited 20 months ago by r-a-y (previous) (diff)

@r-a-y
20 months ago

Updated to fix some whitespace issues.

#5 @DJPaul
20 months ago

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.

#6 @r-a-y
20 months ago

In 10851:

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

Props DJPaul.

See #7104.

#7 @r-a-y
20 months ago

In 10852:

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

This increases legibility from a dev's perspective.

Props DJPaul.

See #7104.

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


20 months ago

#9 @r-a-y
20 months ago

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.

#10 @r-a-y
20 months ago

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.

#11 @r-a-y
20 months ago

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.

#12 @r-a-y
20 months ago

In 10865:

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

See #7104.

#13 @r-a-y
20 months ago

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

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

Thanks for the feedback, @DJPaul!

#14 @DJPaul
19 months ago

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