Add CORS proxy attribute for external PDFs

wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress

42 stars 16 forks source link

Add CORS proxy attribute for external PDFs #203

Closed goldenapples closed 7 years ago

goldenapples commented 7 years ago

See #192.

This is a very basic first pass at this option. Needs some testing for performance, and some ideas about filters to enable/disable this option, as it's definitely going to be performance hit in any case, and it would be good to allow sites with their own caching proxies set up to be able to use them instead.

goldenapples commented 7 years ago

Thanks for reviewing!

Those filter suggestions seem very reasonable. I'll add filters for each of those possibilities, and a bit of documentation around them.

goldenapples commented 7 years ago

Added filters for each of the options above:

shortcake_bakery_pdf_enable_cors_proxy - can be used to disable both the UI and the ajax handler for the CORS proxy.
shortcake_bakery_asset_proxy_stream_options - can be used to adjust the timeout or any other options for the stream used for asset proxying.
shortcake_bakery_pdf_asset_proxy_url - can be used to define a different asset proxy service for these resources.

One final question: I'm wondering if the default for shortcake_bakery_pdf_enable_cors_proxy should be set to false; since it's a feature that should probably only be enabled with conscious thought.

davisshaver commented 7 years ago

@goldenapples I'm pro disabled by default FWIW

davisshaver commented 7 years ago

This came up for us on another project and it sounds like we are going with a sideload solution. Seems like another potential option for CORS handling.

goldenapples commented 7 years ago

Yeah. I agree. I'll clean up and merge this and #205, so they can be used.

goldenapples commented 7 years ago

Ah, I found one more place where this behavior should be disabled by default - so that we're not showing the checkbox on the shortcode UI unless the site explicitly specifies to enable this feature.

Yeah, @davisshaver - do you mind giving this a final review & merge if it looks good to you? Thanks!

goldenapples commented 7 years ago

WPCS really doesn't like the fopen() calls here. But I don't think there's a reasonable way to use the WP_Filesystem API here, so I just ignored the codesniffs for those lines and left a comment about why.