Missing height/width parameter returned from wp_get_attachment_image_src() when Photon is enabled #607

Closed
tollmanz opened this Issue May 29, 2014 · 21 comments

Comments

Projects
None yet
5 participants
@tollmanz

I have been experiencing some issues with themes that I maintain for users who have Photon enabled, which started some time within the last few days. In the themes I work on, we have a number of places in the code where we call wp_get_attachment_image_src() and check the value of the first and second indices to test the image size against certain requirements. When Photon is enabled, those values always return false. Some example code is:

/**
 * Check if a particular size of an image from the media library meets minimum dimension requirements.
 *
 * @since 1.0.
 *
 * @param  int    $image_id   The image id.
 * @param  array  $min_size   The minimum width and height of the image.
 * @param  string $image_size The defined image size to use when analyzing the image.
 * @return array|bool         Image src, width, and height if true, otherwise false.
 */
function my_is_image_large_enough( $image_id, $min_size = array(), $image_size = 'large' ) {
    if ( empty( $min_size ) ) {
        return false;
    }

    // Get the actual dimensions of the closest generated thumbnail size
    $image = wp_get_attachment_image_src( $image_id, $image_size );

    // It's large enough.
    if ( $image[1] >= $min_size[0] && $image[2] >= $min_size[1] ) {
        return $image;
    }

    // Not large enough.
    return false;
}

To reproduce:

  1. Add the following code to functions.php (change the ID if you don't have a 1 image):

    var_dump( wp_get_attachment_image_src( 1, 'full' ) );
  2. Notice that the first and second indices are width and height values

  3. Turn on Photon

  4. Reload the page

  5. Notice that the first and second indices are both false

This appears to be occurring on .org and .com. It seems to have just started a few days ago.

I see this as fairly urgent as changes to these values can cause pretty big problems.

@jeherve jeherve added Photon labels May 29, 2014

@jeherve

This comment has been minimized.

Show comment
Hide comment
Member

jeherve commented May 29, 2014

@blobaugh

This comment has been minimized.

Show comment
Hide comment
@blobaugh

blobaugh May 30, 2014

Contributor

Spoke with @tollmanz on Skype as this issue was being written. After a bit of digging into message archives I surmise the following from a comment from @ethitter:

When https://github.com/Automattic/jetpack/blob/master/class.photon.php#L385-L389 happens we do not yet have an image. All it does is build the url to retrieve the image. Even given the $image dimensions, in order to keep the aspect ration the resulting image may not be that exact size. So the only way to know for sure would be to retrieve the image from Photon, but that would negate the positive impacts Photon has on your site. So instead they chose to just not supply image dimensions.

Some validation on that idea would be appreciated.

Contributor

blobaugh commented May 30, 2014

Spoke with @tollmanz on Skype as this issue was being written. After a bit of digging into message archives I surmise the following from a comment from @ethitter:

When https://github.com/Automattic/jetpack/blob/master/class.photon.php#L385-L389 happens we do not yet have an image. All it does is build the url to retrieve the image. Even given the $image dimensions, in order to keep the aspect ration the resulting image may not be that exact size. So the only way to know for sure would be to retrieve the image from Photon, but that would negate the positive impacts Photon has on your site. So instead they chose to just not supply image dimensions.

Some validation on that idea would be appreciated.

@ethitter

This comment has been minimized.

Show comment
Hide comment
@ethitter

ethitter May 30, 2014

Contributor

The Photon module has always omitted the image dimensions when filtering wp_get_attachment_image_src(). We made this decision when first building the module because while we know the original dimensions, and images are likely to come back from Photon at the same dimensions, we can't be sure that this is the case. The Photon service doesn't return any meta about the resized image, so to avoid possible distortion of the image, we omit the dimensions. We've discussed various ways we could include the dimensions (returning meta via a different endpoint and caching the remote request, for example), but none are considered suitable from a performance perspective.

Contributor

ethitter commented May 30, 2014

The Photon module has always omitted the image dimensions when filtering wp_get_attachment_image_src(). We made this decision when first building the module because while we know the original dimensions, and images are likely to come back from Photon at the same dimensions, we can't be sure that this is the case. The Photon service doesn't return any meta about the resized image, so to avoid possible distortion of the image, we omit the dimensions. We've discussed various ways we could include the dimensions (returning meta via a different endpoint and caching the remote request, for example), but none are considered suitable from a performance perspective.

@blobaugh blobaugh added the wontfix label May 30, 2014

@blobaugh blobaugh closed this May 30, 2014

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

Closed already?

I think the code that @blobaugh pointed out is a red herring. For whatever reason, up until a few days ago, I could get dimensions back from wp_get_attachment_image_src(). Something changed and this is breaking code that tests against those values. Was there a change in the server side component of Photon that could account for this?

Closed already?

I think the code that @blobaugh pointed out is a red herring. For whatever reason, up until a few days ago, I could get dimensions back from wp_get_attachment_image_src(). Something changed and this is breaking code that tests against those values. Was there a change in the server side component of Photon that could account for this?

@blobaugh blobaugh reopened this May 30, 2014

@blobaugh

This comment has been minimized.

Show comment
Hide comment
@blobaugh

blobaugh May 30, 2014

Contributor

"The Photon module has always omitted the image dimensions" < That is why I closed it.

Did you change anything in your config that could have caused the issue to manifest? Was Photon enabled before? ;)

