WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 2 months ago

Last modified 2 months ago

#21632 closed enhancement (fixed)

Adding Imgur as an oEmbed provider

Reported by: bradparbs Owned by: markjaquith
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch needs-refresh
Focuses: Cc:

Description

Adding imgur to the list of oEmbed providers! (wcgr rocks)

Attachments (2)

imguroembed.patch (926 bytes) - added by bradparbs 20 months ago.
Adding imgur to the list of oEmbed providers
21632.diff (840 bytes) - added by nacin 20 months ago.

Download all attachments as: .zip

Change History (27)

bradparbs20 months ago

Adding imgur to the list of oEmbed providers

comment:1 Tyrun20 months ago

Patch works - great job Brad!

comment:2 follow-up: scribu20 months ago

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

Similar: #11978

The thing is that there are quite a few generic image hosting providers, like imageshack, photobucket etc.

We should have a "canonical" plugin that adds support for all these addition oEmbed providers, instead of adding them to Core.

comment:3 in reply to: ↑ 2 markjaquith20 months ago

Replying to scribu:

The thing is that there are quite a few generic image hosting providers, like imageshack, photobucket etc.

We should have a "canonical" plugin that adds support for all these addition oEmbed providers, instead of adding them to Core.

Adding providers is really low cost for us. Knowing that they need a plugin, tracking that plugin down, and installing it, is a big hurdle for users. These "magic" embeds will work better the more that people can rely on popular embed providers to be supported without any effort on their part. I think our hurdle should be the popularity and stability of the embed provider. Imgur has been around for a while, and serves multiple petabytes a month — I think they fit the bill. And FWIW, I'd be happy to add ImageShack and Photobucket too.

comment:4 wonderboymusic20 months ago

Related / please add: #15734

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:5 Tyrun20 months ago

I agree with @markjaquith. Additionally Photobucket is already supported:

http://core.trac.wordpress.org/browser/trunk/wp-includes/class-oembed.php?rev=20874#L44

comment:6 scribu20 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 3.5

I think our hurdle should be the popularity and stability of the embed provider. Imgur has been around for a while, and serves multiple petabytes a month — I think they fit the bill.

Agreed.

comment:7 Tyrun20 months ago

Related please add #21635

comment:8 nacin20 months ago

I believe the correct endpoint is just http://api.imgur.com/oembed, and that the regular expression is dangerously missing a slash. (A domain like i.imgur.com.nacin would work.)

comment:9 nacin20 months ago

Looks like this should additionally be subdomain-agnostic.

Additionally, the endpoint requires at least a height and width of 640 (if either/both are specified) to display http://imgur.com/w7zCp, despite it being only 554 x 541. Otherwise, it provides a height/width of 90 x 90. That's pretty lame and needs more research.

Since they have a flaky endpoint, seems like this should be punted for now.

nacin20 months ago

comment:10 nacin20 months ago

  • Keywords 2nd-opinion added
  • Owner set to markjaquith
  • Status changed from new to assigned

comment:11 nacin20 months ago

  • Status changed from assigned to closed

comment:12 nacin20 months ago

  • Status changed from closed to reopened

comment:13 nacin19 months ago

  • Keywords punt added

comment:14 nacin19 months ago

  • Keywords punt removed
  • Milestone changed from 3.5 to Future Release

Per IRC, punted until imgur obeys maxwidth and maxheight in a sane way.

comment:15 johnbillion3 months ago

  • Keywords good-first-bug needs-refresh added

Anyone want to look into Imgur's oEmbed support and see if they've fixed the maxwidth and maxheight issue?

comment:16 nacin3 months ago

It works now: http://api.imgur.com/oembed?url=http://imgur.com/w7zCp&maxwidth=554

We need to confirm our regex is proper. If it is, it doesn't need to be a regex, as the non-regex variant now handles SSL and non-SSL.

comment:17 nacin3 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 3.9

comment:18 follow-up: Jayjdk2 months ago

I've tested attachment:21632.diff and it works besides the need for refresh (I have a patch but decided not to add it, as this ticket has been marked "good-first-bug").

But I have a question - I'm unsure what is meant by:

We need to confirm our regex is proper. If it is, it doesn't need to be a regex, as the non-regex variant now handles SSL and non-SSL. – nacin

Last edited 2 months ago by Jayjdk (previous) (diff)

comment:19 in reply to: ↑ 18 nacin2 months ago

  • Keywords good-first-bug removed

Replying to Jayjdk:

But I have a question - I'm unsure what is meant by:

We need to confirm our regex is proper. If it is, it doesn't need to be a regex, as the non-regex variant now handles SSL and non-SSL.

Sorry for the shorthand. oEmbed providers can be specified in two ways: as a regular expression and as a simple static string. In the latter case, http://i.imgur.com/* would be converted to https?://i.imgur.com/.* transparently. The HTTPS handling for non-regex providers is newer than my patch. I didn't look at my patch originally and figured that was the main reason for the regex.

