Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Photon: Filter Responsive Images srcset array #2885

Merged
merged 4 commits into from
Oct 28, 2015

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Oct 23, 2015

Related (Resolves?): #2804

This pull request seeks to update Photon to adjust image URLs included in the srcset attribute introduced as part of the WordPress 4.4 "Responsive Images" feature.

This is a relatively naive approach. It simply filters the wp_get_attachment_image_srcset_array array applied by the core wp_get_attachment_image_srcset_array function. It may be preferable to hook in to and override the sizes returned by wp_get_attachment_metadata, as this is the mechanism used by the srcset behavior to determine viable responsive image candidates.

@aduth aduth added the [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins label Oct 23, 2015
@kraftbj kraftbj added this to the 3.8 milestone Oct 23, 2015
@kraftbj kraftbj added the [Type] Bug When a feature is broken and / or not performing as intended label Oct 23, 2015
@kraftbj
Copy link
Contributor

kraftbj commented Oct 25, 2015

I like the direction of this (relying on core's method overall for now). The downside is it Photonizes™ the resized image, not the full-size image with the Photon query args.

E.g. typically an image at https://kraft.im/uploads/IMG_20151018_2018382-520x700.jpg is converted to https://i2.wp.com/kraft.im/uploads/IMG_20151018_2018382.jpg?resize=520%2C700 and all other sizes of that image are dynamically resized from that original.

The PR just adds the i{0-2}.wp.com, but ignore the WP resized-to-full-size conversion. I'd need to play with it more, but I think we need to pull out the dimensions for the query string and remove it from the file name a la https://github.com/Automattic/jetpack/blob/master/class.photon.php#L268-L299

My initial thought is we need to abstract out the content filtering function and reference new functions (e.g. function create_photon_url_from_raw_img) to run within both foreach loops in this new srcset function and the original the_content function.

@kraftbj kraftbj force-pushed the update/photon-responsive-images branch from f3ee86f to 1ffcec2 Compare October 27, 2015 02:06
@zinigor
Copy link
Member

zinigor commented Oct 27, 2015

This looks good to me, works as expected!

@zinigor zinigor added the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 27, 2015
@aduth
Copy link
Contributor Author

aduth commented Oct 27, 2015

Based on some discussion in Core Slack and at Trac#34430 (34430.3.diff), should we expect that we won't need to scrape the width and height and instead receive the dimensions via the filter (as $size_array)?

@kraftbj
Copy link
Contributor

kraftbj commented Oct 27, 2015

@aduth Agreed, assuming .3 or similar functionality is merged.

Worst case, this branch functionally works as trunk stands right now and would work whether or not .3 is merged, so if we're ready for 3.8 and this is still pending, we can ship this and be good for now.

Better case, we can hold off until Core settles down and pick the most efficient/performant solution.

@dereksmart
Copy link
Member

Both the patch and the plan WFM -- Holding off on merging this until right before beta-2 (slated for wednesday EOD or early Thursday)

@kraftbj
Copy link
Contributor

kraftbj commented Oct 28, 2015

Core has been updated to include a sizes_array available via the filter. The downside is it only includes width.

The sources array passed through the filter outputs something akin to

Array ( [0] => Array ( [url] => https://kraft.im/uploads/IMG_20151018_2018382-297x400.jpg [descriptor] => w [value] => 297 ) [1] => Array ( [url] => https://kraft.im/uploads/IMG_20151018_2018382-520x700.jpg [descriptor] => w [value] => 520 ) [2] => Array ( [url] => https://kraft.im/uploads/IMG_20151018_2018382-1200x1617.jpg [descriptor] => w [value] => 1200 ) [3] => Array ( [url] => https://kraft.im/uploads/IMG_20151018_2018382-1250x1684.jpg [descriptor] => w [value] => 1250 ) [4] => Array ( [url] => https://kraft.im/uploads/IMG_20151018_2018382-400x539.jpg [descriptor] => w [value] => 400 ) ) 

The sizes_array: Array ( [0] => 520 [1] => 700 )

The image meta looks like Array ( [width] => 3040 [height] => 4096 [file] => IMG_20151018_2018382.jpg [sizes] => Array ( [thumbnail] => Array ( [file] => IMG_20151018_2018382-150x150.jpg [width] => 150 [height] => 150 [mime-type] => image/jpeg ) [medium] => Array ( [file] => IMG_20151018_2018382-297x400.jpg [width] => 297 [height] => 400 [mime-type] => image/jpeg ) [large] => Array ( [file] => IMG_20151018_2018382-520x700.jpg [width] => 520 [height] => 700 [mime-type] => image/jpeg ) [post-thumbnail] => Array ( [file] => IMG_20151018_2018382-1200x1617.jpg [width] => 1200 [height] => 1617 [mime-type] => image/jpeg ) [aesop-cover-img] => Array ( [file] => IMG_20151018_2018382-1250x1684.jpg [width] => 1250 [height] => 1684 [mime-type] => image/jpeg ) [aesop-tiny-cover] => Array ( [file] => IMG_20151018_2018382-400x539.jpg [width] => 400 [height] => 539 [mime-type] => image/jpeg ) [aesop-component] => Array ( [file] => IMG_20151018_2018382-1250x1684.jpg [width] => 1250 [height] => 1684 [mime-type] => image/jpeg ) [aesop-character] => Array ( [file] => IMG_20151018_2018382-200x200.jpg [width] => 200 [height] => 200 [mime-type] => image/jpeg ) [aesop-collection] => Array ( [file] => IMG_20151018_2018382-300x300.jpg [width] => 300 [height] => 300 [mime-type] => image/jpeg ) [aesop-grid-image] => Array ( [file] => IMG_20151018_2018382-400x539.jpg [width] => 400 [height] => 539 [mime-type] => image/jpeg ) [jetpack-portfolio-admin-thumb] => Array ( [file] => IMG_20151018_2018382-50x50.jpg [width] => 50 [height] => 50 [mime-type] => image/jpeg ) ) [image_meta] => Array ( [aperture] => 2 [credit] => [camera] => Nexus 6 [caption] => [created_timestamp] => 1445199526 [copyright] => [focal_length] => 3.82 [iso] => 243 [shutter_speed] => 0.022316 [title] => [orientation] => 0 [keywords] => Array ( ) ) )

@kraftbj
Copy link
Contributor

kraftbj commented Oct 28, 2015

I'm wondering if it still just makes sense to do how we're doing it. Relevant core change: https://core.trac.wordpress.org/changeset/35412

tl;dr: The PR still works with the changes. I'm not sure if the new changes to Core does anything to help us do it better.

@kraftbj
Copy link
Contributor

kraftbj commented Oct 28, 2015

@zinigor made a point in Slack that Photon can just take the width and figure out the height from there. Removing the call to determine the height would remove a preg_match call so should make this a little more performant.

@kraftbj kraftbj force-pushed the update/photon-responsive-images branch from 0c2788d to d6dda07 Compare October 28, 2015 15:14
@zinigor
Copy link
Member

zinigor commented Oct 28, 2015

Looks awesome! Tested again on latest nightly, works as expected.

dereksmart added a commit that referenced this pull request Oct 28, 2015
Photon: Filter Responsive Images srcset array
@dereksmart dereksmart merged commit 81cfb23 into master Oct 28, 2015
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 28, 2015
@dereksmart dereksmart deleted the update/photon-responsive-images branch October 28, 2015 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants