[buddypress-trac] [BuddyPress Trac] #7104: oembed activity review
noreply at wordpress.org
Tue May 31 13:22:03 UTC 2016
#7104: oembed activity review
Reporter: DJPaul | Owner:
Type: defect (bug) | Status: new
Priority: normal | Milestone: 2.6
Component: API | Version:
Severity: normal | Keywords:
Some observations from a post-commit code review:
/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
* `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!
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7104>
BuddyPress Trac <http://buddypress.org/>
More information about the buddypress-trac