Skip to content

New filter to allow running Photon in wp-admin#6702

Merged
samhotchkiss merged 2 commits intomasterfrom
update/allow-photon-admin
Mar 27, 2017
Merged

New filter to allow running Photon in wp-admin#6702
samhotchkiss merged 2 commits intomasterfrom
update/allow-photon-admin

Conversation

@mjangda
Copy link
Copy Markdown
Member

@mjangda mjangda commented Mar 21, 2017

Proposed new filter is called: jetpack_photon_admin_allow_image_downsize.

This will allow Photon's image_downsize callback to run when viewing the Dashboard. For environments where all intermediate images are dynamically generated (e.g. WordPress.com, VIP Go), this can come in handy as we can re-use the same imagerresizing logic on both front-end and dashboard. Currently Photon just bails early when in an is_admin() context.

Usage:

add_filter( 'jetpack_photon_admin_allow_image_downsize', '__return_true' );

/cc @kraftbj

For environments where all intermediate images are dynamically generated
(e.g. WordPress.com, VIP Go), this can come in handy as we can re-use
the same imagerresizing logic on both front-end and dashboard.
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is_admin bailout is original to Photon (JP 2.0) and, IIRC, in place to avoid Photon from caching images before they're used in public-facing ways.

On a typical WP install, this is fine since WP still creates intermediate sizes.

For sites that want to rely exclusively on dynamic image sizing, this filter or another solution to overriding the default is needed.

Adding the filter preserves the current experience for the majority while providing a method for the evolving minority.

LGTM

@kraftbj kraftbj added [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Pri] Normal labels Mar 21, 2017
*/
apply_filters( 'jetpack_photon_override_image_downsize', false, compact( 'image', 'attachment_id', 'size' ) )
)
false === apply_filters( 'jetpack_photon_admin_allow_image_downsize', false, compact( 'image', 'attachment_id', 'size' ) )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that Photon URLs would then be saved in the database for each post includes images? That would make it difficult for one to move away from Photon later on, as it would require finding and replacing all Photon URLs in posts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but let me re-test checking that specifically.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.

The use case would be for a site that does not generate any intermediate sizes and would want to rely on Photon fully (VIP Go, in this case). Do you think this is acceptable, if we add a clear warning about the implications of this or the alternative is break out the logic for usage from the actual functionality. VIP Go, or other site owners, could remove the default filter on image_downsize from Photon and add their own that directly calls a new function that performs the work without checking any logic?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine, as long as the VIP Go knows to warn their clients about it whenever they want to stop using Photon. @mjangda What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go sites actually run their own mini instances of Photon served from the main site's domain (e.g. https://wpjobmanager.com/wp-content/uploads/2014/02/2014-02-27-at-22.36-1024x609.png?w=100) and we filter the Photon domain accordingly. So the URLs we'd be saving for Go sites would be their own domain. So, we're basically using the Photon code but not the public Photon service :)

Definitely makes sense to have a clear warning there though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added warning in 4f2fb41

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the update, I've marked this ready for merge for one of the Pit Crew to do the final sign-off.

@kraftbj kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 21, 2017
@mjangda mjangda added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 21, 2017
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 21, 2017
@samhotchkiss samhotchkiss merged commit 223c2d9 into master Mar 27, 2017
@samhotchkiss samhotchkiss deleted the update/allow-photon-admin branch March 27, 2017 17:27
@samhotchkiss samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 27, 2017
jeherve added a commit that referenced this pull request Mar 28, 2017
samhotchkiss pushed a commit that referenced this pull request Mar 29, 2017
* Readme: remove old release and add skeleton for 4.8.

* Changelog: add #6572

* Changelog: add #6567

* Changelog: add #6542

* Changelog: add #6527

* Changelog: add #6508

* Changelog: add #6478

* Changelog: add #6477

* Changelog: add #6249

* Update stable version and remove old version from readme.

* Changelog: add 4.7.1 to changelog.

* Readme: add new contributor.

* Sync: update docblock @SInCE version.

Related: #6053

* Changelog: add release post.

* changelog: add #6053

* Changelog: add #6413

* Changelog: add #6482

* Changelog: add #6584

* Changelog add #6603

* Changelog: add #6606

* Changelog: add #6611

* Changelog: add #6635

* Changelog: add #6639

* Changelog: add #6684

* Changelog: add #6710

* Changelog: add #6711

* Changelog: add #5461

* Testing list: update Settings UI feedback prompt.

Props @MichaelArestad

* Changelog: add #6789

* Changelog: add #6778

* Changelog: add #6777

* Changelog: add #6775

* Changelog: add #6755

* Changelog: add #6731

* Changelog: add #6721

* Changelog: add #6705

* Changelog: add #6702

* Changelog: add #6671

* Changelog: add #6637

* Changelog: add #6582

* Changelog: add #6566

* Changelog: add #6555

* Changelog: add #6529

* Changelog: add #6344

* Changelog: add #5763

* Changelog: add #5503

* Changelog: update #6637 changelog.

@see 40e115c#commitcomment-21523982

* Changelog: add #6699

* Changelog: add #6632

* Changelog: add #6769

* Changelog: add #6707

* Changelog: add #6590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Pri] Normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants