[buddypress-trac] [BuddyPress Trac] #7104: oembed activity review

[buddypress-trac]

#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:

 * 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!

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7104>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac