WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 20 months ago

#38003 closed enhancement (fixed)

Add oEmbed support for User, List and Like Twitter timelines

Reported by: earnjam Owned by: ocean90
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

oEmbed support for Tweets was added in 3.4. Support for Twitter Collections and Moments was added in [36954].

Since then, there have been three other Twitter timeline oEmbed endpoints made available: User Timeline, List Timeline and Likes Timeline. https://dev.twitter.com/web/embedded-timelines/oembed

Can we extend the provider list to support those three timeline types?

Attachments (4)

38003.diff (2.0 KB) - added by earnjam 22 months ago.
38003.2.diff (2.0 KB) - added by earnjam 22 months ago.
Cleaned up regex a bit
38003.3.diff (2.5 KB) - added by earnjam 21 months ago.
Updated patch w/recommendations from @Ocean90
38003.tests.diff (1.7 KB) - added by earnjam 21 months ago.
Updated tests to go with 38003.3.diff

Download all attachments as: .zip

Change History (15)

#1 @johnbillion
22 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.7

Thanks @earnjam. Moving to 4.7 for consideration.

#2 @ocean90
22 months ago

As noted in ticket:36197:9, Twitter provides the links for auto discovery but because of wp_filter_oembed_result() we have to add these to the trusted providers list.

@earnjam
22 months ago

#3 @earnjam
22 months ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

38003.diff adds the other 3 Twitter timeline URLs as trusted providers.

My regex isn't the best, so someone should look it over and see if there is a better way to handle it.

In that patch I also slightly modified the existing Twitter providers because they had a . matching any character in the username space, but Twitter usernames can only contain alphanumeric characters and underscores, so [\w] seems like a more specific match. If we'd rather keep the wildcard match, then I can make a new patch.

The [\w] change also throws off the nice whitespace alignment on the array, but I didn't want to muddy up the diff with whitespace changes.

Last edited 22 months ago by earnjam (previous) (diff)

@earnjam
22 months ago

Cleaned up regex a bit

#4 @earnjam
22 months ago

38003.2.diff does 2 things differently:

  1. Realized I didn't need the square brackets around \w, so got rid of those.
  2. Dropped the support for trailing slash on user timeline URL. Twitter doesn't use trailing slashes anywhere, so wasn't sure if we should bother with supporting that or not.

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


21 months ago

#6 @ocean90
21 months ago

  • Owner set to ocean90
  • Status changed from new to reviewing

#7 @ocean90
21 months ago

  • Keywords needs-refresh added; 2nd-opinion removed

@earnjam Based on this support article using \w for the username is fine.

  • Your username cannot be longer than 15 characters. […]
  • A username can only contain alphanumeric characters (letters A-Z, numbers 0-9) with the exception of underscores, […]

Since the length of a username is limited you can replace \w+? with \w{1,15}. For user and list timelines we should support an optional trailing slash: \w{1,15}/?$ and \w{1,15}/likes/?. That makes it consistent with the other services where we use a simple .* match.

A new patch should:

1) Update the regular expressions. 2) Update the docs for the oembed_providers filter to include the new endpoints. 2) Extend Tests_oEmbed::$provider_map with the new endpoints. 3) Extend Tests_oEmbed::oEmbedProviderData() with sample data for the new endpoints.

You don't need to adjust the alignments, I'll do that in a separate commit.

@earnjam
21 months ago

Updated patch w/recommendations from @Ocean90

@earnjam
21 months ago

Updated tests to go with 38003.3.diff

#8 @earnjam
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests needs-refresh removed

Thanks @ocean90!

38003.3.diff reflects the changes you mentioned above. I put the tests in a separate patch at 38003.tests.diff

#9 @ocean90
21 months ago

@earnjam Thanks! Turns out that Twitter's API doesn't like trailing slashes so I'm going to remove the optional slash from the regex.

#10 @ocean90
21 months ago

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

In 38693:

Embeds: Add oEmbed support for User, List and Like Twitter timelines.

Props earnjam.
Fixes #38003.

#11 @ocean90
20 months ago

In 38923:

Embeds: Realign the provider list after [38693].

See #38003.

Note: See TracTickets for help on using tickets.