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

Add support for shortcode embeds that enqueue scripts #9734

Merged
merged 2 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@notnownikki
Member

notnownikki commented Sep 10, 2018

Description

If the embed preview is handled by a shortcode, it might enqueue scripts to make it work.
This change returns the enqueued scripts as part of the embed response, and has
the Sandbox component put them into the preview document.

How has this been tested?

Install Jetpack and activate the shortcodes module.
Try to embed https://www.pinterest.co.uk/heald0081/hair-ideas/

The preview should appear as expected. Without this fix, only the caption appears.

Types of changes

Bug fix

Checklist:

  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@notnownikki notnownikki requested a review from WordPress/gutenberg-core Sep 10, 2018

// Check if any scripts were enqueued by the shortcode, and
// include them in the response.
$enqueued_scripts = array();
foreach ( $wp_scripts->queue as $script ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 13, 2018

Member

In my local tests, I'm still having problems when I try to embed http://pinterest.co.uk/heald0081/hair-ideas. I checked that $wp_scripts->queue is empty which I assume was unexpected.
I changed Gutenberg in a poopy.life sandbox to contain:

if ( $html ) {
					global $wp_scripts;
					var_dump( $wp_scripts->queue );
					exit;

And I checked $wp_scripts->queue was also empty there. I'm not certain about the reason for this. Maybe I'm doing something wrong.

@jorgefilipecosta

jorgefilipecosta Sep 13, 2018

Member

In my local tests, I'm still having problems when I try to embed http://pinterest.co.uk/heald0081/hair-ideas. I checked that $wp_scripts->queue is empty which I assume was unexpected.
I changed Gutenberg in a poopy.life sandbox to contain:

if ( $html ) {
					global $wp_scripts;
					var_dump( $wp_scripts->queue );
					exit;

And I checked $wp_scripts->queue was also empty there. I'm not certain about the reason for this. Maybe I'm doing something wrong.

This comment has been minimized.

@notnownikki

notnownikki Sep 14, 2018

Member

My bet is you tested to make sure it was broken, applied the patch, then tested again? Clear all your embed transients and see if it works :)

@notnownikki

notnownikki Sep 14, 2018

Member

My bet is you tested to make sure it was broken, applied the patch, then tested again? Clear all your embed transients and see if it works :)

This comment has been minimized.

@notnownikki

notnownikki Sep 14, 2018

Member

If not (it hit me a few times!) Can you check that the call to enqueued script happens in the short code?

@notnownikki

notnownikki Sep 14, 2018

Member

If not (it hit me a few times!) Can you check that the call to enqueued script happens in the short code?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

I'm still having problems with the embed of Pinterest, I used a plugin the clear all the transients and the situation persisted. I checked that Pinterest is not a WordPress supported embed in the classic editor https://codex.wordpress.org/Embeds, but JetPack adds a shortcode for it. Do you have JetPack installed in your testing environment? maybe that explains the reason for the difference in the tests because I'm not using JetPack on the environment I'm testing.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

I'm still having problems with the embed of Pinterest, I used a plugin the clear all the transients and the situation persisted. I checked that Pinterest is not a WordPress supported embed in the classic editor https://codex.wordpress.org/Embeds, but JetPack adds a shortcode for it. Do you have JetPack installed in your testing environment? maybe that explains the reason for the difference in the tests because I'm not using JetPack on the environment I'm testing.

This comment has been minimized.

@notnownikki

notnownikki Sep 17, 2018

Member

Yes, this needs Jetpack with the shortcodes module active.

@notnownikki

notnownikki Sep 17, 2018

Member

Yes, this needs Jetpack with the shortcodes module active.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

Ah sorry for the troubles, it works great with JetPAck shortcodes module active.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

Ah sorry for the troubles, it works great with JetPAck shortcodes module active.

@jorgefilipecosta

Things worked correctly in my tests 👍 I had some concerns about the security implications of loading this scripts, but they are loaded inside the sandbox iframe and we had a comment there saying "we can use this in the future to inject custom styles or scripts" so it looks like the component was already built with script injection in mind.

@notnownikki notnownikki merged commit 94be4e3 into master Sep 18, 2018

2 checks passed

codecov/project 49.01% (-0.02%) compared to 410f78a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notnownikki notnownikki added this to the 4.0 milestone Sep 18, 2018

@youknowriad youknowriad deleted the fix/embed-responses-with-scripts branch Sep 18, 2018

@@ -87,9 +87,17 @@ function gutenberg_filter_oembed_result( $response, $handler, $request ) {
global $wp_embed;
$html = $wp_embed->shortcode( array(), $_GET['url'] );
if ( $html ) {
global $wp_scripts;

This comment has been minimized.

@aduth

aduth Sep 18, 2018

Member

Is there an upstream Trac ticket tracking these enhancements to the oEmbed endpoint?

If so, should we add a comment about this addition?
If not, should we create one?

@aduth

aduth Sep 18, 2018

Member

Is there an upstream Trac ticket tracking these enhancements to the oEmbed endpoint?

If so, should we add a comment about this addition?
If not, should we create one?

This comment has been minimized.

@notnownikki

notnownikki Sep 18, 2018

Member

I'm not sure... @pento @mkaz do you know if there's a ticket/tickets open?

@notnownikki

notnownikki Sep 18, 2018

Member

I'm not sure... @pento @mkaz do you know if there's a ticket/tickets open?

This comment has been minimized.

@pento

pento Sep 19, 2018

Member

There isn't yet. I was leaving this one until we're looking to merge for 5.0, as gutenberg_filter_oembed_result() is a giant hack that will be have to be mostly rewritten. So far, the things that have been merged to Core have been the ones we could mostly just copy/pasta.

@pento

pento Sep 19, 2018

Member

There isn't yet. I was leaving this one until we're looking to merge for 5.0, as gutenberg_filter_oembed_result() is a giant hack that will be have to be mostly rewritten. So far, the things that have been merged to Core have been the ones we could mostly just copy/pasta.

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