Update how Imgur IDs are parsed

[pierlon]

Summary

Fixes #5482
Fixes #4697

Checklist

• My pull request is addressing an open issue (please create one otherwise).
• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[pierlon]

This PR is in counterpart with ampproject/amphtml#30589 that fixes how singular Imgur IDs are sanitized. Currently embedding a Imgur gallery or album works fine, but if embedding a singular Imgur image, amp-imgur prepends the Imgur ID with a/, which results in a 404 when loading the embed.

[github-actions]

Plugin builds for 7a07dac are ready 🛎️!

• Download development build
• Download production build

[westonruter]

So is #4697 valid? Instead of:

<amp-imgur
    data-imgur-id="2q6AS6i"
    layout="responsive"
    width="540"
    height="633"
  >
  </amp-imgur>

Should they have provided:

<amp-imgur
    data-imgur-id="a/2q6AS6i"
    layout="responsive"
    width="540"
    height="633"
  >
  </amp-imgur>

?

[pierlon]

I would say it still is valid. Based on the reproduction steps provided the URL in question was https://imgur.com/gallery/2q6AS6i, which when previously parsed the ID would be 2q6AS6i. The iframe URL would then be https://imgur.com/2q6AS6i, instead of the expected https://imgur.com/a/2q6AS6i.

[westonruter]

I'm seeing the amp-imgur getting a width of 580 and a height of 870:

This is coming from the $attr which is passed into AMP_Imgur_Embed_Handler::filter_embed_oembed_html(), which is coming from the initial oEmbed response data:

{
    "version": "1.0",
    "type": "rich",
    "provider_name": "Imgur",
    "provider_url": "https://imgur.com",
    "width": 580,
    "height": 870,
    "html": "<blockquote class=\"imgur-embed-pub\" lang=\"en\" data-id=\"a/rAG6Q2w\"><a href=\"https://imgur.com/a/rAG6Q2w\">View post on imgur.com</a></blockquote><script async src=\"//s.imgur.com/min/embed.js\" charset=\"utf-8\"></script>"
}

There is no width or height in the actual html, even though it's attempting to parse it out. Is this dead code?

amp-wp/includes/embeds/class-amp-imgur-embed-handler.php

Lines 52 to 68 in 5e85cc8

	if ( preg_match( '/width=["\']?(\d+)/', $return, $matches ) ) {
	$attr['width'] = $matches[1];
	}
	if ( preg_match( '/height=["\']?(\d+)/', $return, $matches ) ) {
	$attr['height'] = $matches[1];
	}
	if ( empty( $attr['height'] ) ) {
	return $return;
	}
	$attributes = wp_array_slice_assoc( $attr, [ 'width', 'height' ] );
	if ( empty( $attr['width'] ) ) {
	$attributes['layout'] = 'fixed-height';
	$attributes['width'] = 'auto';
	}

[westonruter]

The width and height issue appears to be specific to https://imgur.com/a/rAG6Q2w

[westonruter]

The excessive height seems to be a bug with Imgur's data response.

Nevertheless, should we be parsing the width and height when there is nothing in the HTML?

[pierlon]

Yea it does seem to be dead code. I'll remove it.

[westonruter]

Rebasing onto develop to fix build failure.

[westonruter]

I would say it still is valid. Based on the reproduction steps provided the URL in question was https://imgur.com/gallery/2q6AS6i, which when previously parsed the ID would be 2q6AS6i. The iframe URL would then be https://imgur.com/2q6AS6i, instead of the expected https://imgur.com/a/2q6AS6i.

But https://imgur.com/2q6AS6i/embed?pub=true is a failure whereas https://imgur.com/a/2q6AS6i/embed?pub=true succeeds.

So then with the reversion of ampproject/amphtml#30589 wouldn't this be required?

<amp-imgur
    data-imgur-id="a/2q6AS6i"
    layout="responsive"
    width="540"
    height="633"
  >
  </amp-imgur>

[westonruter]

Wait, so this is the case actually. In #4697 the issue was for embedding https://imgur.com/gallery/2q6AS6i.

Before this PR it was generating (which currently works until ampproject/amphtml#30589 is merged):

<amp-imgur width="580" height="870" data-imgur-id="2q6AS6i" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:580px;height:870px;" i-amphtml-layout="fixed"></amp-imgur>

Whereas with this PR, the result is:

<amp-imgur width="580" height="870" data-imgur-id="a/2q6AS6i" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:580px;height:870px;" i-amphtml-layout="fixed"></amp-imgur>

The explicit a/ here will ensure that https://imgur.com/a/2q6AS6i/embed?pub=true is the result.