Contributor

blobaugh commented May 30, 2014

"The Photon module has always omitted the image dimensions" < That is why I closed it.

Did you change anything in your config that could have caused the issue to manifest? Was Photon enabled before? ;)

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

@blobaugh Thanks for reopening! I know that I was the one to throw that red herring out there so sorry for that.

The code in question is in a number of Theme Foundry themes. On both .org and .com (more so on .com), we've received complaints that images weren't showing up all of the sudden. Each of these cases involved code that check image dimensions before displaying the images. We were able to fire up a test site using JetPack 3.0.1 with Photon enabled that confirmed the issue. Turning off Photon fixed the issue. We also put together a quick work around that retrieves dimensions from the Photon URL, which fixed the issue on .org, but not .com.

On 5/29, we received three reports of images going missing on .com, which makes me think something about the Photon service changed that caused this issue. While those values have been set to false for some time, it seems that it didn't cause an issue until yesterday or a few days ago. This is tricky for me to debug because I cannot get insight into the .com side of things.

One of our devs, @coreymckrill, pointed out that another commit could possibly be involved: 8b6608e.

@blobaugh Thanks for reopening! I know that I was the one to throw that red herring out there so sorry for that.

The code in question is in a number of Theme Foundry themes. On both .org and .com (more so on .com), we've received complaints that images weren't showing up all of the sudden. Each of these cases involved code that check image dimensions before displaying the images. We were able to fire up a test site using JetPack 3.0.1 with Photon enabled that confirmed the issue. Turning off Photon fixed the issue. We also put together a quick work around that retrieves dimensions from the Photon URL, which fixed the issue on .org, but not .com.

On 5/29, we received three reports of images going missing on .com, which makes me think something about the Photon service changed that caused this issue. While those values have been set to false for some time, it seems that it didn't cause an issue until yesterday or a few days ago. This is tricky for me to debug because I cannot get insight into the .com side of things.

One of our devs, @coreymckrill, pointed out that another commit could possibly be involved: 8b6608e.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

That commit I believe set it so that we wouldn't 'eat' a data-width or data-height attribute -- only a straight width or height attribute.

@tollmanz -- As you're able to reproduce it in Jetpack, can you try using git bisect to confirm when was the cause?

Member

georgestephanis commented May 30, 2014

That commit I believe set it so that we wouldn't 'eat' a data-width or data-height attribute -- only a straight width or height attribute.

@tollmanz -- As you're able to reproduce it in Jetpack, can you try using git bisect to confirm when was the cause?

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