Clearly, it's not — imgur has multiple subdomains. At the very least, I've seen both 'i' and 'm' recently. So, the patch looks good.

(I have a patch but decided not to add it, as this ticket has been marked "good-first-bug").

Appreciate it. In this case though, good-first-bugs != refresh a patch that otherwise works. I think John was hoping someone would investigate Imgur's endpoint, test it, and confirm we have the proper regex (are there other domains to check, for example?). But you and I pretty much handled that. :-)

Time to commit this, I think.

comment:20 nacin2 months ago

Hmm, maybe this isn't quite ready for commit.

I'm trying to find any kind of "institutional" support for oEmbed. It's not listed anywhere on api.imgur.com. How did we come to learn its existence? I found http://imgur.userecho.com/topic/53378-any-plan-to-provide-oembed-api-interface/ with a reply from Alan, in which he wrote (two years ago): "That's a really good idea. I've just added it to my todo list. A blog post will most definitely be made when it's released, but it should be sometime next month." Good to know they like this, but it's still listed as "PLANNED" and I can't find anything on their (WordPress-powered) blog: http://imgur.com/blog/?s=oEmbed. I also can't find anything in their API google group: https://groups.google.com/forum/?fromgroups#!searchin/imgur/oembed. Searching Google for "oembed imgur" doesn't turn up much, and "oembed site:imgur.com" is limited to a link to their API page (which contains no documentation — but did it?).

oEmbed is not widely, so I don't expect them to make it a first-class citizen on their API documentation or anything, but it's a bit odd I can't find anything. We've gone through this problem with Twitter where they routinely forget about and break their oEmbed endpoint, which I'd like to avoid here. I'm currently in their API IRC channel and am asking for more info.

comment:21 nacin2 months ago

Venturing into #imgur-dev was successful!

5:55	nacin	Hello, I have a question about imgur's oEmbed endpoint. I found http://api.imgur.com/oembed but I can't find any documentation or announcement on it anywhere. Is this a supported API?
5:57	nacin	I found http://imgur.userecho.com/topic/53378-any-plan-to-provide-oembed-api-interface/ with a reply from two years ago saying it'll be added, along with an announcement. This endpoint has been functional for at least 18 months best I can tell, and within that timeframe at one point it was updated to more properly follow one of the oEmbed spec parameters, so it seems to be maintained, but I am not sure.
5:57	nacin	Couldn't find an announcement on the blog, or documentation on the API site, or any chatter about it in the google group.
5:59	nacin	FWIW, I'm a lead developer of WordPress — thanks for using us for your blog; it could use an update though :-). We only like adding oEmbed providers when we know they're not likely to disappear or break. (Twitter likes to mess with us). So this is background research.
6:01		DrGrim entered the room.
6:03	DrGrim	nacin: it's still supported
6:03	DrGrim	i actually updated it a few months ago
6:03	DrGrim	it's also in the <head> of our site
6:03	nacin	DrGrim: any plans to document and/or ditch it?
6:03	nacin	oh, good to know!
6:04	nacin	it wasn't obeying maxwidth when images were narrower than the maximum requested width, I imagine that's one of the things you fixed
6:04	DrGrim	definitly not ditching it. I didn't actually think it needed documentation because scrappers and such pick it up from the source code
6:04	DrGrim	yeah, i rewrote the it all. hopefully it works now :)

comment:22 nacin2 months ago

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

In 27113:

Add imgur to the list of oEmbed providers.

props bradparbs.
fixes #21632.

comment:23 DrewAPicture2 months ago

Added to the oEmbed providers list in the Codex for 3.9.

comment:24 follow-up: GaryJ2 months ago

While it's a really low-cost addition to have this provider, is it really something that a significant proportion of users are going to be using? Or does that not apply here due to it being one line of code?

comment:25 in reply to: ↑ 24 nacin2 months ago

Replying to GaryJ:

While it's a really low-cost addition to have this provider, is it really something that a significant proportion of users are going to be using? Or does that not apply here due to it being one line of code?

These regular expressions are run only on URLs that have already been extracted from the post and made possible candidates for embedding. They're fast, there aren't many of them, and at the pace we add them, we're not going to be making any measurable dent in performance.

That said, we do have some guidelines for when to add an oEmbed provider: http://make.wordpress.org/core/handbook/design-decisions/#whitelisting-oembed-providers. It wasn't linked here because these guidelines were written down in the time since this ticket first stalled about 17 months ago, but imgur fits those guidelines very well. The only thing holding them back originally was the shaky API endpoint.

imgur is huge and growing. While you and I may not use it, a large swath of the internet does, and they use it to create content. And that section of the internet produces content that manages to get me to click on a link to an imgur.com page on average once a day, and I bet I see way more images elsewhere getting hotlinked from it. A lot of imgur users surely also have WordPress sites. It makes sense to add it.

Note: See TracTickets for help on using tickets.