Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#38003 closed enhancement (fixed)

Add oEmbed support for User, List and Like Twitter timelines

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


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.

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

Attachments (4)

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

Download all attachments as: .zip

Change History (15)

#1 @johnbillion
8 years 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
8 years 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.

8 years ago

#3 @earnjam
8 years 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 8 years ago by earnjam (previous) (diff)

8 years ago

Cleaned up regex a bit

#4 @earnjam
8 years 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.

8 years ago

#6 @ocean90
8 years ago

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

#7 @ocean90
8 years 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.

8 years ago

Updated patch w/recommendations from @Ocean90

8 years ago

Updated tests to go with 38003.3.diff

#8 @earnjam
8 years 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
8 years 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
8 years 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
8 years ago

In 38923:

Embeds: Realign the provider list after [38693].

See #38003.

Note: See TracTickets for help on using tickets.