( and for those that are unfamiliar with it, git bisect is amaaaaazing -- http://robots.thoughtbot.com/git-bisect )

Member

georgestephanis commented May 30, 2014

( and for those that are unfamiliar with it, git bisect is amaaaaazing -- http://robots.thoughtbot.com/git-bisect )

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

Good idea @georgestephanis! This will take some time to get set up since I need a public facing site to test this and the one I was using before isn't very convenient for running Git commands on it.

Good idea @georgestephanis! This will take some time to get set up since I need a public facing site to test this and the one I was using before isn't very convenient for running Git commands on it.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

@tollmanz You should be able to run it in VVV in local mode by changing the module header to lie and say it can be run without a wpcom connection -- as we're testing the code that processes the data, we don't care if the image actually pulls in.

Member

georgestephanis commented May 30, 2014

@tollmanz You should be able to run it in VVV in local mode by changing the module header to lie and say it can be run without a wpcom connection -- as we're testing the code that processes the data, we don't care if the image actually pulls in.

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

I was able to test this and I cannot even get started with git bisect as I cannot identify a good commit.

This is consistent with the fact that those values have set as false for sometime. Is it possible that a change to the server side of the Photon component in the last week or so can be messing this up?

I was able to test this and I cannot even get started with git bisect as I cannot identify a good commit.

This is consistent with the fact that those values have set as false for sometime. Is it possible that a change to the server side of the Photon component in the last week or so can be messing this up?

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

I don't see how. The server side is pretty much just https://code.trac.wordpress.org/browser/photon/

Member

georgestephanis commented May 30, 2014

I don't see how. The server side is pretty much just https://code.trac.wordpress.org/browser/photon/

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

Also, the server-side functions as a pull-CDN. It would have nothing to do with variables being passed to the img tag.

Member

georgestephanis commented May 30, 2014

Also, the server-side functions as a pull-CDN. It would have nothing to do with variables being passed to the img tag.

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

Thanks for that code! I didn't know Photon was publicly available. I'll keep digging.

Thanks for that code! I didn't know Photon was publicly available. I'll keep digging.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

cc: @darthhexx in case any of this is ringing any bells.

(Again, I doubt it could be related to the server side stuff, but he's been tinkering with that lately, so who knows)

Member

georgestephanis commented May 30, 2014

cc: @darthhexx in case any of this is ringing any bells.

(Again, I doubt it could be related to the server side stuff, but he's been tinkering with that lately, so who knows)

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

@georgestephanis Thanks again for taking a look into this. I've had some luck with this:

add_filter( 'jetpack_photon_override_image_downsize', '__return_true' );
$info = wp_get_attachment_image_src( $attachment_id, 'full' );
remove_filter( 'jetpack_photon_override_image_downsize', '__return_true' );

I get real values back. The concern is that this appears to work well on .org, but not .com. I'm beginning to think something else is going on here.

@georgestephanis Thanks again for taking a look into this. I've had some luck with this:

add_filter( 'jetpack_photon_override_image_downsize', '__return_true' );
$info = wp_get_attachment_image_src( $attachment_id, 'full' );
remove_filter( 'jetpack_photon_override_image_downsize', '__return_true' );

I get real values back. The concern is that this appears to work well on .org, but not .com. I'm beginning to think something else is going on here.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

It looks like those filters may not be used on .com -- the photon module codebase isn't a full sync between jetpack and wpcom. One sec, I'm gonna dig something up.

Member

georgestephanis commented May 30, 2014

It looks like those filters may not be used on .com -- the photon module codebase isn't a full sync between jetpack and wpcom. One sec, I'm gonna dig something up.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

Actually, I don't see that filter in Jetpack either via a quick search.

Hold on, doing some digging on my own.

Member

georgestephanis commented May 30, 2014

Actually, I don't see that filter in Jetpack either via a quick search.

Hold on, doing some digging on my own.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

found in jetpack

if ( is_admin() || apply_filters( 'jetpack_photon_override_image_downsize', false, compact( 'image', 'attachment_id', 'size' ) ) )

For some reason search was just failing me.

Member

georgestephanis commented May 30, 2014

found in jetpack

if ( is_admin() || apply_filters( 'jetpack_photon_override_image_downsize', false, compact( 'image', 'attachment_id', 'size' ) ) )

For some reason search was just failing me.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis May 30, 2014

Member

(I'm sure you've already done all this digging yourself, it just helps me to formalize my thoughts as I dig to jot down my own breadcrumbs)

Context: source of wp_get_attachment_image_src() -- https://github.com/WordPress/WordPress/blob/3.9.1/wp-includes/media.php#L645-L674

The only way that the src could return something and width and height both evaluate to false is if the first early return kicks off --

// get a thumbnail or intermediate image if there is one
if ( $image = image_downsize($attachment_id, $size) )
    return $image;

As otherwise it would return false --

if ( $src && $width && $height )
    return array( $src, $width, $height );
return false;

So the problem likely has to be in the image_downsize() function -- https://github.com/WordPress/WordPress/blob/3.9.1/wp-includes/media.php#L124-L203

Which has an early fail out on the image_downsize filter -- where if it gets anything, it kicks out early. As we're using that and feeding data, it's doing that here -- https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L58-L59

The function is https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L325-L416

Incidentally, while functions.photon.php is synced with wpcom, class.photon.php is not.

full is a special case ... https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L355-L367

Which it sets $image_args['width'] and $image_args['height'] both equal to the original metadata height and width as stored in the db.

crop is true, so $transform is resize not fit

But in the end, everything comes down to https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L384-L389 manually setting false in the responses.

@tollmanz -- how would you feel about adding a filter onto line 415 return $image; to make it return apply_filters( 'photon_filter_image_downsize_results', $image, $attachment_id, $size ); -- I'd have preferred to pass $image_args directly, but that's only used by the first conditional block, not the latter, so it would be inconsistent without some greater structural change.

At least that's how we're doing it in Jetpack, I'm about to dive into wpcom now.

Member

georgestephanis commented May 30, 2014

(I'm sure you've already done all this digging yourself, it just helps me to formalize my thoughts as I dig to jot down my own breadcrumbs)

Context: source of wp_get_attachment_image_src() -- https://github.com/WordPress/WordPress/blob/3.9.1/wp-includes/media.php#L645-L674

The only way that the src could return something and width and height both evaluate to false is if the first early return kicks off --

// get a thumbnail or intermediate image if there is one
if ( $image = image_downsize($attachment_id, $size) )
    return $image;

As otherwise it would return false --

if ( $src && $width && $height )
    return array( $src, $width, $height );
return false;

So the problem likely has to be in the image_downsize() function -- https://github.com/WordPress/WordPress/blob/3.9.1/wp-includes/media.php#L124-L203

Which has an early fail out on the image_downsize filter -- where if it gets anything, it kicks out early. As we're using that and feeding data, it's doing that here -- https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L58-L59

The function is https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L325-L416

Incidentally, while functions.photon.php is synced with wpcom, class.photon.php is not.

full is a special case ... https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L355-L367

Which it sets $image_args['width'] and $image_args['height'] both equal to the original metadata height and width as stored in the db.

crop is true, so $transform is resize not fit

But in the end, everything comes down to https://github.com/Automattic/jetpack/blob/3.0.1/class.photon.php#L384-L389 manually setting false in the responses.

@tollmanz -- how would you feel about adding a filter onto line 415 return $image; to make it return apply_filters( 'photon_filter_image_downsize_results', $image, $attachment_id, $size ); -- I'd have preferred to pass $image_args directly, but that's only used by the first conditional block, not the latter, so it would be inconsistent without some greater structural change.

At least that's how we're doing it in Jetpack, I'm about to dive into wpcom now.

@tollmanz

This comment has been minimized.

Show comment
Hide comment
@tollmanz

tollmanz May 30, 2014

@georgestephanis helped me debug this issue on WordPress.com and it is now clear that this has nothing to do with JetPack. Thanks for looking at this issue everyone.

@georgestephanis helped me debug this issue on WordPress.com and it is now clear that this has nothing to do with JetPack. Thanks for looking at this issue everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment