Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#21632 closed enhancement (fixed)

Adding Imgur as an oEmbed provider

Reported by: bradparbs's profile bradparbs Owned by: markjaquith's profile 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 12 years ago.
Adding imgur to the list of oEmbed providers
21632.diff (840 bytes) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (27)

@bradparbs
12 years ago

Adding imgur to the list of oEmbed providers

#1 @Tyrun
12 years ago

Patch works - great job Brad!

#2 follow-up: @scribu
12 years 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.

#3 in reply to: ↑ 2 @markjaquith
12 years 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.

#4 @wonderboymusic
12 years ago

Related / please add: #15734

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#5 @Tyrun
12 years 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

#6 @scribu
12 years 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.

#7 @Tyrun
12 years ago

Related please add #21635

#8 @nacin
12 years 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.)

#9 @nacin
12 years 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.

@nacin
12 years ago

#10 @nacin
12 years ago

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

#11 @nacin
12 years ago

  • Status changed from assigned to closed

#12 @nacin
12 years ago

  • Status changed from closed to reopened

#13 @nacin
12 years ago

  • Keywords punt added

#14 @nacin
12 years 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.

#15 @johnbillion
11 years 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?

#16 @nacin
11 years 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.

#17 @nacin
11 years ago

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

#18 follow-up: @Jayjdk
11 years 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 11 years ago by Jayjdk (previous) (diff)

#19 in reply to: ↑ 18 @nacin
11 years 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.

#20 @nacin
11 years 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.

#21 @nacin
11 years 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 :)

#22 @nacin
11 years 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.

#23 @DrewAPicture
11 years ago

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

#24 follow-up: @GaryJ
11 years 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?

#25 in reply to: ↑ 24 @nacin
11 years 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.